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

Update status bar when opening a binary file as text #59914

Merged
merged 2 commits into from Oct 8, 2018

Conversation

Projects
None yet
3 participants
@atitoa93
Contributor

atitoa93 commented Oct 3, 2018

Fixes #59832

WIP: Making sure no extra work happen if the active editor remains the same. More information in the issue comments.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 3, 2018

@atitoa93 yeah we still need that other listener unfortunately otherwise the editor status does not update when you click between 2 opened editors in 2 separate groups.

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 3, 2018

Ops, Nice catch 👍

@atitoa93 atitoa93 force-pushed the atitoa93:issue/59832/update-statusbar-binary-files branch 2 times, most recently from 3bbe23b to 62b5a14 Oct 3, 2018

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 3, 2018

Yeah, this works, though now we update 2 times when opening an editor. I was hoping for a solution where we maybe have a new event that is very specific to this case (opening a binary file) that the editor status could listen to (similar to the encoding change). I realize this is a bit more work, so I can also try to come up with a bit more advice as needed.

@bpasero bpasero added this to the Backlog milestone Oct 3, 2018

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 3, 2018

@bpasero Here is the approach I came up with.

  • Adding a new field in FileEditorInput class called forceOpenAsTextFromBinary for example, maintaining this field with the current callback setForceOpenAsText.
  • Register a new event called onDidVisibleEditorOpenAsTextFromBinary for example, and fire this event only if the input has forceOpenAsTextFromBinary=true.

What do you think?

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 4, 2018

@atitoa93 yes something like that, it should be another thing we could add to activeEditorListeners in updateStatusBar and should probably be declared on IFileEditorInput so that the dependency layers are OK.

@bpasero bpasero modified the milestones: Backlog, On Deck Oct 4, 2018

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 4, 2018

@bpasero While developing the approach I mentioned above, came to my mind a more elegant and generic solution. All that I have to do is considering opening as text from binary an active editor change (and actually it's almost like changing to a different editor).
The generality in this solution that it will work with any (current and future) dependency, not just the status bar.

@atitoa93 atitoa93 force-pushed the atitoa93:issue/59832/update-statusbar-binary-files branch from 62b5a14 to f005614 Oct 4, 2018

@msftclas

This comment has been minimized.

msftclas commented Oct 4, 2018

CLA assistant check
All CLA requirements met.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 5, 2018

@atitoa93 I thought about that too, but that solution you implemented makes it very specific to file editor inputs and we would have to add more custom code for other inputs that behave similar. That is why I would prefer either my initial idea or a more generic approach that works for all editors.

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 5, 2018

@bpasero I see what you mean. What about applying the same technique but with the EditorInput class instead of FileEditorInput, naming the new attribute something like newInputInPlace for example.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 5, 2018

@atitoa93 I find it weird to have a property/method for this "state", when is this state being reset? An event would make much more sense imho.

@atitoa93 atitoa93 force-pushed the atitoa93:issue/59832/update-statusbar-binary-files branch from f005614 to a9fa622 Oct 6, 2018

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 6, 2018

@bpasero I did the change to be an event instead of an attribute, but there is something weird happening, It renders only the selection change. I spent a lot of time trying to figure out why but I couldn't. Any help will be appreciated.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 6, 2018

@atitoa93 instead of listening on the level of the editor service, why not just listen at the place where it is needed (around here):

this.activeEditorListeners.push(activeCodeEditor.onDidChangeConfiguration((event: IConfigurationChangedEvent) => {

There is already an array of active editor listeners that will get cleaned up when the editor changes. To get to the editor, just call const activeEditor = this.editorService.activeEditor

@atitoa93 atitoa93 force-pushed the atitoa93:issue/59832/update-statusbar-binary-files branch from a9fa622 to 0815b91 Oct 6, 2018

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 6, 2018

@bpasero I tried that before but it rendered the selection change only. And I thought I have to call updateStatusBar from the render function to work, but actually, the same issue happened.
I updated the PR with comments to describe the solutions I tried. both renders the selection change only. I tried to reset the state (i.e this.state = new State()) inside the event and it didn't work too.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 7, 2018

@atitoa93 so you say this does not work:

this.activeEditorListeners.push(activeEditor.onDidOpenInPlace(() => {
	this.updateStatusBar();
}));

Can you debug what happens in the updateState method if you do this?

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 7, 2018

@bpasero Quick update: it works fine if I run it on debugger mode and putting a breakpoint in updateState method. But, when I run it on development mode, it only renders the selection change. I'll continue debugging and investigating more the delayedRender thing.

@atitoa93 atitoa93 force-pushed the atitoa93:issue/59832/update-statusbar-binary-files branch from 0815b91 to 74b5e8d Oct 8, 2018

@atitoa93

This comment has been minimized.

Contributor

atitoa93 commented Oct 8, 2018

@bpasero So after a debugging session lasts for a couple of hours, I couldn't discover what exactly causes the problem. But, what is happening is the execution of updateStatusBar runs before TextFileEditor::setInput therefore all the changes trackers methods in updateStatusBar find the model undefined and don't change the state. Seems that the two methods aren't dependent on each other as when it runs on debugging mode it works fine.
I added setTimeout(() => this._onDidOpenInPlace.fire(), 100); To delay updating the status bar a bit and it worked just fine. I would like to understand why that behavior happens and for sure figure out a better way to solve it.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 8, 2018

@atitoa93 I like your solution very much, fixed the timeout. Thanks!

@bpasero bpasero merged commit 315d352 into Microsoft:master Oct 8, 2018

1 of 2 checks passed

VS Code in progress
Details
license/cla All CLA requirements met.
Details

@bpasero bpasero modified the milestones: On Deck, October 2018 Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment