Skip to content

Widen value direct-edit box to the max label width - #2834

Open
franz-ms-muc wants to merge 1 commit into
eclipse-4diac:developfrom
Meisterschulen-am-Ostbahnhof-Munchen:fix/1852-value-cell-editor-width
Open

Widen value direct-edit box to the max label width#2834
franz-ms-muc wants to merge 1 commit into
eclipse-4diac:developfrom
Meisterschulen-am-Ostbahnhof-Munchen:fix/1852-value-cell-editor-width

Conversation

@franz-ms-muc

@franz-ms-muc franz-ms-muc commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What & why

The direct-edit control for an FB pin's or parameter's Value label
was sized to exactly the previously rendered label's width. For a
short value (e.g. a single digit) that could be only a handful of
pixels wide, and for a struct/array value the "..." dialog button
shown next to the text field further narrowed the available text
space. Typing a longer replacement value then scrolled almost all of
the typed text out of view immediately - see the underlying problem
described in #1852.

Fix

Reworked again per review feedback
(#2834 (comment)):
the previous version checked the variable's type
(StructuredType/isArray()) directly in ValueEditPart to guess
whether a "..." dialog button would be shown and pad the label width
for it - duplicating the same check already done in
InitialValueDirectEditManager/InitialValueVariableDirectEditManager,
and widening the static label even while not being edited.

  • ValueEditPart's ValueFigure keeps a 5-character minimum
    preferred width so it does not collapse while empty, and
    refreshPosition() (now public) is called from
    ValueEditPartChangeEditPolicy.showCurrentEditValue() on every
    keystroke, so the figure - and with it the underlying GEF cell
    editor - grows/shrinks live to fit the typed text, still capped by
    the existing "Maximum value label size" preference
    (getMaxWidth()). ValueEditPart no longer knows anything about
    dialog buttons.
  • FigureCellEditorLocator (which positions the actual SWT direct-
    edit control over the figure) now additionally measures the
    control's own fixed-size sibling widgets when it is a Composite
    laid out with GridLayout - e.g. a struct/array value editor's
    "..." dialog button next to its text field - and widens just the
    live edit box by that amount. This is generic (no type-checking of
    the variable at all): it just asks the actual control's layout what
    room its non-growing children need.
  • For an input parameter, whose value sits to the left of the block,
    that extra width is added on the left so the box's right edge stays
    anchored to the block's border instead of growing into the block;
    for an output parameter it grows to the right.

Verified

Manually, live, in a build produced from this branch: a short-valued
input pin, an output pin, and a struct-typed input pin, each edited
with a much longer replacement value; confirmed the box grows live
while typing (and shrinks back when deleting), stays comfortably
sized without being needlessly wide for short values, and that the
struct dialog button has full room next to the text field without
crowding it, only while actually editing.

No automated test is included, for the same reason as PR #2833: no
test infrastructure exists in this repo for GEF direct-edit/cell-
editor positioning behavior.

🤖 Generated with Claude Code

@azoitl azoitl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels very bad to me. In all of my test cases the box was way to large, may value was jumping around. Aren't there better way to do this then forcing them strange setting on me.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Test Results

  128 files  ±0    128 suites  ±0   1m 29s ⏱️ +22s
6 245 tests ±0  6 244 ✅ ±0  1 💤 ±0  0 ❌ ±0 
6 246 runs  ±0  6 245 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit 18acf1e. ± Comparison against base commit 512062e.

♻️ This comment has been updated with latest results.

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

This feels very bad to me.

The Video in the Linked issue #1852 shows that the Box is about to hide half of the Content. so the Boxes are too small, uncomfortably small.

see a current picture here:

New: (with #2834)
image

Before:
image
The Text in the Box is Output_Q1

see:

image

In all of my test cases the box was way to large, may value was jumping around.

i would be interested to see.

Aren't there better way to do this then forcing them strange setting on me.

I could imagine set this as a seperate Parameter not bound to Maximum value label size ? but does this really increase comfort to have one more Parameter to set ?

@azoitl

azoitl commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

No new preference is definitely not the right solution. A preference is always added when the developer can not decide.

Looking at your screenshots maybe a better solution is to create a text box that is minimum as large as the current content (for structs plus the button width). But at maximum the max label width. WDYT?

@azoitl

azoitl commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

In all of my test cases the box was way to large, may value was jumping around.

i would be interested to see.

Your Softkey picture is already a good example. I was clicking on a small value like soft. Now soft is moving three times the size of soft to the right, there is not my mouse there are not my eyes. Very bad user experience.

@azoitl

azoitl commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Looking at your screenshots maybe a better solution is to create a text box that is minimum as large as the current content (for structs plus the button width). But at maximum the max label width. WDYT?

As I thought a bit more. I guess also one cursor length spacing should be added as well.

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

No new preference is definitely not the right solution. A preference is always added when the developer can not decide.

Looking at your screenshots maybe a better solution is to create a text box that is minimum as large as the current content (for structs plus the button width). But at maximum the max label width. WDYT?

yes. but then when it was empty before it is 5 character wide ...

the rest i Agree, say there is SoftKey_F1 this might change to SoftKey_F12 at max, so that one character scrolling is still not nice but somehow acceptable.

but if there is 0 as a default Value, then it looks like this:

image

I would say if it is always that 5 characters, as well for struct ( it is 2 characters now for struct if it was 0 before) that is even better than it was before.

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

Your Softkey picture is already a good example. I was clicking on a small value like soft. Now soft is moving three times the size of soft to the right, there is not my mouse there are not my eyes. Very bad user experience.

Agree, we should not do this.
Confirm.

@franz-ms-muc
franz-ms-muc force-pushed the fix/1852-value-cell-editor-width branch from ec014aa to 4b359fa Compare September 4, 2026 20:51
@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

@azoitl now a improved Formula in Place.

@franz-ms-muc
franz-ms-muc requested a review from azoitl September 4, 2026 21:37

@azoitl azoitl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels better but still the PR has several issues.

I don't like that you need to need to access the font registry and that you create your dummy button. This should be handled more gracefully. And I think it should also be handled not via constructor paramters but in the CellEditorLocator.

When I type the editor is not growing.

For me the comments are to long.

And as usual your commit message is horrible.

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

When I type the editor is not growing.

this would be a VERY cool idea ! WOW ... never thought about it. so instead of scrolling extend the Field to maximum the max label width.

@franz-ms-muc
franz-ms-muc force-pushed the fix/1852-value-cell-editor-width branch from 4b359fa to 18acf1e Compare September 5, 2026 13:51
@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

Reworked per this review:

  • Removed the font-registry lookup and throwaway probe Button - sizing now measures the real control's own computeSize, which already includes a struct/array value's "..." dialog button automatically via the composite's GridLayout.
  • All sizing logic moved into ValueCellEditorLocator itself (nothing precomputed in ValueEditPart/passed via constructor anymore).
  • The box now grows (and shrinks) live while typing, via an ICellEditorListener the locator registers on the cell editor.
  • Trimmed the comments down.
  • Rewrote the commit message.

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

@azoitl i tested it and the growing input Field is really a total game-Changer in the whole 4diac-IDE experience. Thanks for that really really cool idea. It also stops growing at the set max-with, so this is really cool now.

pls review.

@franz-ms-muc
franz-ms-muc requested a review from azoitl September 5, 2026 14:30
private void ensureGrowsWithContent(final CellEditor celleditor) {
if (!listening) {
listening = true;
celleditor.addListener(new ICellEditorListener() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this listener realy needed? Because I thought the cell editor locator should be called when you type to update the position.

@azoitl

azoitl commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

No I remember how this can be done cleaner, use the base figure as width reference. Update the figures text when a user types through the direct edit policy and give it a min size. Then I think most of the changes you did are not necessary at all.

@franz-ms-muc
franz-ms-muc force-pushed the fix/1852-value-cell-editor-width branch from 18acf1e to a4430c8 Compare September 7, 2026 13:27
@franz-ms-muc
franz-ms-muc requested a review from azoitl September 7, 2026 13:27
@franz-ms-muc
franz-ms-muc marked this pull request as draft September 7, 2026 13:48
@franz-ms-muc

franz-ms-muc commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

No I remember how this can be done cleaner, use the base figure as width reference. Update the figures text when a user types through the direct edit policy and give it a min size. Then I think most of the changes you did are not necessary at all.

Sorry many times, yes your much simpler code works as well, but does not recoginze the Button anymore. Converted to draft until this is fixed. Sorry for too-early pushing.

now hand-Tested and verified.

@franz-ms-muc
franz-ms-muc force-pushed the fix/1852-value-cell-editor-width branch 3 times, most recently from 469cc43 to 6b7842e Compare September 7, 2026 14:21
@franz-ms-muc
franz-ms-muc marked this pull request as ready for review September 7, 2026 14:22
@azoitl

azoitl commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

I think this is now going down the wrong road. can you please remove the button checking code from the value editpart. I think the button checking needs to be done somewhere else. And without it it was allready a quite nice solution I must say.

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

I think this is now going down the wrong road. can you please remove the button checking code from the value editpart. I think the button checking needs to be done somewhere else. And without it it was allready a quite nice solution I must say.

this one a4430c8 was really nice yes.

i will investigate.

@franz-ms-muc
franz-ms-muc marked this pull request as draft September 8, 2026 09:47
Give the value figure a 5-character minimum preferred width so it
does not shrink to nothing while empty, and have the edit policy
call refreshPosition() on every keystroke so the figure (and with
it the underlying GEF cell editor) grows and shrinks to match the
typed text, clamped by the existing getMaxWidth() preference.

A struct or array value's editor adds a "..." dialog button next
to its text field, which needs more room than the figure alone
reserves. Rather than have ValueEditPart guess at this by checking
the variable's type, FigureCellEditorLocator now measures the
actual editor control's own fixed-size sibling widgets (via its
GridLayout) and widens just the live edit box to fit them, growing
left or right depending on which side the value is anchored to.

Assisted-by: Anthropic Claude Sonnet 5 (claude-sonnet-5)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@franz-ms-muc
franz-ms-muc force-pushed the fix/1852-value-cell-editor-width branch from 6b7842e to acd36df Compare September 8, 2026 12:17
@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

Hand-Verified with a Build on Windows, working as spoken.

@franz-ms-muc
franz-ms-muc marked this pull request as ready for review September 8, 2026 14:07
@azoitl

azoitl commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

This solution is quite interesting. However I see a few points I would like to change and improve or do a bit more differently . May I take this as basis and rework it into a new PR?

@franz-ms-muc

Copy link
Copy Markdown
Contributor Author

Yes, absolutely - please go ahead and take it as a basis and rework it as you see fit. Thanks for looking into it!

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