Skip to content

Allow inline editing of values #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

martin-fleck-at
Copy link
Contributor

  • Propagate value updates back to the data provider and tracker
  • Simplify column declaration and provide dedicated 'edit' property
  • Provide edit renderer for text value changes

Refactor:

  • Convert some utility functions to React components
  • Convert thenable to promises

Closes #16

Add more edit controls for specific types

  • Enumeration Dropdown for fields with enumeration values
  • Boolean Checkbox edit for non-enums of width 1

Closes #22

Consider monospace font for values

Closes #45

@jreineckearm
Copy link
Contributor

Nice, @martin-fleck-at ! Finally got around to testing this. This looks really promising. :-)

I have started testing only now but would like to share some feedback before diving deeper into details. Still looking for an enum bit field. :-)

  • You have to use double-click to enter the string edit mode. I think that's fine to avoid unintentionally ending up in the mode for such displays. Similar to the variables view. However, I see collapse/expand toggle for nodes that are for example register values which can be unfolded to bit fields. Is it possible to avoid the collapse/expand if this escalates into a real double-click? I don't know if HTML/react has handlers to deal with it. Or if any kind of debouncing would help.
  • I like the dotted line for editable values. But if you haven't read the docs you may not necessarily notice that difference for read-only registers without it. I appreciate that this is a future enhancement for which we also want to collect real user feedback. But any idea to better distinguish r/w vs read-only values visually would be useful. Note that the edit-action pen is also hard to spot if you don't look out for it, now that the icon list for each entry got more crowded.
  • Is it possible to render controls other than the string edit even if not selected for edit? Meaning, if something would become a checkbox, can this always be rendered as a checkbox? This would allow a few clicks less to switch to edit mode for such values. But more importantly help visually to read bit field values. For the example of single bits it's for the user looking at a checked or unchecked checkbox vs reading a value 0b0 and 0b1. The latter requiring more "effort" to focus the eye on the last character in the string.

@jreineckearm
Copy link
Contributor

We also should have a quick chat when you are available next time about the enum dropdown usage. I can see the logic working, but have some ideas of how to further improve this (always show the dropdown, shown content vs dropdown entry content, spacing). We may also have to touch again the topic of resizing the two columns (name - value). I believe part of the improvements would require some more capabilities on that end.

@planger
Copy link

planger commented Feb 13, 2025

One other observation: when switching into edit mode, the text slightly jumps as we introduce the input field. It would be great to adjust the sizing so we can avoid the shift between the edit and non-edit modes.

- Propagate value updates back to the data provider and tracker
- Simplify column declaration and provide dedicated 'edit' property
-- Always render the expander on the first column
- Provide edit renderer for text value changes

Refactor:
- Convert some utility functions to React components
- Convert thenable to promises

Closes eclipse-cdt-cloud#16

Add more edit controls for specific types

- Enumeration Dropdown for fields with enumeration values
- Boolean Checkbox edit for non-enums of width 1

Closes eclipse-cdt-cloud#22

Consider monospace font for values

Closes eclipse-cdt-cloud#45
@martin-fleck-at martin-fleck-at force-pushed the issues/16-22-45_custom-controls_antd branch from f56ee0e to 76147a2 Compare March 13, 2025 01:33
- Align enum value label with enum value options
- Avoid expanding rows on double click used for editing mode
- Automatically render enums and booleans as editable fields
- Use dotted lines for all editable fields
- Use reduced opacity for non-editable fields
- Align text with editable fields
@martin-fleck-at
Copy link
Contributor Author

@jreineckearm I pushed an update that should hopefully address most of your concerns:

  • Align enum value label with enum value options
  • Avoid expanding rows on double click used for editing mode
  • Automatically render enums and booleans as editable fields
  • Use dotted lines for all editable fields
  • Use reduced opacity for non-editable fields
  • Align text with editable fields

Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Looks already better. :-)

  • Double-clicking the value works better now without immediately expanding/collapsing the node. If you are a slow double-clicker, things can get a bit fiddly again. But appreciate that the chosen workaround can't take a much longer delay which would become problematic as well. Might be worth in future to only toggle when clicking on the arrow (but not now).
  • The jumpy behavior when entering the edit field is gone.
  • I like always seeing the checkboxes/enums.
  • Enums look better with the new for formatting
  • The export feature still writes only numerical values. Which is good.

Some findings while testing:

  • Checkbox behavior doesn't look right.
    • It doesn't seem to reflect the values before any interaction. The top three boxes should be checked.
      image
    • Clicking one of them for the first time, I briefly see it checked. But then cleared. Which makes kind of sense because my click effectively cleared the bit to 0. But the cleared bit isn't reflected in the full register value.
    • Clicking the checkbox then again two times, the check mark seems to get in sync. And is correctly reflected in the value.
    • It looks like only one checkbox at a time keeps the check mark. After it looks like things are starting to be sync, clicking the next checkbox clears the first again.
  • For enums, display string seems to be one state "behind" after changing values. That is for both changing the full register value and using the dropdown.
  • It looks like drop downs for write-only registers don't show the selectable values. In the below screenshot, the two (Write Only) entries marked red are drop-down entries of the above.
  • For edit fields I see something in between:
    • If you have a register with bitfields shown as edit fields, and if you change the full register value, then the display string is changing, too. But when you then entire the edit mode, the edit field holds the previous value. If you change again the full register value, you can again observe the one-state-behind effect.
  • The "edit" icons next to dropdowns and checkboxes don't make sense anymore. And don't take any effect. We should remove the icon now that editing is more easily accessible by clicking values. I think that would be also fine for string values unless you have strong concerns users won't understand the new usage pattern.

I am afraid we at minimum need to get the "lagging" checkbox/dropdown states sorted.

- Fix issue of checkbox not initializing correctly
- Improve issue of not syncing correctly with outside updates
- Do not show update command for auto-editable checkbox and enum
- Fix linting error
- Replace 'return' with 'await'
@martin-fleck-at martin-fleck-at force-pushed the issues/16-22-45_custom-controls_antd branch from 76147a2 to 0952d20 Compare March 20, 2025 14:40
@martin-fleck-at
Copy link
Contributor Author

Thank you very much @jreineckearm for your review! I tried to address most of it as good as I could but I do face some issues that prevent me from properly testing the write-update so I hope we are getting closer to having this work as expected.

  • Checkbox behavior doesn't look right.

There was indeed an issue that we didn't use the edit value but the label value and therefore ended up comparing a binary formatted value with a regular formatted value. I hope that is fixed now.

  • For enums, display string seems to be one state "behind" after changing values. That is for both changing the full register value and using the dropdown.

Unfortunately, I cannot really write in my setup so it is a bit hard for me to test. But there was an issue with properly syncing values from the outside. I hope that fixes this. Please let me know if it does not and maybe I can find a way to mock writing or something.

  • It looks like drop downs for write-only registers don't show the selectable values. In the below screenshot, the two (Write Only) entries marked red are drop-down entries of the above.

I think we are missing the screenshot here? Could you elaborate on that a bit. I know that we do have that in our formatValue method that we only show (Write Only) if access type is write-only, instead of the usual label. That was already the case before but it seems odd to me.

  • If you have a register with bitfields shown as edit fields, and if you change the full register value, then the display string is changing, too. But when you then entire the edit mode, the edit field holds the previous value. If you change again the full register value, you can again observe the one-state-behind effect.

Going back into edit mode should now properly reflect the last value we received.

  • The "edit" icons next to dropdowns and checkboxes don't make sense anymore. And don't take any effect. We should remove the icon now that editing is more easily accessible by clicking values. I think that would be also fine for string values unless you have strong concerns users won't understand the new usage pattern.

I removed the edit icons from enums and boolean types.

@jreineckearm
Copy link
Contributor

Thanks, @martin-fleck-at , for the updates! Will re-test the fixed items tomorrow morning.

Unfortunately, I cannot really write in my setup so it is a bit hard for me to test. But there was an issue with properly syncing values from the outside. I hope that fixes this. Please let me know if it does not and maybe I can find a way to mock writing or something.

Will re-test if it got fixed.

I think we are missing the screenshot here?

My bad. Will provide one when I got the HW set up again.
My expectation was that the displayed value remains (Write Only) no matter what value is written (we cannot read it back). But I was hoping that I can select from the available enum values to write a specific value to such bit field/register. Right now, I see multiple entries in the drop down all saying (Write Only) instead of enum values like Value 0, Value 1, etc.

@jreineckearm
Copy link
Contributor

Thanks again, @martin-fleck-at !

I re-tested a couple of things.

  • Checkbox behavior doesn't look right.

There was indeed an issue that we didn't use the edit value but the label value and therefore ended up comparing a binary formatted value with a regular formatted value. I hope that is fixed now.

Appears to be fixed. Thanks! See some other behavior though around checkboxes that need review. More details further below.

  • For enums, display string seems to be one state "behind" after changing values. That is for both changing the full register value and using the dropdown.

Unfortunately, I cannot really write in my setup so it is a bit hard for me to test. But there was an issue with properly syncing values from the outside. I hope that fixes this. Please let me know if it does not and maybe I can find a way to mock writing or something.

Thanks! That got fixed with your latest changes.

I think we are missing the screenshot here?

Here the screenshot (not for the HW setup you have, will see to find similar for that, see further below).
image

Going back into edit mode should now properly reflect the last value we received.

Fixed, thanks!

I removed the edit icons from enums and boolean types.

Removed, thanks!

While testing this version, I spotted a couple more things.
I appreciate some of this would be easier handled with a reproducer for your HW setup. Working on it...
Note that I am happy to get things addressed separately if they behaved the same before these changes/the move to Ant.
Also, I am happy to move harder to fix problems with small user impact to separate issues for later.

  • Change a checkbox value -> write access fails -> checkmark is kept on the box while the raw value reads back as 0 for that bit. Note that reverting to the previous state works better for the enum dropdowns. Should be looked at, brings raw value and checkbox display out of sync.

  • Open a node with many children -> view brings last child in view. This feels a bit jumpy. Will try to find a reproducer for your HW if you can't find one. Note that this only happens if all children are dropdowns/checkboxes. The problem doesn't show if at least one string edit/display field is involved. Can live with a separate fix if hard to fix.

  • (Write Only) string edit fields: should start with a clear edit field, currently holds (Write Only)

  • Initial refresh sometimes seems to miss after connect. And it doesn't get triggered when expanding a node. Starts working properly after next step/start-stop. Will try to find a reproducer for your HW, I so far spotted this only for one peripheral of one device.

  • Write-only register with checkboxes -> odd behavior of checkmarks display. Randomly (?) sets other checkboxes after a clicked one cleared. Didn't understand the pattern yet. Maybe an artifact of failing reads specific for this debugger backend. Will test others. Could be a candidate for later.

  • Sometimes see rendering artifacts when scrolling down (jumps back). I see it when a write-only register with checkbox children is expanded. Will try to find a reproducer for your HW, might not be highest priority if hard to understand the problem.

Will keep you posted if I find that something is a problem with the debug adapter.
Will provide a reproducer for your HW offline.

- Only use '(Write Only)' on drop down label but keep option labels
- Avoid jumping on expand for auto-edit rendered fields (focus issue)
- Use empty string when editing a Write-Only field
- Remove state handling from cell and simply render the column value
Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

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

@martin-fleck-at , thanks a lot for working on the reported issues!

All seem to be fixed now except from the "Write-only register with checkboxes -> odd behavior of checkmarks display". TBF: that description of mine was quite vague at the time. And I cannot see such behavior anymore. I assume this got fixed with all the improvements you made since my initial feedback. It was very likely related to the incorrect checkbox behavior and the "lagging states" we had initially.

I am good to go with the current state of this. Thanks a lot for the hard work on this improvement! This is bringing a lot of value to our users. :-)

@jreineckearm jreineckearm merged commit 4517c58 into eclipse-cdt-cloud:main Mar 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants