Enable extenders to overwrite default settings

[planger]

What it does

Extenders can use the AdapterCapabilities to specify their own default settings, including the visible columns (see MemoryDisplaySettings interface). If an extension specifies such settings for a specific debug session (or type), those settings take priority over the user's VS Code settings of the Memory Inspector. To indicate that default settings are overwritten by an extension, an additional message can be provided by the extension shown at the bottom of the options overlay.

Users can prevent extenders from overwriting the default settings via a dedicated VS Code
setting (allowSettingsExtension).

As we now have two levels of defaults, this change also replaces the reset button with a ... button opening a drop-down menu similar to the view menus in VS Code. This menu contains two reset entries, if the debugger extension provided defaults; one for resetting to debugger defaults, the other one for resetting to VS Code defaults.

This also enables adding additional context menu entries via the usual VS Code menu contribution point, which can be used by extenders. In this change we already make use of it, to control the visibility of sections in the options overlay. We also hide columns and address format by default, because the columns can easily be enabled and disabled via context menu anyway, and address format is less-frequently changed by users.

Fixes #77

How to test

To properly test this, you need to set up another extension that registers an adapter.
I've published the one I've used for testing:
https://github.com/planger/customize-memory-inspector/commits/planger/issues/77/

Once you have contributed settings, you can test whether they are used correctly, and whether the reset buttons work correctly.

Aside from that you can check whether switching the visibility of option overlay sections work as expected.

Review checklist

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

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the code of conduct

[colin-grant-work]

I'm not sure I'm persuaded of the necessity of this feature.

• The configuration is already pretty complex, with defaults drawn from VSCode preferences and then overridable per view.
• This adds a third source of preferences, and one that users can't actually see, adding complexity and opacity.

Can you provide an example that would motivate different defaults for different kinds of debuggers? What about a particular debugger would make it desirable to configure its display parameters differently - apart from MAU, which I think needs to be something discovered per session rather than per debugger. #77 suggests some use cases, but at the moment, only padding and MAU are available in the existing settings, and I don't think they require such a heavy solution, or the integration of other settings into the solution - since I consider padding mainly a matter of taste, only the question of MAU really needs to be answered by the debugger.

If we do want to pursue this type of feature, I think I'd prefer a mechanism that's more transparent to users. For example, a convention that a plugin could contribute preferences in a style like this:

memoryInspector.[gdb].<same name as our preferences>

And then this plugin could do something like

const allPreferences = vscode.workspace.getConfiguration();
const basePreferences = extractDisplayPreferences(allPreferences.get(manifest.PACKAGE_NAME));
const maybeDebuggerDefaults = extractDisplayPreferences(allPreferences.get(`${manifest.PACKAGE_NAME}.[${session.type}]`));
const active = merge(basePreferences, maybeDebuggerDefaults);

Then the user would have full control over both the default defaults and the debugger-specific defaults, making it both more transparent and giving the user more granular control, beyond accept everything / nothing from debugger.

[jreineckearm]

@planger , thanks for opening this Draft! This is a good starting point to continue discussions in the open.

@colin-grant-work , thanks for your feedback and thoughts on this. Due to the complexity of this, we may need to have another brainstorming session in a call if you are open to it. And if we don't come to a good conclusion in this thread.

The motivation behind this change is to give debug adapters the chance to pre-configure the Memory Inspector windows as they see them most suitable for their users.

Example

Looking at what would be the recommended preference-set for our debug adapter in the context of our Arm Cortex-M tooling, I'd see the following that would diverge from the current defaults:

• Address Padding: 32-bit (or later driven by architecture information coming from the debug adapter). Current default: Unpadded. Yes, it is probably a matter of taste. But can be a valuable visual hint that a debugged CPU targets a 32-bit address space.
• Groups per Row: Autofit. Current default 4
• MAUs per Group: 4. Current default: 1
• Scrolling Behaviour: Auto-Append. Current default Paginate
• From Add auto-refresh capabilities for memory inspector windows #115 - Periodic Refresh: while running. Current default off

In future, we will also support Arm Cortex-A systems in our debug adapter extension. There are likely to be other demands on some of the above default settings. There are 32-bit and 64-bit flavors in the Arm A-profile. And other characteristics which for example may make the "Periodic Refresh" with "while running" less suitable (impact of MMUs in the system).

Other thoughts to consider are use case specific recommendations. And heterogeneous multi-core debug setups where we may want to debug 32-bit CPUs and 64-bit CPUs in parallel. With views adjusting at least their initial settings to the targeted CPU.

I start seeing the value of more obviously overriding Memory Inspector defaults from a debug adapter extension. Yet, overriding extension settings with other extension settings might become confusing as well. As users probably neither see the link without a good hint in the Memory Inspector itself. They can be in completely different sections of the extension settings GUI. But revisiting our approach again now, I also see that this has its problems.

The more I think about this and future demands, we are probably best off to open up default configurations through debug adapter launch configurations. As much as I dislike bloating those up, this may help to preserve a per-connection character of some of those defaults. Especially if driving further the thought of contexts in #133 . Transparency and how to present all of this to the user is still a big challenge. And may need assistance from the Memory Inspector. But I appreciate that it shouldn't burden the Memory Inspector as much as the debug adapter extension itself.

We'll give this another thought.

[colin-grant-work]

I do think it's a tricky situation. There may be cases where we'd like a debugger to be able to make recommendations, but I think it could be frustrating for a user to feel like they set things up the way they wanted them and then someone came along and wiped those configurations out. I think adding preferences for each debugger isn't really ideal - too many places to look, too much redundancy - but putting it in the executable code isn't very user-friendly, because then the user can't preview it at all.

Putting things in the launch config seems like a potential middle ground, since it allows selective overriding for each session, but it make sit harder for the debugger to impose its own defaults - or if it's easy for the debugger to do so, we end up back in the opaque case. Ideally, we'd be able to modify the global schema for debug configurations to include a section related to Memory Inspector behavior rather than forcing every debugger to add that to their own configuration schema, but that's an implementation detail.

On the other hand, if I'm worried about our users getting frustrated, I can solve that problem for myself by just not implementing this for our debugger; if you think this is the best way to give your users what you think they should have, whatever frustration might arise would be for you to handle, and I don't have to stand in the way 😉 .

[planger]

@jreineckearm As discussed offline, I've now switched to collapsible sections in the advanced options overlay and removed the menu entries for showing/hiding sections. Please let me know what you think! Thank you!

Screencast.from.05-22-2024.11.43.09.AM.webm

[jreineckearm]

May need to find a different name if the setting survives following discussions. Settings Extension has potential for confusion (because of the various meanings of extension).

[jreineckearm]

@planger , thanks for this! This is much more intuitive than the show/hide section toggles.
The one thing that I am still uncertain about is the removal of the Reset button. But the more I use this version, the more I get used to the three-dots-button. Still need to test the combination with the test adapter...
We may need to refine the styling and maybe the indentation of settings within sections. And to sort out how to feed in the default overrides: known launch configuration items vs extension API vs anything else.

[arekzaluski]

@planger Work very nicely and it id a huge improvement. It is also consistent with default vscode panels behaviour.
It renders correctly on all themes:

one minor request:
I'll increase the padding between chevron and title in settings.

How it looks now:

How it looks for vscode panels:

[planger]

Thank you very much for your feedback @jreineckearm and @arekzaluski!

I've adjusted the styling and message placement (ea868ec):

Also I've made a new proposal for a setting name to avoid the confusing allowSettingsExtension name. Now it is named allowDebuggerOverwriteSettings. Still a bit clunky but slightly better.

Please let me know what you think! Thank you!

[jreineckearm]

Thanks @planger , I took it for another spin

• In my case (maybe a resolution thing?), the contribution message and the first section are still quite close to each other. Maybe adding another 1 or 2 pixels of spacing could help:
  
• Should we add a little indentation before the expanded options. Similar has been done on "Periodic Refresh" (which would be extended by that). This could help to spot the section headers.
• The settings overrides are not immediately applied if you open a new empty window with the "Memory: Show Memory Inspector" command and then add an address. I believe I saw somewhere the removal of "initial settings" in source. Maybe related to that?

Regarding the earlier discussion around how to contribute things: I believe we should give it a try with what we have and learn from user feedback. The contribution in a way also allows to work with launch config options. Just that the debug adapter would need to analyze the config items and pass them through the API. That gives a debug adapter full flexibility, and a simple checkbox setting like "apply recommended defaults to Memory Inspector" could be enough for a start to enable/disable such contributions.

[planger]

Thank you for your feedback @jreineckearm! Strange, there seems to be some different rendering going on in Windows and Linux (see my screenshot on Linux above). I've adjusted the styling. Hopefully, it now looks good on all platforms. Also, I added some left padding, see below.

Can you please check how it looks on your end? Thank you!

The settings overrides are not immediately applied if you open a new empty window with the "Memory: Show Memory Inspector" command and then add an address. I believe I saw somewhere the removal of "initial settings" in source. Maybe related to that?

Great catch! I need to debug that in more detail, because theoretically it shouldn't be different to before from what I see. However, the initialization onReady may now be async as we need to fetch information from the adapter capabilities. This may cause this issue. I can reproduce it when opening the memory viewer via the command and not the variable view, but we need to debug this further and get back to you as soon as we've found the culprit.

Thank you!

PS: As concurrent changes introduced some conflicts, I pulled from main and resolved them.

[colin-grant-work]

Great catch! I need to debug that in more detail, because theoretically it shouldn't be different to before from what I see. However, the initialization onReady may now be async as we need to fetch information from the adapter capabilities.

I believe that @WyoTwT also ran into trouble with the order of events on startup when adding asynch code to the onReady handlers, perhaps he can provide some pointers.

[WyoTwT]

I ran into problems with theonReady handlers and a call to this.refresh() call in memory-webview-main.ts (and to fix it I had to add the code on lines 211-218 of my PR ).

I realize now that the rebase (to pull in changes to the main between my first version and the current version) may have made this change redundant. In the initial version, I was replacing a this.refresh() with a conditional this.refresh() but it appears the original this.refresh() was removed so I'd need to revisit the onReady problem I was seeing.

[planger]

@WyoTwT Thank you very much for your feedback! It indeed sounds like we observe the same issue. Also this PR and your PR (#133) have quite a number of overlapping changes. As I feel your PR is already rather close to being merged, I can try rebasing this PR on top of yours and see if that resolves the issue of using async.

Thank you!

[planger]

Fyi and note to my self, I've cleaned up and force-pushed the commit history to split the visual overlay panel improvements from the introduction of the adapter capability extensions.

[planger]

I ran into problems with theonReady handlers and a call to this.refresh() call in memory-webview-main.ts (and to fix it I had to add the code on lines 211-218 of my PR ).

I realize now that the rebase (to pull in changes to the main between my first version and the current version) may have made this change redundant. In the initial version, I was replacing a this.refresh() with a conditional this.refresh() but it appears the original this.refresh() was removed so I'd need to revisit the onReady problem I was seeing.

@WyoTwT As it turns out, the reason for my issue was that the groups to be rendered haven't been updated if there was no memory before and then memory was added. I extended the condition when to recompute the groups to be rendered. Now it works fine. Infact this was already a bug, because we didn't recompute if e.g. new columns have been added thus impacting the available space. This bug only appears with Autofit though, because otherwise the number of groups is static.

[arekzaluski]

LGTM

[jreineckearm]

@planger , thanks for the latest updates!
The last styling updates were IMO really useful.
Also, I can confirm that the initial update problem for a new, empty window are gone.

IMO in a good state to turn into a full PR.

[planger]

@colin-grant-work May I ask you for a review? I've split the changes into two commits, one for the options overlay improvements and one for the extensibility. If needed I can also split this up into two PRs, but one is built on top of the other as they both touch options-widget.tsx. Thank you very much!

[colin-grant-work]

I had a look over the code last week. I'm still not thrilled with the settings override mechanism, but I think it's integrated well architecturally. The changes to the advanced options widget are very nice. 👍

[colin-grant-work]

Maybe something like the 'suggested' default display settings, since they aren't necessarily enforced?