Skip to content

Conversation

@Kalmaegi
Copy link
Contributor

This PR fixes #143392

@Kalmaegi
Copy link
Contributor Author

@bpasero There have been some unforeseen circumstances, I resubmitted it.

@bpasero bpasero added this to the On Deck milestone Feb 20, 2022
@bpasero bpasero marked this pull request as draft February 20, 2022 16:40
@bpasero bpasero modified the milestones: On Deck, February 2022 Feb 20, 2022
Copy link
Member

@bpasero bpasero left a 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.

@Kalmaegi
Copy link
Contributor Author

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 bpasero modified the milestones: February 2022, March 2022 Feb 21, 2022
@Kalmaegi
Copy link
Contributor Author

@bpasero I did some modifications and testing, hopefully, all possible scenarios are covered, and It looks like

const observeActiveEditor = () => {

and
this._register(this.onDidModelChange(e => {

can serve the same purpose, I chose handle with the first

@Kalmaegi Kalmaegi marked this pull request as ready for review February 24, 2022 08:12
@bpasero bpasero self-requested a review February 25, 2022 11:19
Copy link
Member

@bpasero bpasero left a 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.

@Kalmaegi
Copy link
Contributor Author

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:

this._register(this.onDidModelChange(e => {

@bpasero bpasero merged commit 02d6d9b into microsoft:main Feb 26, 2022
@bpasero
Copy link
Member

bpasero commented Feb 26, 2022

Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable "Close to the Right" action on last tab context menu

2 participants