Add support for switching debug sessions #152
Conversation
|
This looks like a good improvement. As eclipse-cdt-cloud/cdt-gdb-vscode#110 (comment) says, I don't see how it solves eclipse-cdt-cloud/cdt-gdb-vscode#110 - but that shouldn't hold this change up. Additional comments in that discussion |
|
|
||
| public constructor(protected extensionUri: vscode.Uri, protected memoryProvider: MemoryProvider, protected sessionTracker: SessionTracker) { | ||
| this.messenger = new Messenger(); | ||
| protected participantSessions = new Map<WebviewIdMessageParticipant, string>(); |
There was a problem hiding this comment.
This is the crux of the thing, I think. Basically, this implements the following idea:
- webviews have a 'context' state (here, just session).
- webview main stores that state for each webview.
- that state is used to supplement the other data from the webview when processing its actions.
At the moment that 'supplement' is dispatch: we pick the session's memory provider based on the session information. Pluggability will require that that 'context' be able to be elaborated and given special significance by different adapters. For that, at least the following will need to be done (some maybe here, otherwise as markers for later)
- The context will need to be more complex than a
string - The context will need to be passed to the memory provider that handles the request so that its fields can be handled appropriately.
I think these above could be handled in this PR, even if it's a bit redundant given the current functionality. I don't think I'd insist, though.
- We'll need some kind of framework for the shape of options, something like
interface Context { priority: number; options: string[]; }
interface ContextOptions {
Sessions: Context;
}and then when a webview changes its context, we ask the appropriate memory provider for more detailed options if it has any and send those to the webview.
There was a problem hiding this comment.
Would you be interested in doing either of the first two steps in this MR, or would you prefer to defer the whole effort?
There was a problem hiding this comment.
Prefer to refactor as a follow up if that's OK?
src/webview/memory-webview-view.tsx
Outdated
| protected async setOptions(options?: MemoryOptions): Promise<void> { | ||
| messenger.sendRequest(logMessageType, HOST_EXTENSION, `Setting options: ${JSON.stringify(options)}`); | ||
| this.setState({ configuredReadArguments: { ...this.state.configuredReadArguments, ...options } }); | ||
| return this.fetchMemory(options); | ||
| } | ||
|
|
||
| protected fetchMemory = async (partialOptions?: MemoryOptions): Promise<void> => { | ||
| // Ensure view has completed rendering | ||
| await new Promise(p => setTimeout(p, 0)); |
There was a problem hiding this comment.
What problems were you running into that this aims to fix?
There was a problem hiding this comment.
There's a race condition in the form validation engine being used which marks the address as invalid without properly awaiting the view to sort itself out. I sunk a bunch of time into trying to find it...
There was a problem hiding this comment.
I have this PR to address that. I haven't merged it because I wasn't sure about the implications of the approach I'd adopted in cases of webviews being popped out, but I can take a look.
There was a problem hiding this comment.
Missed that, great!
There was a problem hiding this comment.
I've merged that MR - see if you still see the validation error without this timeout.
There was a problem hiding this comment.
I'll remove the... hack
| this.setSession(participant, vscode.debug.activeDebugSession?.id); | ||
| this.setSessions(participant, this.sessionTracker.getSessions()); | ||
| await this.setMemoryDisplaySettings(participant, panel.title); | ||
| } |
There was a problem hiding this comment.
I'm getting a build error about the unused options parameter. Fixed with the leading underscore but maybe you wanted to use that parameter?
There was a problem hiding this comment.
Yeah, that was dropped in the fix from Colin but retained in the merge. Now fixed
thegecko
force-pushed
the
session-support
branch
from
1d796f6 to
910eab3
Compare
What it does
May close #67, eclipse-cdt-cloud/cdt-gdb-vscode#110
Add dropdown to memory view when multiple debug sessions are active which allows the user to switch between the sessions. This enables multiple windows to target different debug sessions at the same time.
How to test
Open a few windows when there are multiple debug sessions active
Review checklist
Reminder for reviewers
Re: #133 Which attempted the same but using the amalgamator
cc @WyoTwT @jonahgraham