[feat]: hide widget value when connected #1077
Conversation
yiqun12
changed the title
yiqun12
changed the title
src/widgets/BaseWidget.ts
Outdated
| /** Synonym for "low quality". */ | ||
| showText?: boolean | ||
| /** Hides the value of the widget, showing only the label. */ | ||
| stripValue?: boolean |
There was a problem hiding this comment.
Should this be a property of the base class? Or just specific subclasses?
There was a problem hiding this comment.
hmm Good point — stripValue really only applies to display-only (UI) widgets, so moving it out of the base type makes sense. I’ll extract a DisplayWidgetOptions interface and apply it where needed.
There was a problem hiding this comment.
I did a careful review of the codebase and decided to keep it in the base class since it's easier to extend in the future. Otherwise, we would need to modify each subclass (e.g., NumberWidget, StringWidget, BoolWidget), which would make the code harder to maintain as we add more widget types. The property also fits well with the existing fields already defined in the base class.
There was a problem hiding this comment.
Should I update playwright screenshot for comparison?
yiqun12
changed the title
yiqun12
changed the title
src/widgets/BaseWidget.ts
Outdated
| const { displayName } = this | ||
| const x = margin * 2 + leftPadding | ||
| const totalWidth = width - x - 2 * margin - rightPadding | ||
| const area = new Rectangle(x, y, totalWidth, height * 0.7) | ||
|
|
||
| ctx.fillStyle = this.secondary_text_color | ||
|
|
||
| if (stripValue) { | ||
| // When the value is hidden, only show the name of the widget | ||
| drawTextInArea({ ctx, text: displayName, area, align: "left" }) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Does it need to be moved up here? It seems like we are making the diff largely than it needs to be. Also, we are now destructuring this twice. Third, we are now potentially changing behavior because we are not restoring the context at the end of text draw (ctx.fillStyle = this.secondary_text_color) in the same sequence as before.
There was a problem hiding this comment.
Hey Christian, actually the main change here is due to some code style cleanup, which made the diff look bigger than it actually is. The behavior remains unchanged. I also did a small optimization while cleaning it up.
There was a problem hiding this comment.
I think this code is ready for review.
webfiltered
left a comment
webfiltered
left a comment
There was a problem hiding this comment.
I am a little confused by this, to be honest. It's got basically the same problem here as we went over in one of the previous PRs: instead of simply setting a property on the widget (or using an existing one), the PR is passing the stripValue param down through nested function calls.
A stripValue property has been declared on the base class, but it is not actually used.
Hmm... Ty for clarifying. I think i am getting this. Let me check the solution for not passing flags through many layers of code. |
Good point, I think I understand what you mean now. I referred to how widget.computedDisabled is used, and added a computedStripValue property on BaseWidget to control this behavior. Indeed, passing stripValue down through multiple function parameters felt clunky. |
Previously, when a widget was connected to an input, both its name and value mismatch, making it hard to understand the widget’s value at a glance. issue #4025 Comfy-Org/ComfyUI_frontend#4025
This commit updates the drawing logic to improve clarity:
Only hides the value when an input is connected
Keeps the widget name/label visible