Add auto-refresh capabilities for memory inspector windows

[martin-fleck-at]

Extend Auto Refresh capabilities with 'After Delay' and 'On Focus'

• Rework 'refreshOnStop' boolean setting to 'autoRefresh' enumerable -- On Stop (previously: 'on' for 'refreshOnStop') -- On Focus (previously: always implicit on view state change) -- After Delay (new) -- Off (previously: 'off' for 'refreshOnStop')

• On Stop -- Rework global setting to be local for each Memory Inspector -- Listen to debug session stopped event and propagate to view

• On Focus -- Rework implicit refresh update to option in setting -- Listen to view state changes and propagate to view

• After Delay -- New option to explicitly define a delay when re-fetching the memory -- Minimum: 500ms, default: 500, input step size: 250ms

Refactoring:

• Split debug session tracking into dedicated class with session events -- Convert debug events into session events with additional data
• Split context tracking from memory provider into dedicated class
• Move manifest to common for default values and avoid duplication -- Align 'Min' with 'Minimal' value from manifest

Minor:

• Add title toolbar item for C/C++ file to access open memory inspector
• Improve debugging experience by using inline source maps
• Align creation of option enums to use const objects
• Additionally guard 'body' on debug responses for safety
• Avoid functional React state update where unnecessary

Fixes #91

How to test

• Enable auto-refresh in the display options
• If your DAP provides logging you can watch the request/response pairs in the output

(Note: Options have been replaced by enum and look a little bit different now)

Review checklist

• [x] 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

[colin-grant-work]

We already have the preference "memory-inspector.refreshOnStop", with which this new preference clashes a bit. I'd suggest that we generalize to something like the autosave preference with various triggers:

So that we could have, for now off onStop afterDelay + the delay preference.

[martin-fleck-at]

We already have the preference "memory-inspector.refreshOnStop", with which this new preference clashes a bit. [...] So that we could have, for now off onStop afterDelay + the delay preference.

You definitely raise a very interesting point! My reasoning for a dedicated boolean flag (auto refresh enabled) was that I wanted to give the user the option to disable the auto-refresh without the need to change (and lose) the value to something "invalid" (like -1). So in my mind auto refresh and refresh on stop were definitely not mutually exclusive. I don't know if there really is a use case for onStop + afterDelay but maybe @jreineckearm has some experience with this.

Curiously, the refresh on stop option is one of the few "global" setting that we have, i.e., it cannot be changed on a per-view basis. I wonder how that should play out when we combine it with the auto-refresh which I believe should definitely remain on a per-view basis since multiple open views might otherwise flood the DAP with requests. What would you expect to happen if you switch globally from afterDelay to onStop? Should we automatically disable the checkbox and input field for the auto refresh UI? Should it still be enabled (even the checkbox) but simply not have an effect? I feel like we need to carefully think about that as otherwise this might get confusing.

[martin-fleck-at]

(force-pushed to solve merge conflict)

[jreineckearm]

@colin-grant-work , @martin-fleck-at , a couple of very good points. Let's not rush into merging this one before the release and spend a little more though on it. I'll get back to this next week.

[colin-grant-work]

Curiously, the refresh on stop option is one of the few "global" setting that we have, i.e., it cannot be changed on a per-view basis.

I suggest that we allow it - or the new preference that combines it with auto-refresh - to be configured on a per-view basis, as you've done in your PR. Then there can't be a conflict: the options would be encapsulated in a single piece of configuration, and that configuration would configurable in the usual places (globally and per view).

Should we automatically disable the checkbox and input field for the auto refresh UI? Should it still be enabled (even the checkbox) but simply not have an effect?

I think we have a good model for this in the files.autoSave preference: the files.autoSaveDelay takes effect only if files.autoSave is set to afterDelay and is ignored otherwise. I think with a minimum of documentation, it will make sense to users that memory-inspector.autoRefresh.refreshRate only takes effect if memory-inspector.autoRefresh.behavior (or whatever name we choose) is set to afterDelay.

[martin-fleck-at]

Curiously, the refresh on stop option is one of the few "global" setting that we have, i.e., it cannot be changed on a per-view basis.

I suggest that we allow it - or the new preference that combines it with auto-refresh - to be configured on a per-view basis, as you've done in your PR. Then there can't be a conflict: the options would be encapsulated in a single piece of configuration, and that configuration would configurable in the usual places (globally and per view).

Ah yes, I somehow misinterpreted your previous statement, having all as a per-view basis that absolutely makes sense. Thank you for the clarification!

[martin-fleck-at]

Just throwing that one out there as well: Currently we always refresh the memory inspector when the view state (active/focussed or visible) changes and the view is visible. That can result in quite a few refreshes as during debugging the code editor usually gets focus so clicking between the code editor and the memory inspector also triggers a refresh every time. So while we are at it: Should we also introduce a On Focus Change option?

[colin-grant-work]

So while we are at it: Should we also introduce a On Focus Change option?

Or maybe a not on focus change :-). But yes, I think if that's currently happening as part of the default behavior (and if it's something that we can prevent from happening), then it would make sense to include it among the options.

[jreineckearm]

I agree that we should try to combine the different triggers into a single user visible setting. I think however that a simple drop-down may not be sufficient. It would rather need to be a drop-down with checkbox items (or similar) to selectively allow adding/removing triggers.

Currently we always refresh the memory inspector when the view state (active/focussed or visible) changes and the view is visible.

Is this like in always fetching data from the debug adapter? I am not entirely sure that switching tabs should do a full re-read from the target system. Appreciate though that this may be a result of the WebView not preserving the previous state when hidden for example due to a tab-switch. And that also a debug adapter needs to do its part in caching values where sensible. I believe however that we shouldn't burden the user with this decision. We may need to investigate how we can get cleverer here. Such cleverness (like only loading visible data) may also help to improve performance.

On onStop vs afterDelay: it really depends on how long the refresh period is for afterDelay. A large refresh period may require an additional onStop update. On the other hand, we also should consider if afterDelay should always be active. Or if it should only refresh while a CPU is running. Traditionally, I am more used to only update while CPU is running. But there were asks in the past to also refresh if the CPU is in stopped state but peripherals and their memory-mapped register interfaces still receiving and processing stimuli from the outside world.

[martin-fleck-at]

@jreineckearm @colin-grant-work I pushed an update to the current branch and updated the description. We now have the following options:

• On Stop (previously: 'on' for 'refreshOnStop')
• On Focus (previously: always implicit on view state change when the view was visible, now on focus)
• After Delay (new: every n milliseconds we refresh the view)
• Off (previously: 'off' for 'refreshOnStop')

All options are now view-local, i.e., scoped per view. This made it necessary for new events to be sent to the webview (e.g., stopped on a debug session and view state change of a webview). I think in the long run, it is definitely a good idea to have that information within the webview so we can be more fine-grained and smarter with memory updates. The refactoring of the session tracking could also serve nicely as a basis for https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/97.

As discussed before those options are currently mutually exclusive (enum) but there might be a use case where you actually want to combine them (i.e., they become toggles) - as mentioned by @jreineckearm. Unfortunately, I lack the real world application for this use case, so I'd highly value your input on how we should best proceed on this. I think having agreement on that before merging the PR is key as we are already breaking the previous refreshOnStop setting by replacing it and for sure we want to avoid too many breaking updates between releases [1].

[1] Note I also found a very small UX issue in the settings sub-group rendering which may also break settings https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/119.

[colin-grant-work]

I'll take a look at this tomorrow.

[martin-fleck-at]

@colin-grant-work Thank you very much for your feedback! It took me a while to get back to this but I pushed an update now and replied to your comments - some of which may still need some broader discussion but I'm not sure they all need to happen as part of this task.

[jreineckearm]

My apologies for only coming back to this now!

I kind of like the idea of condensing the possible triggers to reduce complexity. But I am a little concerned about losing the ability to mix triggers because of flattening them into an enum. Especially if we need to add more in future. I still have no good idea how to realize this instead. My initial reaction would have been to have a kind of dropdown where you can toggle check marks to indicate which triggers are active. But I couldn't find that pattern yet in VS Code configurations. Often, such things seem to be realized by a bunch of checkboxes.... which we worst case have to accept for now. Let me do some more digging if I find something in the design guidelines...

Regarding onFocus: I am curious if we really should expose this as an option. It feels to me like this is a way to work around the fact that a hidden Memory Inspector instance may not be updated when a different trigger occurred. We probably should rather address this or delay such trigger until the window becomes visible next time. Instead of always updating. This can become quite expensive if the debug adapter lacks caching. Especially if "cycling" through views with Ctrl+Tab.

I think afterDelay isn't really the right name for what's happening. The option was supposed to periodically update the window. What's called delay is in fact the delay between periodic updates.

One original thought about the periodic updates was that they are only active while the CPU is running. Given above discussion, this would become the next option to take into account. Assuming we merge this PR first to get periodic updates in. And address the always-periodic-update vs periodic-update-while-running separately. Which almost could be a dropdown with three options by itself (off being the third)....

Will have a chat with Martin about this again tomorrow. But it feels like we need to keep the options separate and revisit if we can handle the onFocus thing better without the need to make it configurable. Let's not forget that we also have the freeze feature to suppress updates if we don't want them.

[jreineckearm]

I had another look at applying the auto-save settings scheme to the window refresh. Unfortunately, there are some differences because of which I don't think they apply one-to-one to the memory window updates:

• The triggers for auto-save are rather focused on a document/editor (view). They are all tied to a user interaction/modification within that view. If a user stops interaction with the view, one of these trigger options can fire.
• One could say that the auto-save triggers "build up" on each other. First you have the afterDelay which fires after inactivity of the user for a while. Then you have the onFocusChange which is more deliberate user action to trigger the update. The next level is to require the onWindowChange which is an even strong user action then just changing focus.
• The triggers (or better events) for window updates and target reads are less controllable by the user. They can happen whenever something happens in the target system. This sometimes is out of the user's control.
• However, the user may want or even need to configure sensitivity to each of these events.
  • For example the onStop event is something I expect a user to disable very rarely. That's normally the point in time all debug views are updated for the majority of target systems. There may only be very rare cases where you need to suppress this.
  • The onFocus update should normally be inactive, if exposed at all. Window contents should normally be updated in the background. Reason is that, even if cached, refreshing and re-rendering a large window buffer on each focus change is quite expensive. However, most of the times there is no reason for that. Even worse: if a target architecture doesn't like memory reads while the CPU is running, or if memory reads while running return inconsistent data (for example if an MMU is in play), this may do more harm than good. But as I understand, these refreshes do happen at the moment on a focus change? I think this needs change.
  • Looking at periodic updates, you may want to limit that to a certain CPU state (CPU running or not running) which would give the support for this event already three states to consider. See above about supporting memory reads while CPU runs.

I believe we should keep the trigger/event configuration on the level it was before rather than trying to condense them into a flat list.

Should the settings be only global or also local? I believe also local. Even if I am worried about the advanced window settings getting more and more these days. But this could be handled by a redesign of the in-window settings that distinguishes between common settings and expert settings.

[colin-grant-work]

I'm not sure exactly which state you're referring to when you say

I believe we should keep the trigger/event configuration on the level it was before rather than trying to condense them into a flat list.

Do you just mean that you would like to have two configurations (or three, more likely):

• onStop: boolean
• afterDelay: boolean
• delay: number

And not any form of onViewStateChanged?

If so, it sounds like you have strong feelings about it, and I can live with that arrangement.

[martin-fleck-at]

@colin-grant-work @jreineckearm I rebased the PR and added another commit which separates the options again. We now have Refresh On Stop with on/off as before and Periodic Refresh with always/while running/off. As suggested, I removed the explicit option for On Focus again. I think now everything should be as expected. If you have some time to have another look, I'd really appreciate it.

[jreineckearm]

@colin-grant-work , @martin-fleck-at : and apologies again for taking a while to come back to this.

Do you just mean that you would like to have two configurations (or three, more likely):

onStop: boolean
afterDelay: boolean
delay: number

Correct (afterDelay now periodicRefresh): My main concern is the mutual exclusiveness which the flattened enum would impose.

And not any form of onViewStateChanged?

Correct, we should carefully review if we can improve the window to deal with this without introducing another option. For example by tracking a dirty state based on refresh settings and doing an update when the window comes back to the foreground next time, which clears the dirty state. Or by updating windows in the background (pro: capture and keep the state as configured by refresh setting, con: potential target reads that are not needed if the window doesn't come back to the foreground until the next refresh trigger). I believe this requires more careful thought and discussion about the best strategy. Also, we should aim for a separate PR for this to encapsulate the involved complexity change.

If so, it sounds like you have strong feelings about it, and I can live with that arrangement.

Well...kind of. I'd love to get these enhancements in to test them with users. :-) If we see that the approach doesn't work with users, then we can revisit to flatten the options.

[planger]

@colin-grant-work Thank you very much for your feedback! Just as a heads-up, we'll look into it early next week, as many of us are out of office this week. Thanks!

[martin-fleck-at]

@colin-grant-work @jreineckearm I merged the master into this and resolved some conflicts and then added the PR feedback on top of that. It would be great if someone could have another look to see if everything still works properly. Thank you!

[jreineckearm]

Still works as expected after today's updates.