Add auto-refresh capabilities for memory inspector windows #115
Conversation
|
We already have the preference
So that we could have, for now |
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 Curiously, the |
martin-fleck-at
force-pushed
the
issues/91
branch
from
70acba8
to
f58701c
Compare
|
(force-pushed to solve merge conflict) |
|
@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. |
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).
I think we have a good model for this in the |
Ah yes, I somehow misinterpreted your previous statement, having all as a per-view basis that absolutely makes sense. Thank you for the clarification! |
|
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 |
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. |
|
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.
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. On |
martin-fleck-at
force-pushed
the
issues/91
branch
from
f58701c
to
9322f7a
Compare
|
@jreineckearm @colin-grant-work I pushed an update to the current branch and updated the description. We now have the following options:
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 #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 [1] Note I also found a very small UX issue in the settings sub-group rendering which may also break settings #119. |
- 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 eclipse-cdt-cloud#91
martin-fleck-at
force-pushed
the
issues/91
branch
from
9322f7a
to
dc631e7
Compare
|
I'll take a look at this tomorrow. |
| "editor/title": [ | ||
| { | ||
| "command": "memory-inspector.show", | ||
| "group": "navigation", | ||
| "when": "memory-inspector.canRead && (resourceLangId === c || resourceLangId === cpp)" | ||
| } |
There was a problem hiding this comment.
Is this related to the functionality elsewhere in the PR? It looks like the aim is to expose the Memory Inspector functionality more prominently, but the resourceLangId is only a so-so proxy for 'is a source file of a source file for a debug session that can read memory'.
There was a problem hiding this comment.
You are correct, it is not strictly part of this PR and so I only mentioned it as a "minor" in the PR description
Add title toolbar item for C/C++ file to access open memory inspector
Mainly because I used it a lot to open a memory inspector from a source file. I understand that the condition is far from complete in this regard but since we are only opening an empty memory inspector, I don't see the harm. However, I understand if you want me to move this out of this PR as wel.
There was a problem hiding this comment.
I suppose that if I would put it anywhere, I would put it (if possible) on the toolbar of the debug view rather than the file, since the 'memory' and the 'file' don't have a clear correspondence. For now, perhaps separate from this PR so that it can be discussed separately.
package.json
Outdated
| "Refresh memory views when when debugger stops (e.g. a breakpoint is hit)", | ||
| "Memory view data is manually refreshed by user" | ||
| "Refresh when the debugger stops (e.g. a breakpoint is hit)", | ||
| "Refresh automatically when the view is newly focussed.", |
There was a problem hiding this comment.
| "Refresh automatically when the view is newly focussed.", | |
| "Refresh automatically each time the view receives focus.", |
There was a problem hiding this comment.
Much better!
|
|
||
| activate(context: vscode.ExtensionContext): void { | ||
| context.subscriptions.push( | ||
| vscode.debug.registerDebugAdapterTrackerFactory('*', this), |
There was a problem hiding this comment.
I understand the conceptual separation of jobs between this class and the memory provider, and I like the idea of separating the tracking part from the memory provider part, but I'm not thrilled about both of them registering tracker factories on *. It's a tricky problem: I was looking at this plugin earlier today, and on the one hand, I understood the interest in centralization, but on the other hand, you're likely to still to have the same number of handlers doing the same amount of work, with the only difference being which ones VSCode calls directly and which ones it calls indirectly.
I think for now, I'm basically happy with the division of labor you've implemented, but if you see a way to avoid doubling the session trackers, I might like it even better.
There was a problem hiding this comment.
Hmm... that is a very good point and I too was a bit frustrated with the lack of data that VS Code gives about the session, so I definitely see how that plugin came to be. While it centralizes the code nicely (which is a big plus imho), I don't think it will really improve performance since the amount of work will be mostly the same.
I'm not sure how we can reduce the number of session trackers without simply introducing a level of indirection which will lead to a very similar result than the plugin you mentioned. However, I'm also not sure how much impact that will have since currently we have two "global" trackers, one for session information and one for memory. If that number grows and the connections between the trackers become somehow difficult to manage, we should definitely re-think this. But if you agree, I'd postpone that for now and maybe track that in a separate issue if we encounter any problems.
src/plugin/memory-webview-main.ts
Outdated
| WebviewSelection, | ||
| WriteMemoryArguments, | ||
| WriteMemoryResult, | ||
| writeMemoryType, | ||
| } from '../common/messaging'; | ||
| import { getVisibleColumns, WebviewContext } from '../common/webview-context'; | ||
| import { AddressPaddingOptions, MemoryViewSettings, ScrollingBehavior } from '../webview/utils/view-types'; | ||
| import type { MemoryViewSettings, ScrollingBehavior } from '../webview/utils/view-types'; |
There was a problem hiding this comment.
The annotation here is correct, but the fact that it's correct suggests that these types should be moved to a common area so that we don't have to guard against accidentally importing webview code inappropriately.
There was a problem hiding this comment.
I 100% agree that it is a good idea to actually move the necessary interfaces into a common area, will do, thanks!
| // we are only interested in the events of the active session | ||
| if (!event.session?.active) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this true? Suppose that a view wanted to be updated on stopped events, and a background session stopped - would we want to ignore that? (though VSCode might automatically foreground the session...)
There was a problem hiding this comment.
I believe it is true until we properly implement #97. For now I just went with backwards compatibility and we always just show data from the current session. I believe as soon as we properly tie a view to a particular session this condition no longer holds true. In fact, I hope with the additional data that we send to the session as part of this PR, #97 will not gonna be too much work.
| debugCapabilities?: DebugProtocol.Capabilities; | ||
| clientCapabilities?: DebugProtocol.InitializeRequestArguments; | ||
| active?: boolean; | ||
| stopped?: boolean; |
There was a problem hiding this comment.
Very good question! If the session would hold it's state itself (like it does in Theia) we wouldn't have the need to externally track that information and we currently use it whenever we want to update the memory views about changes in the session.
You are arguing that it should be a completely separate event that is sent to the memory views without remembering that we were stopped, right? What is the advantage of that versus remembering the state and sending it from there? I see that in your issue you mentioned that sometimes the event might not (yet) be fired so it could happen that we send "out-dated" information. However, since we always send everything we need to know about the session, that would be corrected as soon as the event is actually fired, or am I missing something?
I'm just a bit hesitant about that because the delay might be true for some sessions but it is not true for all session types and all other Debug Views also rely on the event-based mechanism as far as I can see. I'm not too strongly opposed either, I am just not sure I see the harm for now.
| } else if (isDebugEvent('memory', message)) { | ||
| this.fireSessionEvent(session, 'memory-written', message.body); |
There was a problem hiding this comment.
This illustrates some of the difficulty with the division of labor. This isn't really a matter of status, and the rest of the bookkeeping for memory retrieval and writing is handled in the memory provider. I think it would be preferable to keep this in the memory provider, but I don't feel very strongly about it.
There was a problem hiding this comment.
I see your point. However, I'm not sure we should see it strictly as 'state management' vs 'memory management' but rather 'enriched session management' by the tracker (e.g., more events could be added in the future). Similar to the npm package you shared above, I'd almost argue that the session tracker is just one layer in-between and the reason the memory-provider also needs to register as a debug tracker is because we want to allow external contributions.
| if (this.state.autoRefresh === 'After Delay' && this.state.autoRefreshDelay && this.state.autoRefreshDelay > 0) { | ||
| // we do not use an interval here as we only want to schedule another refresh AFTER the previous execution AND the delay has passed | ||
| // and not strictly every n milliseconds. Even if 'fetchMemory' fails here, we schedule another auto-refresh. | ||
| const scheduleRefresh = () => this.fetchMemory().finally(() => this.updateAutoRefresh()); |
There was a problem hiding this comment.
Currently in fetchMemory, we are not awaiting doFetchMemory so it will return immediately and we won't have waited for the memory to be fetched.
There was a problem hiding this comment.
Good catch! I now return the doFetchMemory so we are actually working with the correct promise!
src/webview/memory-webview-view.tsx
Outdated
| // may happen when we initialize empty | ||
| return; | ||
| } | ||
| this.doFetchMemory(completeOptions); |
There was a problem hiding this comment.
| this.doFetchMemory(completeOptions); | |
| await this.doFetchMemory(completeOptions); |
There was a problem hiding this comment.
I chose to return the doFetchMemory instead but if you prefer await, I'm also fine with that.
- Improve wording in setting descriptions - Move shared types into common area instead of using type imports - Fix 'fetchMemory' not returning the correct promise
martin-fleck-at
force-pushed
the
issues/91
branch
from
cf8befb
to
96d8750
Compare
|
@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. |
|
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 I think One original thought about the 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 |
|
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:
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. |
|
I'm not sure exactly which state you're referring to when you say
Do you just mean that you would like to have two configurations (or three, more likely):
And not any form of If so, it sounds like you have strong feelings about it, and I can live with that arrangement. |
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:
-- Convert debug events into session events with additional data
-- Align 'Min' with 'Minimal' value from manifest
Minor:
Fixes #91
How to test
(Note: Options have been replaced by enum and look a little bit different now)
Review checklist
Reminder for reviewers