Skip to content

Conversation

@KarryCharon
Copy link

Due to the fact that many third-party custom_nodes use getTitle() to implement predetermined logic, we cannot use the title field for node panel translation. Therefore, we have added the label as a translation field here.
See this problem we encountered:
AIGODLIKE/AIGODLIKE-ComfyUI-Translation#255

@webfiltered
Copy link
Contributor

Hi there, just saw this and wanted to flag a few things:

  1. title property itself is referenced by a few size calculations
  2. getTitle() is also called on collapsed nodes (appears to be size calc also)
  3. Subgraph and panel code also directly reference node title
  4. Other extensions may use the title and be broken by this

Possible solution

i18n is important, so initial suggestion:

  1. Check ComfyUI repo(s) consuming litegraph for references to title or LGraphNode.getTitle()
  2. Community check to see if it could impact their UI-heavy extensions

And the updated change, which I can help with:

  1. Use title as a backend field everywhere - i.e. no change
  2. Add a display title property - something that indicates it's only for display, and localised (displayTitle / localisedTitle?). Maybe stuff all possible values inside an i18n property which could be null checked if no localisation in use.
  3. Find/replace all references to title with calls to getTitle() or new localised getter

Workaround

Alternatively, if you are -currently- localising a title by overwriting the LGraphNode.title, you could just recurse through the anything everywhere nodes when added to the graph, and update them with the titles you're setting.

@KarryCharon
Copy link
Author

KarryCharon commented Aug 6, 2024

Hi there, just saw this and wanted to flag a few things:

  1. title property itself is referenced by a few size calculations
  2. getTitle() is also called on collapsed nodes (appears to be size calc also)
  3. Subgraph and panel code also directly reference node title
  4. Other extensions may use the title and be broken by this

Possible solution

i18n is important, so initial suggestion:

  1. Check ComfyUI repo(s) consuming litegraph for references to title or LGraphNode.getTitle()
  2. Community check to see if it could impact their UI-heavy extensions

And the updated change, which I can help with:

  1. Use title as a backend field everywhere - i.e. no change
  2. Add a display title property - something that indicates it's only for display, and localised (displayTitle / localisedTitle?). Maybe stuff all possible values inside an i18n property which could be null checked if no localisation in use.
  3. Find/replace all references to title with calls to getTitle() or new localised getter

Workaround

Alternatively, if you are -currently- localising a title by overwriting the LGraphNode.title, you could just recurse through the anything everywhere nodes when added to the graph, and update them with the titles you're setting.

Yes, we considered that. So we use the node.label field for display only, it will not impact other custom_nodes funcationality (such as size calc things you said).

@KarryCharon
Copy link
Author

@webfiltered As a result, most developers do not need to care about this node.label filed, and just use getTitle() to get node real title like before.

@webfiltered
Copy link
Contributor

Yes, we considered that. So we use the node.label field for display only, it will not impact other custom_nodes funcationality (such as size calc things you said).

Size calculations in litegraph will unfortunately be impacted - litegraph itself reads the title property to determine how wide to make a node, how much to collapse it, etc.

@KarryCharon
Copy link
Author

Yes, we considered that. So we use the node.label field for display only, it will not impact other custom_nodes funcationality (such as size calc things you said).

Size calculations in litegraph will unfortunately be impacted - litegraph itself reads the title property to determine how wide to make a node, how much to collapse it, etc.

Can you show me which LOC of js file? I'm so sorry about that I'm not familier enough with litegraph framework eh...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants