Allow periodic updates (running and stopped CPU)

[martin-fleck-at]

• Add configuration for periodic refresh mode and interval
• Add commands to modify periodic refresh mode and interval
• Add context menu entries to modify periodic refresh mode and interval

Additional:

• Extract configuration provider for settings, session and ws-scoped

Fixes #36

---

Please note that this PR is based on #46 and should therefore only be merged after the other PR was merged. If #46 proves to be difficult, we may rebase this commit on the main branch.

[martin-fleck-at]

As discussed directly with @jreineckearm, I pushed a commit to simplify some options:

• Context Menu: Mode before Interval
• Settings: Mode before Interval, Group Periodic Refresh
• Remove periodic refresh and mode from session and only use workspace
• Make Set Periodic Refresh also available as More actions
• Remove 'While Stopped' configuration option but keep logic

[jreineckearm]

Thanks, I think this is much more intuitive now! Also slightly reorganizing the extension settings is very helpful.

Some more items to look into

• When periodic refresh is active, something seems to be wrong with rendering expand/collapse states. Example:
  • Periodic refresh is always on.
  • Expand a node
  • The node expands, collapses, and expands again. Ending in the requested state. But the temporary and intermediate inversion is a little irritating.
• I like the restructured context menu. :-)
  • But it feels a bit odd to have an "ignore" option at the top. Should we reorder "Set Value Format", "Periodic Refresh", "Ignore..."?
  • I assume we can't have similar structuring (Periodic Refresh group) in the "..." menu?
• The periodic refresh sometimes makes the context menu disappear while trying to navigate through it. Same for the quickpick to change the refresh period if you made it that far. This happens when you have a changing value in the visible part of the table.
• I do understand that this presumably comes from internal states to deal with on/off? But should we really allow users to have another way of turning the refreshs of next to the "mode" settings?
  
• Still wondering if it would make sense to have the available refresh modes as child entries here rather than the quick-pick? But could be a future enhancement after the general introduction of the feature.
  

[martin-fleck-at]

Hi Jens, thank you very much for your review! I pushed a commit that fixes the "cosmetic" parts that you mentioned. As for the behavior I might need more information on how to actually reproduce this.

• When periodic refresh is active, something seems to be wrong with rendering expand/collapse states. Example:

  • Periodic refresh is always on.
  • Expand a node
  • The node expands, collapses, and expands again. Ending in the requested state. But the temporary and intermediate inversion is a little irritating.

I cannot reproduce this on my side. Do there have to be some actual value updates for this to appear?

• But it feels a bit odd to have an "ignore" option at the top. Should we reorder "Set Value Format", "Periodic Refresh", "Ignore..."?
• I assume we can't have similar structuring (Periodic Refresh group) in the "..." menu?

I re-ordered the menu items as you suggested.

• The periodic refresh sometimes makes the context menu disappear while trying to navigate through it. Same for the quickpick to change the refresh period if you made it that far. This happens when you have a changing value in the visible part of the table.

Can you give me an example of a place where some values change over time so I can see if I can reproduce this?

• I do understand that this presumably comes from internal states to deal with on/off? But should we really allow users to have another way of turning the refreshs of next to the "mode" settings?

Agreed, allowing -1 is a bit odd. I updated the label and the validation to avoid the user setting -1. Technically, any number below or equal to 0 will still cause periodic updates to stop.

• Still wondering if it would make sense to have the available refresh modes as child entries here rather than the quick-pick? But could be a future enhancement after the general introduction of the feature.

I agree that would be lovely but unfortunately, VS Code does not support checked commands (or menu items), see microsoft/vscode#109306. There even has been a proposal but nothing has been merged. I also had a quick look into a workaround, where we simply add the checkmark to the label. That would work but it still looks a bit odd in my opinion. If you want I can implement that but apparently all extensions do run into the same problem with the VS Code API.

[jreineckearm]

Thanks for the updates, @martin-fleck-at !

I will try to create a reproducer for your HW setup and share separately tomorrow. Here already some comments.

I cannot reproduce this on my side. Do there have to be some actual value updates for this to appear?

Yes, it appears to happen when a changing value is visible. If I scroll somewhere without any re-rendering happening, then the context menu becomes more stable.

Can you give me an example of a place where some values change over time so I can see if I can reproduce this?

Yes, will try to set something up for you. Appreciate it's a bit tedious to do these fixes w/o a reproducer at hand.

Agreed, allowing -1 is a bit odd. I updated the label and the validation to avoid the user setting -1. Technically, any number below or equal to 0 will still cause periodic updates to stop.

Thanks! Let me test tomorrow how this looks now.

I agree that would be lovely but unfortunately, VS Code does not support checked commands (or menu items)

Apologies, I remember now that we had this topic already on the Memory Inspector last year. Please ignore.

[jreineckearm]

Thanks again, @martin-fleck-at , for the recent changes!

Some more test results

I cannot reproduce this on my side. Do there have to be some actual value updates for this to appear?

Saw it now also for a register group without a visibly changing value. Enable always update (did it through context menu + quick pick), CPU in debug halt state, then try SystemControl for your HW, no previously expanded children. Will try to find other examples and send them offline.

I re-ordered the menu items as you suggested.

Awesome, looks more intuitive now (at least to me :-) ).
Sorry, another comment: to avoid duplication, we could pull the (Workspace) suffix to the top-level Periodic Refresh entry. And remove the suffix from the children together with the mention of Periodic Refresh in the child entries? Something like this:

Set Value Format
Periodic Refresh (Workspace) ----- Set Mode
                               '
                               '-- Set Interval
Ignore Peripheral (Workspace)

Can you give me an example of a place where some values change over time so I can see if I can reproduce this?

Will try to find an example and send it offline. I saw this behavior once with the above mentioned expanded SystemControl while always updating + CPU in debug halt state (quick pick disappeared for no good reason).

Agreed, allowing -1 is a bit odd. I updated the label and the validation to avoid the user setting -1. Technically, any number below or equal to 0 will still cause periodic updates to stop.

Great, thanks! Maybe combine the two sets of parenthesis in the shown text, i.e. something like (Workspace, press enter ...)?
Maybe also helpful to mention the default of 500ms in the text if users want to go back to the initial settings while not knowing them necessarily by heart? Could be either between Workspace and press enter... or at the end.

I'll send you a reproducer for a peripheral with changing values for your HW setup offline.

[martin-fleck-at]

Saw it now also for a register group without a visibly changing value. Enable always update (did it through context menu + quick pick), CPU in debug halt state, then try SystemControl for your HW, no previously expanded children. Will try to find other examples and send them offline.

Still having troubles to reproduce, unfortunately.

Awesome, looks more intuitive now (at least to me :-) ). Sorry, another comment: to avoid duplication, we could pull the (Workspace) suffix to the top-level Periodic Refresh entry. And remove the suffix from the children together with the mention of Periodic Refresh in the child entries? Something like this:

I moved the '(Workspace)' suffix as you suggested.

Will try to find an example and send it offline. I saw this behavior once with the above mentioned expanded SystemControl while always updating + CPU in debug halt state (quick pick disappeared for no good reason).

For me the quickpick stays as long as I don't click on the tree somewhere. But this might have been fixed with an update in #46 where we now no longer steal the focus.

Great, thanks! Maybe combine the two sets of parenthesis in the shown text, i.e. something like (Workspace, press enter ...)? Maybe also helpful to mention the default of 500ms in the text if users want to go back to the initial settings while not knowing them necessarily by heart? Could be either between Workspace and press enter... or at the end.

Unfortunately, I cannot combine the text as the (Press Enter...) text comes from VS Code itself. Maybe rephrasing would remove the double parenthesis, not sure what you prefer.