onDidChangeTextEditorViewColumn doesn't fire when an editor is dragged between open columns

[eamodio]

• VSCode Version: Code - Insiders 1.17.0-insider (f6ef4be, 2017-10-04T17:40:56.306Z)
• OS Version: Windows_NT x64 10.0.16299

---

Steps to Reproduce:

1. Subscribe to the window.onDidChangeTextEditorViewColumn event
2. Open a few files with a layout so that you have some tabs in each of 2 columns
3. Drag a tab from one column into the other
4. 🐛 Observe that the window.onDidChangeTextEditorViewColumn event doesn't fire

It seems that window.onDidChangeTextEditorViewColumn only fires when an editor is the first in a new column or the last in a column.

//cc @jrieken

Reproduces without extensions: No

[jrieken]

@eamodio Are you dragging a tab which editor is visibile to the user or a background tab? In the latter case it is expected that you don't receive an event because a tab (with out editor) isn't an editor

[eamodio]

@jrieken I'm dragging a visible editor tab

[jrieken]

Yeah, I see what you mean... Funky...

[eamodio]

Somewhat related, is there any way to know/get/correlate to the original editor during a column move? Right now this event only gives you the new editor instance and the new column location, but there doesn't seem to be a way to correlate that to the original editor instance that is changing columns.

The combination of these is currently causing me issues with GitLens as I'm trying to maintain blame annotation state in an editor -- even if you switch tabs, move columns, etc. Switching tabs is fine, because the editor instance remains the same, but moving columns causes a new editor to be created and the original editor to be disposed and I can't figure out a good way to correlate the two because the id changes, and while the document instance is the same between them, it is also the same between open editors for the same file (and I'd much rather have the state per-editor rather than per-document).

[jrieken]

💩 the underlying issue here is that the event representing the tab changes is send before the model is set to the text editor widget. For the API (which calls the combination of the editor widget and document a text editor) that means it doesn't know the new editor yet...

@bpasero Is firing the onEditorsChanged and onEditorsMoved events before the editor widgets are updated the intended behaviour here?

[bpasero]

@jrieken the onEditorsChanged is our old onEditorInputChanged event and that one should only fire when the input has changed in the editor instance. I have to check for the onEditorsMoved event. Can you share some steps what you are doing and where you set the breakpoint to see that the event comes before the editor control is updated?

[jrieken]

Update logic starts here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/electron-browser/mainThreadEditors.ts#L62. Steps as described above

[bpasero]

@jrieken I see that today 2 events are used to drive this:

• onEditorsChanged
• onEditorsMoved

I turns out that onEditorsMoved was badly named, it was actually only fired when an editor group is reordered (by grabbing the entire group and dropping it to another place). I pushed a change to rename this event to onEditorGroupMoved. I verified that reordering groups correctly results in the onDidChangeTextEditorViewColumn events.

That leaves us with the issue of moving an editor tab to another group which is broken atm. I have a feeling that instead of onEditorsChanged (which is fired for a lot of things that we do not care about), we should have a new event onEditorMoved that mainThreadEditors listens on instead. I can easily add this event and if it helps, it could also carry around the information from which position to which position the editor was moved.

However, another question is when this event should be fired. Currently the onEditorsChanged fires multiple (3) times (when moving an editor around) in the different phases of this interaction:

• when the editor opens in the group that the editor is dropped to
• when the editor closes in the group that it was dragged from
• when the editor "behind" the dragged one opens because it previous active one was moved

If we had a new event onEditorMoved, I could probably wire it in so that it only fires after all of the 3 editor change events are done. However I am not sure that this is enough for the editorPositionData logic in mainThreadEditors and extHostTextEditors to catch up. It seems that when the event fires so late, the extension host has already updated its list of editors and no change would be recorded.

In the end it would probably be easier to ditch the editorPositionData and just forward the new onEditorMoved event to the extension host side with all the information about which editor moved from where to where.

[eamodio]

Just my 2c -- +1 for 1 event that encompasses the whole move chain

[jrieken]

However I am not sure that this is enough for the editorPositionData logic in mainThreadEditors and extHostTextEditors to catch up. It seems that when the event fires so late, the extension host has already updated its list of editors and no change would be recorded.

When a new api editor (document+editor) comes into play we "read" it's editor position eagerly. It must not always have one because of embedded, diff, peek etc editors. I don't think we a problem with those because the editor widget is already/always placed in a workbench editor

One event is always better then many, I wouldn't change the logic tho. A workbench editor isn't an api editor and an editor widget isn't an editor. Those max 3 api editors that can have column data is cheap to compute in the way we do it today

[bpasero]

@jrieken yeah I would not change the logic. The challenge I have seen is that if I send just one event after all the dust settles (all editor operations are done), the receiving side $acceptEditorPositionData does not emit the event because there is no mis-match in columns on the extension host side anymore (probably the editors by then are already updated).

[jrieken]

... does not emit the event because there is no mis-match in columns on the extension host side anymore (probably the editors by then are already updated).

Hm... This makes me think that dragging an editor from column 1 to column 2 would create a new editor anyways... because the model moves from one editor widget to another... Confusing myself... I believe the change is only valid in our API when having 2 or 3 columns and when removing column N-1... Maybe, not even sure about that

[bpasero]

@jrieken yes that is true. the editor-group-move event fires today when you either reorder groups via DND or when you close a group to the left and the others are moving.

Moving an editor from one group to another basically means that the editor in the group that it was moved from shows the previous opened file and that at the destination group the editor changes to the dragged one. So with that it seems kind of hard to compute the event properly based on editors.

[jrieken]

It's unfortunate... It maybe wasn't the best idea to have that event

[bpasero]

@jrieken should we only react on the editor group move event and not on the editor change event (here)? This would mean no more events for moving an editor from one tab to another (which is broken today anyways), but the group-move is working reliably because we keep the editors around in that case. We could just update the documentation around this to clarify what this event means.

[jrieken]

We could just update the documentation around this to clarify what this event means.

That's a good start. How realistic is it too actually move the editor, I mean not set/unset/swap the documents but move the editor around?

[bpasero]

@jrieken needs thinking. today the model is very simple: each group (up to three) has exactly one Alex editor ready to show documents. If a group actually closes because of the last editor closing I am actually already reparenting the editor control to the correct group (this is what happens when the editor group move event is fired).

For moving an editor across groups things are more complicated because the number of groups typically stays the same and the group where the editor is moved from typically has another tab in the background that now wants to become active. If we would reparent the editor to the group you are dragging to, we no longer have an editor for the inactive tab that now becomes active. We would then have to also reparent the editor from the other group over, which could result in weird events (because the user did not actually move the editor in both ways).

[bpasero]

Closing "as designed". Dragging a tab to another editor group is a close of the editor in the source group and the opening of a (potentially) new editor in the target group. It is unlikely that I would change this behaviour in the workbench for now.