eclipse-cdt-cloud / vscode-memory-inspector

vscode memory inspector
https://open-vsx.org/extension/eclipse-cdt/memory-inspector
Eclipse Public License 2.0
6 stars 10 forks source link

Allow to write memories within the memory-table #108

Closed haydar-metin closed 7 months ago

haydar-metin commented 7 months ago

What it does

Introduce write memory capability within the memory table.

Top: Little Endian Bottom: Big Endian

Peek 2024-03-14 11-16

Closes #10

How to test

  1. Click on a data group within the table
  2. Provide a different value
  3. submit with enter or blur the element
  4. Changes should be applied

Review checklist

Reminder for reviewers

jreineckearm commented 7 months ago

@colin-grant-work , @planger , thanks for joining today's call where we discussed the memory edit strategy! A summary can be found here: https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/100#issuecomment-2008003753

We agreed to continue with the approach as implemented in this PR, i.e. to edit values on group level which considers endianess. But I see that @colin-grant-work has already resolved the discussion about this and indicated he is looking into a potential future approach to enhance this. Thanks for bearing with us on this one, @haydar-metin ! :-)

colin-grant-work commented 7 months ago

The styling of the input can push the content onto two lines:

image

Not a fatal flaw, and could be addressed in a follow-up.

colin-grant-work commented 7 months ago

I have posted a branch that allows both group-level editing (by double click) and sub- or super-group editing by context menu (though it is limited to a single row):

image

image

image

If that's of interest, feel free to use it. The when clause for the item could probably be tightened up.

haydar-metin commented 7 months ago

@colin-grant-work that looks great! I will take a look at it and try it out and depending on which way we will take, I will introduce the fixes you have requested.

colin-grant-work commented 7 months ago

@colin-grant-work that looks great! I will take a look at it and try it out and depending on which way we will take, I will introduce the fixes you have requested.

Based on @jreineckearm's feedback, I'd suggest deleting the pieces that relate to editing by selection - they're fairly discrete - and keeping the parts for editing at group level. Your current code also has an additional check to see whether the user actually changed anything and skips sending the write request if they didn't - that would need to be brought back.

colin-grant-work commented 7 months ago

@jreineckearm, I think I'm struggling a bit to understand the role of endianness in the display.

At the moment, for example, we reverse the content of a group if the display is set to little endian. As you know, I'm a little leery about treating the group as a significant unit, but I guess the logic runs something like this:

But then, if we take the approach here, I think we un-reverse while editing so that we're inputting things in the order they actually appear in memory (so typing f in the input becomes 0000 000f when we go back to the normal view). I'm not sure that that will make sense to users: we're nice enough to show them the more legible format, but then we ask them to enter things in the harder-to-read format, and then we take what they gave us and reverse it.

I think at a minimum, we should be consistent and let people enter the memory in the same form that they see it before they start editing it. More generally, I'm probably still not very happy with treating the group as a meaningful unit when we haven't established what meaning it should have and reversing some chunks of memory so that addresses occur in a strange order (3, 2, 1, 0, 7, 6, 5, 4, 11, 10, 9, 8, etc., for example) seems a little bit suspect to me. But I'm going to defer that unease to the bigger discussion about trying to represent the significance of the data rather than the form at some point down the road.

jreineckearm commented 7 months ago

The styling of the input can push the content onto two lines:

Agreed. Needs attention but can be done after a first release. Users will be able to figure out quickly that they can widen the column a little to make that artifact go away.

I think we un-reverse while editing so that we're inputting things in the order they actually appear in memory (so typing f in the input becomes 0000 000f when we go back to the normal view).

Phew, you made me nervous for a minute. But things are as I would expect them. :-)

Background for this behavior is the expectation of how to treat a group value while editing. Fetching and ordering the value adheres to the selected endianess. The interpretation is then however based on how you'd treat a human readable variable value, i.e. MSByte/Base is left, LSByte/Base is right. If you want F0000000 then you need to type is as such. If you just type F it is padded to the left with zeroes.

I appreciate that this falls again into the area of the "meaningful unit" discussion which we need to have soon. But testing this with real users could give us answers soon whether it's confusing. TBF, we are keen to get a new release out so that we can start promoting it as part of our tooling at a big trade show soon. I am sure we'll get plenty of feedback then. :-)

I'll set up another call soon to talk the endianess story through in more details if that's OK. But would advocate to solve the remaining code review issues now, and refine the use after a release.

haydar-metin commented 7 months ago

@colin-grant-work @jreineckearm The changes are now the same as #110 without the edit-by-select feature. I didn't touch the The styling of the input can push the content onto two lines issue for now. It happens only for autofit. I would need to check the CSS out again.

jreineckearm commented 7 months ago

@haydar-metin , I've created issue #112 to address the line break in a later PR. It shouldn't hinder us to merge this bigger change here.

jreineckearm commented 7 months ago

I've also created the following new issues to follow-up on open discussions triggered/associated with this change after a first release to gather user feedback:

haydar-metin commented 7 months ago

@jreineckearm I will return to you tomorrow by verifiying the examples done in the GIF. Maybe the correct handling was lost between the branches.