-
Notifications
You must be signed in to change notification settings - Fork 36.9k
Fix the #143392 #143498
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
Fix the #143392 #143498
Conversation
|
@bpasero There have been some unforeseen circumstances, I resubmitted it. |
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 would suggest to look closer at an existing example of a context key that is updated within a group and also for the context menu and just monkey-see-monkey-doo.
| export const ActiveEditorPinnedContext = new RawContextKey<boolean>('activeEditorIsNotPreview', false, localize('activeEditorIsNotPreview', "Whether the active editor is not in preview mode")); |
The context key should not be specific for the context menu, because that is just a corner case.
I misunderstood the previous meaning,I'll make some adjustments, thanks! |
|
@bpasero I did some modifications and testing, hopefully, all possible scenarios are covered, and It looks like
and
can serve the same purpose, I chose handle with the first |
bpasero
left a comment
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.
Besides the 💄 feedback, I wonder if the context key fails to update properly when you move a tab, because in that case onDidActiveEditorChange will not fire.
You are right, we need to handle move events, active events don't handle this well, It was an oversight on my part, switch to deal here:
|
|
Thanks! |
This PR fixes #143392