sticky TabMovesFocus config

[jrieken]

Steps to Reproduce (1/2)

• enable workbench tab (showTabs)
• have two editor columns, editors in each
• run the Keyboard Shortcuts config action
• select Cmd+1 to focus column Open Source VS Code #1
• select a different writable editor in that column
• 💥 tab moves focus

Steps to Reproduce (2/2)

1. Have a 2 pane layout.
2. Hit Cmd , to open user settings.
3. Hit Cmd 1 to focus the first pane.
4. Use quick open to switch to an editable file.
5. Hit Cmd 2 to focus the user settings and make any valid change there.
6. Hit Cmd 1 to focus the first pane.

Tab will now focus away from the editor instead of inserting a TAB.

[joaomoreno]

Yeah today I was in a state in which Tab was moving focus instead of indenting. I couldn't reproduce though. I know I didn't have tabs enabled, too.

[jrieken]

@alexandrudima I have pushed da56a2c to 'fix' this cos I could see this event wasn't fired after the upateOptions call. Unsure what the underlying issue is.

[jrieken]

@joaomoreno Can you check with my steps from above without tabs? Also can you check that my change fixes this?

[joaomoreno]

Yup! I can repro before your change but not anymore after it. 👍

[jrieken]

When the Cmd+, command happens a third editor is created and leaked. That editor is readonly and somehow interferes with another editor...

[jrieken]

@bpasero Please check the changes in 3f446e4. The issue is like this:

• editors in a single column/pane share the same keybinding context
• editor inputs that are somehow different cause multiple code editor widgets to be created for one column (which then share the same kb context)
• when an editor is invisible it still listens to configuration change events and updates its editor widget (respectively its context cause this), editors don't seem to be disposed ever

My main question is why we have more than (max) three code editor widgets?

[joaomoreno]

I hit another problem which led me to realise that all editor settings are affected. I've updated the initial comment with more generalised repro steps.

[jrieken]

bummer - that's likely due to the change @aeschli and I made. Invisible editors don't listen to config changes anymore tho probably should update when becoming visible...

[jrieken]

We must do something about this...

[jrieken]

@joaomoreno I have moved the issue you described in #9892 cos it's a regression from not listening to config change event anymore

[joaomoreno]

Got it, thought it to be exactly the same. Sorry about that.

[bpasero]

@jrieken we can have more than 3 code editor widgets easily once we show the diff editor. So typically you can have 6 editors lying around (3 code editors and 3 diff editors).

There is no specific reason why a second code editor instance is being created for some inputs (readonly) and the one we have for files not being reused. This could probably be changed, but given the diff editor challenge, we might need a better solution to handle that case as well.

[jrieken]

How about disposing?

[bpasero]

@jrieken yes I can do that. @alexandrudima objections?

The consequence is that an editor gets disposed as soon as either another editor or non-editor opens in a slot (this includes binary files, HTML previews, extension editors).

[alexdima]

Not optimal, but disposing editors should be fine: there is some start-up time (in the range of a few millis) that is spent mostly instantiating contributions, but it might be non-noticeable.

[bpasero]

@jrieken @alexandrudima I am also fine to have API to call to indicate to the editor that it is hidden, in fact I am already calling onHide() on the editor when this happens: https://github.com/Microsoft/vscode/blob/master/src/vs/editor/common/editorCommon.ts#L3564 from https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/textEditor.ts#L163

[alexdima]

I'm sorry I don't have the time to go through all editor contributions and adopt in each and every one of them that they don't touch context keys unless the editor is visible. It is also a solution that does not guarantee bug-free new contributions. (It is possible that people update those context keys from external sources in various event listeners)

Moreover, a pointer to the keybinding service can be grabbed by any editor client and a context key can be set outside editor contributions.

[bpasero]

Ok I think I need more background on the issue, since I am not on the same page since I was on vacation. Somehow I thought this issue was coming from our use of keybinding service but I probably got that wrong.

Note that we have hidden editors since the beginning so I am not sure what recent change causes the editor config to get wrong.

[alexdima]

Well, an example always helps. Consider the context key findWidgetVisible.

The FindController (that is an editor contribution) manages this context key. Specifically:

• when the find controller comes up, it sets the key to false.
• when the find widget gets shown, it sets the key to true.
• when the find widget gets hidden, it sets the key to false.
• when the find controller gets disposed, it sets the key to false.

Now consider two editors that each have their own FindController working on the same keybinding service (which is what happens today). Now, there will be two instances of the FindController and they will both write to the findWidgetVisible key in the keybinding service.

Most times, the first one maybe has sufficient time to stop itself (via setModel(null) on the editor perhaps that leads to the find widget closing, that leads to the key being set to false) and the second one runs after the first one (via setModel(model) on the second editor that leads to the find controller restoring its previous state, that might lead to the key being set to true.

Now, if the timing is not spotless or if there are other conditions, the "hidden" find controller might interfere with the "visible" find controller and set the findWidgetVisible context key to an unexpected value.

[bpasero]

@alexandrudima yeah I see the issue but is that not #10122 ?

I am asking because this issue is about sticky TabMovesFocus config. Is it the same issue?

[alexdima]

They might all have the same root cause. Context keys might end up with incorrect values when multiple editor instances try to drive them.

[bpasero]

Joh explained to me, I got it now. All related. We decided to start to dispose() hidden editors.

@alexandrudima is there a difference between dispose() and destroy() in your world?

[alexdima]

I left destroy() in for compatibility purposes (simply calls dispose()), dispose() is preferred.

[bpasero]

Merging into #10358