search

eclipse-theia / theia

Eclipse Theia is a cloud & desktop IDE framework implemented in TypeScript.
http://theia-ide.org
Eclipse Public License 2.0
19.29k stars 2.45k forks source link

Fix tab group API event order #13812

Closed msujew closed 2 weeks ago

msujew commented 3 weeks ago

What it does

Works around (but not actually closes) https://github.com/eclipse-theia/theia/issues/13679

This fixes the tab group API event order and ensures that the tab groups are always up-to-date on the plugin host by delaying the triggering of other events like onDidChangeActiveEditor.

This also sets all columns of groups to 0. This should at least work around https://github.com/eclipse-theia/theia/issues/13679. See follow ups for more details on this.

How to test

  1. Assert that the CI is green (TypeScript tests pass)
  2. Open the App and open a TypeScript file
  3. The TypeScript commands should be available
  4. This should also work after switching between tabs and tab groups.

Follow-ups

The implementation of the ViewColumnService hasn't been kept up-to-date with the current requirements of Theia or VS Code. For example, it isn't aware of the position of tab groups (only of tabs) or of floating editor windows. And even for those tabs, it yields very much unexpected values. This needs an entire refactoring using something similar like VS Code's EditorGroupService implementation.

I'll probably get to this in a separate PR, but that's a larger issue so I'm not sure when I'll be able to do that.

Review checklist

Reminder for reviewers

tsmaeder commented 2 weeks ago

If I understand correctly, the idea of this workaround is to delay notifications through the editor service until the tabs main service has had time to update the tab info, right? The problem seems to arise because we state (editor state, tab groups) that needs to be update "atomically" in order to not present an inconsistent picture to clients. However, since we're using independent widget listeners to trigger updates to various components, we can't sequence the actions properly. To me it sounds as if we needed to introduce a single coordinator object that listens to the widget changes and then orchestrates the proper sequence of updates. Once the local state is consistent again, we can replicate the state to the back end/plugin host. Your thoughts?

msujew commented 2 weeks ago

@tsmaeder My thoughts exactly, see the follow-ups section of the PR. There would only be a single service that would fire events in the expected order while also correctly keeping track of the tabbars/viewcolumns.