Introduce Auto-Append scrolling behavior and rename Infinite behavior to Grow

[haydar-metin]

What it does

Introduces the Auto-Append scrolling behavior and renames the Infinite scroll behavior to Grow.

It also separates the user arguments from the actual memory read arguments:

The growing memory content buffer would be cleared by a subsequent click on the Go button. #56

When the user provided arguments are the same then we have the following view:

If the user navigates within the table:

--

How to test

1. Switch to auto-append scrolling behavior in the settings
2. Scroll to the bottom

• New data will be appended to the bottom and you can scroll again
• The options will be updated with a new label (see description above)

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Closes #56

[planger]

Thank you very much @haydar-metin! Seems to work well. A few minor comments inline below.

[planger]

@haydar-metin I think @colin-grant-work fixed the bug which is fixed in this PR also in #78. So probably it'd be best to rebase this PR on top of #78. Thanks!

[haydar-metin]

The code triggered an error if the memory could not be fetched for specific areas, so I changed:

• If the memory is empty/erroneous for a request, then we reset the memory and show that we have no results (We should implement in a different PR error alerts/messages for users)
• Show more information in the console:
  • 

[haydar-metin]

Based on the discussions, We now have the following options:

1. Suggestion

• Remove Infinite scroll behavior
• Add the Auto-Append scroll. That means, more memory will be appended after reaching the end of the list automatically.
• Remove the Load x more bytes below button in Auto-Append mode

2. Suggestion

• Remove Infinite scroll behavior
• Add the Auto-Append scroll. That means, more memory will be appended after reaching the end of the list automatically.
• Show the Load x more bytes below button in Auto-Append mode

3. Suggestion (Current implementation)

• Add the Auto-Append scroll. That means, more memory will be appended after reaching the end of the list automatically.
• Remove the Load x more bytes below button in Auto-Append mode
• TODO: Update the descriptions

I suggest to go with either 1) or 2) as the current implementation for Infinite scroll is not really what one would expect.

[colin-grant-work]

I suggest to go with either 1) or 2)

I agree. I don't have strong feelings about whether to show the buttons when append-by-scroll is enabled. Not showing them saves a little space; showing them gives people a little more granular control.

I'd also suggest allowing append-by-scroll at the top of the container as well as the bottom - the user could decide they want to see things just above the current frame as well as just below.

[jreineckearm]

I suggest to go with either 1) or 2)

I agree. I don't have strong feelings about whether to show the buttons when append-by-scroll is enabled. Not showing them saves a little space; showing them gives people a little more granular control.

I believe we should start with option 1). And add back the button if users ask for it. If not there, it gives the user a visual indication which mode is active.

I'd also suggest allowing append-by-scroll at the top of the container as well as the bottom - the user could decide they want to see things just above the current frame as well as just below.

I would not add this to this mode. There is a subtle difference between scrolling downwards and upwards:
The user made a conscious decision where to start filling the contents of the memory window. Scrolling downwards doesn't automatically change that choice. But if it would automatically prepend, that decision may be altered when trying to quickly get back to the starting point via mouse-wheel/scrollbar. For example after having scrolled down a little, getting enlightened by something they spot further down, and then quickly attempting to double-check something at the start of the window. Automatic pre-pending has potential to become tedious during usage.
By offering the button to prepend, the user needs to make another conscious decision to change the IMO most important parameter in the window setting.
If the user additionally wants to gate the downwards scrolling then they can use the other already existing mode.

[haydar-metin]

The latest commit now replaced Infinite with Auto-Append:

• No auto-append on scrolling to the top
• No Load x more bytes below button

[haydar-metin]

@colin-grant-work Thank you for the code feedback! The code looks now cleaner in my opinion!

[haydar-metin]

@colin-grant-work Thank you for your guidance and sharing your knowledge with me! Your comments were helpful. I also tested the frozen case.

[jreineckearm]

Thanks so much @haydar-metin for this great work, and @colin-grant-work for the effort in reviewing and providing guidance!
I tested this feature today in more depth. And it overall is working great!

I found a few scenarios which might need review. But overall, I believe we are getting there. :-)

• If you change the global setting from "Paginated" to "Auto-Append", then you need to close existing instances and open them again for the change to take effect. I think this is OK for now. But since we don't have this option available in the local window settings, it felt a bit inconsistent with others.

• Something behaves a little unexpected if you change the "Groups per Row" set for example from 1 or 2 to 16 or 32. If I then trigger a reload (Go button or hitting enter in one of the argument fields), then it loads more than Length Bytes (in my case 256). Presumably a side effect of filling the entire window. Probably fine. But we could instead load only what's requested and on the first scroll attempt load more?

• If you scroll a while and then change for example the "Groups per Row" setting, then the window seems to keep it's number of rows. And hence starts to load more data to fill them all. I wonder if it makes more sense to recalculate and reduce the number of rows instead. Apologies if I misinterpreted what's going on, but something is loading more data in this case. And it doesn't seem to be related to the above mentioned behavior.

• If I click "128 bytes above", then it adjust the actual offset. But I don't see the beginning of the table to move to the corresponding address. Tested with current main status which shows the same behavior. So not introduced with this PR, hence should be handled separately.

  • Update: I tested this now also with the 1.0.1 release of the Memory Inspector. I got a feeling the "Paginated" mode never really worked (both directions). And for the "Infinite" mode the buttons only worked downwards. I think now even more that this needs attention in a separate issue.

• Navigating to the end with the 'End' key also triggers an append. It probably shouldn't. But I believe we need to review the entire keyboard navigation in the view a little more in future. I for example see a cell highlight after clicking into the table and using the arrow keys. This entire topic should be addressed separately to not overload this PR.

Given the size this PR has already, I am wondering if we are happy to merge it as it is and continue from here to refine via new issues?

[haydar-metin]

@jreineckearm Thank you for the feedback and extensive testing! I would appreciate it to do the changes in a follow up issue, with more specifications / requirement details.

If you change the global setting from "Paginated" to "Auto-Append", then you need to close existing instances and open them again for the change to take effect. I think this is OK for now. But since we don't have this option available in the local window settings, it felt a bit inconsistent with others.

You can reset it by using the functionality introduced by @planger

Something behaves a little unexpected if you change the "Groups per Row" set for example from 1 or 2 to 16 or 32. If I then trigger a reload (Go button or hitting enter in one of the argument fields), then it loads more than Length Bytes (in my case 256). Presumably a side effect of filling the entire window. Probably fine. But we could instead load only what's requested and on the first scroll attempt load more?
If you scroll a while and then change for example the "Groups per Row" setting, then the window seems to keep it's number of rows. And hence starts to load more data to fill them all. I wonder if it makes more sense to recalculate and reduce the number of rows instead. Apologies if I misinterpreted what's going on, but something is loading more data in this case. And it doesn't seem to be related to the above mentioned behavior.

Yes, both scenarios depend on the visible rows. If any change to bytes per word, words per group or groups per row would result in a table without a scrollbar, then it fetches additional bytes. The same approach also applies to reloading. Moreover, we load some additional rows to the required visible rows to be sure that everything works correctly.

And if you scroll and then change any setting, then it depends on your current position. If changing the settings would result in either a) non-scrollbar b) or your current scrollbar position not to be visible, then it will also cause a refetch. However, if the change does not cause the previously mentioned issues, then it will not trigger a refetch.

Summarized a refetch is always triggered when the scrollbar can not be rendered because of the current settings.

If I click "128 bytes above", then it adjust the actual offset. But I don't see the beginning of the table to move to the corresponding address. Tested with current main status which shows the same behavior. So not introduced with this PR, hence should be handled separately.
Update: I tested this now also with the 1.0.1 release of the Memory Inspector. I got a feeling the "Paginated" mode never really worked (both directions). And for the "Infinite" mode the buttons only worked downwards. I think now even more that this needs attention in a separate issue.

I think that is a Arm-Debug configuration specific issue. If I test it with the Embedded-Debug launch configuration for the Hello World Example then it works, however, with the Arm-Debug Launch Configuration I can also recreate those issues.

Navigating to the end with the 'End' key also triggers an append. It probably shouldn't. But I believe we need to review the entire keyboard navigation in the view a little more in future. I for example see a cell highlight after clicking into the table and using the arrow keys. This entire topic should be addressed separately to not overload this PR.

Yes, Primereact supports keyboard navigation. We can customize it depending on the requirements. However, it would be better to do that in a separate issue and PR.

[colin-grant-work]

It's now impossible to grow the view using the "Load more bytes below" bar. At a minimum, we need to update its text to indicate that it's now moving, not loading more memory. But I believe that that is still a valid use case: appending, but not automatically.

Dismissing old review, since I assumed it would be superseded by new review.

[haydar-metin]

@colin-grant-work

It's now impossible to grow the view using the "Load more bytes below" bar. At a minimum, we need to update its text to indicate that it's now moving, not loading more memory.

The original implementation of Pagination only moved the offset for both directions. This behavior and labels didn't change here. The growing part only applied to Infinite scroll, which is now removed. For this reason, there are also no implementation-specific details concerning growing the list below, as it would not be used anywhere.

I will now use the following criteria to show the different texts: 09b0b60

    const isGrowing = props.direction === 'above' && props.shouldPrepend === true;
    const growLabel = <span>Load {selector} more bytes {props.direction}</span>;
    const moveLabel = <span>Move {selector} bytes {props.direction}</span>;

Hint: props.shouldPrepend === true is easier to read in this case IMO.

But I believe that that is still a valid use case: appending, but not automatically.

Yes, as discussed, we can introduce this feature if the necessity arises. Code wise, it should be something really easy to do.

[thegecko]

There's a lot of discussion on this PR and I'm keen we are pragmatic around moving forward. If there are no major blockers, can we merge this and see how we get on with the setup in the real world? We can always revisit this later.

[jreineckearm]

Fine with me. The recently found issues can be handled by new PRs.
@haydar-metin , re Arm Debugger: thanks for sharing your results with embedded-debug. We indeed were able to confirm this is a defect in the debugger backend for arm-debug.

[colin-grant-work]

The original implementation of Pagination only moved the offset for both directions.

I don't believe this is correct. The original implementation always increased the number of bytes loaded - i.e. it was equivalent to Infinite scroll. Later, pagination was introduced without growth; now the original behavior has been removed entirely. Our users are accustomed to the original behavior; it's also the behavior found in the Theia extension, so it has some history; and I strongly prefer it to either alternative, so I would prefer that it not be lost here.

[colin-grant-work]

@thegecko, I'm sorry to be a blocker, but I really don't want to merge this having lost the original behavior of the More(!)MemorySelect headers and footers.

[haydar-metin]

@colin-grant-work Oh sorry! With original I meant the version here in regard to how Pagination worked before doing any changes:

https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/blame/main/src/webview/components/memory-table.tsx#L57

I didn't expect the scroll behaviors to have such a history. So, Infinite was the real original implementation. Then, we shouldn't remove it.

So, I'm not quite sure what to do, should I just bring Infinite mode back again? Add the button and combine it with Auto-Append? @colin-grant-work @jreineckearm @thegecko

[colin-grant-work]

I think for now, so we can move things forward, it would be best to bring back Infinite (maybe as Grow) as an option. Then we cover three of the four permutations of Paginate vs. Grow x Auto-Append / No Auto-Append, and the code doesn't have to change much.

[haydar-metin]

Ok and final question, should I also revert the labels (Move x bytes vs Load x more bytes) to be the same as before I started this PR? Or should Pagination have the new labels?

And should I rename Infinite to Grow?

Thanks!

[jreineckearm]

The original implementation always increased the number of bytes loaded - i.e. it was equivalent to Infinite scroll. Later, pagination was introduced without growth

Oh, I see. Similar to @haydar-metin , I was mislead by the focus of discussions on Paginated and removing Infinite and thought that Paginated was the behavior you were keen on preserving.

Yes, let's bring that mode back then. We definitely need to use a different name though. I believe the "what's meant with Infinite" caused a lot of headaches during this discussion. So, users will probably even have a harder time to understand it. Grow would be fine, or Append.

Ok and final question, should I also revert the labels (Move x bytes vs Load x more bytes) to be the same as before I started this PR?

@haydar-metin , I'd be good either way. But since we talk about preserving something that users are accustomed to, we probably should switch back to Load.

[haydar-metin]

The latest commit now renames Infinite to Grow and the behaviors and labels are now the same as before this PR.

• Pagination
  • Triggering Above Button: offset - bytes
  • Triggering Below Button: offset + bytes
• Grow
  • Triggering Above Button: offset - bytes, count + bytes
  • Triggering Below Button: count + bytes
• Auto-Append
  • Triggering Above: offset - bytes, count + bytes
  • Automatically appends on reaching the bottom of the list

[jreineckearm]

Thanks, LGTM! Awesome work!

[colin-grant-work]

Thanks for the changes, and thanks for bearing with all of my comments!