Skip to content
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

Properly reset editor.lineNumbers after disabling zen mode #90822

Merged
merged 1 commit into from Feb 18, 2020

Conversation

jeanp413
Copy link
Contributor

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
Contributor

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
Member

bpasero commented Feb 18, 2020

@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:

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
Contributor

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
Contributor Author

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

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line numbers invisible after Zen Mode toggled off
3 participants