-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[vscode] Implement stubbed API for activeStackFrame API #13900
Conversation
It seems the 3 Macos tests can't start. That should not block the review however. |
e3fa0f5
to
ade6c8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be one of the weaker API definitions from the VS Code team. What does "focused" even mean? Some small comments.
@@ -262,6 +269,37 @@ export class DebugExtImpl implements DebugExt { | |||
}); | |||
} | |||
|
|||
set activeStackItem(stackItem: theia.DebugStackFrame | theia.DebugThread | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check whether the active item actually changed before sending notifications.
@@ -254,8 +259,12 @@ export class DebugSession implements CompositeTreeElement { | |||
return this._currentThread; | |||
} | |||
set currentThread(thread: DebugThread | undefined) { | |||
if (this._currentThread === thread) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I comparing instances the right thing or should we look at the id?
protected _currentFrame: DebugStackFrame | undefined; | ||
get currentFrame(): DebugStackFrame | undefined { | ||
return this._currentFrame; | ||
} | ||
set currentFrame(frame: DebugStackFrame | undefined) { | ||
if (this._currentFrame === frame) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, is there only ever one copy of the same frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code should update the existing one instead of creating new ones (see
const frame = this._frames.get(id) || new DebugStackFrame(this, this.session); |
I pushed a new commit to address the issue.
Yes, a surprising API. In this case, 'focus' means what is current selection in the UI from what I can see from VS Code 🤷 |
fixes eclipse-theia#13846 contributed on behalf of STMicroelectronics
c633d7d
to
d7e3d98
Compare
What it does
Implement previously stubbed API for activeStackFrame API (DebugStackFrame & DebugThread)
Fixes #13846
Contributed on behalf of STMicroelectronics
How to test
activestackitem.mp4
We don't have exactly the same interface as in VS Code, so we get more easily the empty stack item message. That should not impact users, as selecting a thread or a stack frame still throws the active stack item change event.
Follow-ups
none
Review checklist
Reminder for reviewers