Skip to content
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

Allow memory display configuration per memory view instance #64

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

planger
Copy link
Contributor

@planger planger commented Feb 13, 2024

What it does

Implements the following behavior for memory display options:

  • A new memory view is opened with the display configuration as defined in the VS Code settings.
  • Changes made to the local options in a view affect only the specific view where the changes were made.
  • Altering global settings in the VS Code prefs doesn't impact any already open views.
  • Add button in the local options to reset to defaults, which applies the configuration as defined in the VS Code settings.

Implementation-wise this applies the following changes:

  • Defaults from VS Code setting are initially set from main.
  • From then on, memory display configuration lives in the view state only.
  • Remove previous messenger-based handling to change configuration.
  • Consolidate column activation and other display configurations into one type.
  • Add reset button to options which sends a notification to main so that it re-sets the options defined in the VS Code settings.

Fixes #60

How to test

  • Open VS Code settings for Memory inspector
  • Start a debug session
  • Open Memory Inspector view instance 1 and verify it applies the ones configured in the VS Code settings
  • Open Memory Inspector view instance 2 and verify it applies the ones configured in the VS Code settings
  • Change all memory display options in instance 1 (column activations, words by group, groups per row) and verify
    • They have the expected effect in instance 1
    • They have no effect in instance 2
    • They have no effect on the settings
  • Change memory display options in instance 2 and check again they don't have an effect outside of this view
  • Click button Reset to Defaults in the memory display options of instance 2 and verify
    • They reset exactly to the defaults from the VS Code settings
    • The instance 1 remains unchanged
  • Change the defaults in the VS Code settings
  • Click button Reset to Defaults in the memory display options of instance 2 again and verify they are applied correctly

Review checklist

Reminder for reviewers

* Defaults from VS Code setting are initially set from main
* From then on, memory display configuration lives in the view state
* Remove previous messenger-based handling to change configuration
* Consolidate column activation and other display configurations into one type
* Add reset button to options which re-reads defaults and sets it to view state

Fixes eclipse-cdt-cloud#60
src/plugin/memory-webview-main.ts Outdated Show resolved Hide resolved
src/webview/components/memory-widget.tsx Outdated Show resolved Hide resolved
src/webview/components/options-widget.tsx Outdated Show resolved Hide resolved
src/webview/components/options-widget.tsx Outdated Show resolved Hide resolved
src/webview/components/options-widget.tsx Outdated Show resolved Hide resolved
src/webview/memory-webview-view.tsx Outdated Show resolved Hide resolved
@planger
Copy link
Contributor Author

planger commented Feb 14, 2024

@colin-grant-work Thank you very much for your swift review!

You're right the column visibility / activation was somewhat overlapping. I wanted to centralize the view settings specified from the outside (main) including the column visibility so that initializing and resetting from the outside can be done with one event without having to handle column visibility and other settings separately. At the same time I didn't want to change the column contribution service's activation handling, expecting it to be (evolved into) something separate.

I've now consolidated the column visibility and activation. Both is effectively managed in the column contribution service owned by the App, which only passes on its column state to the child components (no separate visibility state) along with methods to change columns and other configurations.
To still have one view settings event from the outside, the App applies the settings passed from the outside (including the visibility state configured in the VS Code settings) to the column contribution service on change.

Thus, there are two interfaces:

  1. MemoryViewSettings: this includes all available settings and is passed to the App
  2. MemoryDisplayConfiguration: this is used as internal state being passed on to the child components

Consequently, MemoryViewSettings include both, the MemoryDisplayConfiguration and the ColumnVisibilityStatus. I did some renaming and added a bit of type doc to help making that clear.

Please let me know what you think! Thanks again!

@colin-grant-work
Copy link
Contributor

Change the defaults in the VS Code settings
Click button Reset to Defaults in the memory display options of instance 2 again and verify they are applied correctly

From the description I think the idea is that the changes to the VSCode setting shouldn't take effect until the webview asks to be reset to default, but at the moment, it appears that things aren't working quite as intended, and I believe it has to do with the lifecycle of VSCode webviews.

In the following sequence:

  • Open memory view.
  • Set the groups per row to 16.
  • Open VSCode settings in same column as memory view (i.e. hide the memory view with the settings).
  • Set groups per row preference to 8.
  • Switch back to memory view.
  • ---> memory view has switched to 8 groups per row.

I think that when revealed, the webview is behaving as though it's new.

@planger
Copy link
Contributor Author

planger commented Feb 16, 2024

@colin-grant-work Thanks again for testing! Unfortunately, I can't reproduce the issue though.

I'm surprised that this happens for you, since we are setting retainContextWhenHidden: true. Afaik, setting retainContextWhenHidden to true should keep the webview including its state alive even when the web view is in the background. I assume this is also why it works for any other state (column width, offset, etc.) in the webview without explicitly storing it.

memory-inspector-state.mp4

@colin-grant-work
Copy link
Contributor

Odd - I'm also no longer able to reproduce it. Not sure what was going on in the one session where it happened consistently.

@colin-grant-work colin-grant-work merged commit b94dc49 into eclipse-cdt-cloud:main Feb 19, 2024
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
Development

Successfully merging this pull request may close these issues.

Allow display configurations per Memory Inspector view instance
2 participants