Skip to content

Properly reset editor.lineNumbers after disabling zen mode#90822

Merged
isidorn merged 1 commit intomicrosoft:masterfrom
jeanp413:zen-mode-linenumbers-1
Feb 18, 2020
Merged

Properly reset editor.lineNumbers after disabling zen mode#90822
isidorn merged 1 commit intomicrosoft:masterfrom
jeanp413:zen-mode-linenumbers-1

Conversation

@jeanp413
Copy link
Copy Markdown
Contributor

@jeanp413 jeanp413 commented Feb 17, 2020

This PR fixes #90240

Uses a Set to remember all the editors we have hidden line numbers for. Not sure if it's safe to call updateOptions on disposed editor widgets (this could happen if we have multiple editor groups and we close any of them)

cc @isidorn

@isidorn
Copy link
Copy Markdown
Collaborator

isidorn commented Feb 18, 2020

Seems to work great. Merging in. Thanks a lot for this PR.
As for updating options on disposed editor widgets that also seems to work fine. @bpasero I could not detect a way to see if the editor is disposed, when do you exactly dispose them?

@isidorn isidorn merged commit 70b5e93 into microsoft:master Feb 18, 2020
@isidorn isidorn added this to the February 2020 milestone Feb 18, 2020
@jeanp413 jeanp413 deleted the zen-mode-linenumbers-1 branch February 18, 2020 15:00
@bpasero
Copy link
Copy Markdown
Member

bpasero commented Feb 18, 2020 •
edited
Loading

@isidorn @jeanp413 not a big fan of how this ends up adding a new field to the layout.ts class which is only used by zen mode. Why can this not be added to the transitionDisposables set that is called each time zen mode changes? There should be a disposable for each editor that gets the line number updated, no?

As for IEditor, let me copy some JSDoc that maybe explains better how to listen for disposal:

vscode/src/vs/editor/common/editorCommon.ts

Lines 248 to 258 in 9a81eb3

export interface IEditor {
/**
* An event emitted when the editor has been disposed.
* @event
*/
onDidDispose(listener: () => void): IDisposable;
/**
* Dispose the editor.
*/
dispose(): void;

Upon disposal, simply remove the entry from the transition disposables.

@isidorn
Copy link
Copy Markdown
Collaborator

isidorn commented Feb 18, 2020

Yeah, adding to the tranisitionDisposables makes better sense.
And we could listen to the dispose event
@jeanp413 sorry for preemptively mering this. Just let me know if you will provide an additional pr that tackles Ben suggestions, if not I can also look into this. Thanks

@jeanp413
Copy link
Copy Markdown
Contributor Author

jeanp413 commented Feb 18, 2020

@isidorn Sure I'll create another PR to tackle bpasero's feedback

@jeanp413 jeanp413 mentioned this pull request Feb 18, 2020
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@isidorn isidorn

Labels

None yet

Projects

None yet

Milestone

February 2020

Development

Successfully merging this pull request may close these issues.

Line numbers invisible after Zen Mode toggled off

3 participants

@jeanp413 @isidorn @bpasero