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

Tab API implementation #12109

Merged

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Jan 25, 2023

What it does

resolves #11692

this implements the following parts of the vscode extension tab api:

Tab
TabChangeEvent
TabGroup
TabGroupChangeEvent
TabGroups
TabInputCustom
TabInputTerminal
TabInputText
TabInputTextDiff
TabInputWebview

Following InputTypes are implemented but not used by the actual Events since there is no support for it currently in theia.
TabInputNotebook
TabInputNotebookDiff

i sadly couldn't quite figure out what the preserve focus in the close functions is for. If someone knows please let me know.

How to test

you can find a small test-extension here: tab-test-0.0.1.zip
or build it here: https://github.com/jonah-iden/tab-test

There is 3 main functionalities to test:
The listeners can partly be tested while testing the other functionalities

test close current tab

  1. open a tab
  2. execute command "Close Active Tab"
  3. see current Tab should close.
  4. Also a message should pop up signifiying that a tabClose (or tab Group close if it was the last tab) event has occured

test close current Group

  1. open multiple tabs. At best in a second group (but works also when there is only one group)
  2. execute command "Close Active TabGroup"
  3. All tabs in current group should have been closed
  4. messages for close events should also pop up

other events to test

  • move tab in current group.
  • activate Tab
  • change tab from preview to normal
  • pin tab

Review checklist

Reminder for reviewers

@msujew msujew changed the title Jbicker/tab api implementation Tab API implementation Jan 25, 2023
@jonah-iden jonah-iden marked this pull request as ready for review January 26, 2023 12:22
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, that seems to mostly work well :)

I've noticed a few discrepancies between how the events work though between vscode and Theia:

  • In Theia, changing between tab groups also fires the onDidChangeTabs event, while vscode only fires the onDidChangeTabGroups event.
  • Changing between tab groups seems to often only work in one direction and the event fires with undefined changed even if the newly active tab has an active widget.
  • Changing between tabs fires the event twice, once with the old widget and once with the new widget.
  • Closing a tab group doesn't fire the close event for the individual tab.
  • Closing tabs/tab groups sometimes shows undefined and sometimes the last active tab. There seems to be a race condition somewhere. Closing a tab group should always show undefined while a tab should always show it's actual value.

packages/plugin/src/theia.d.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/tabs.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/tabs/tabs-main.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/tabs/tabs-main.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Jan 26, 2023
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way better, that led me to notice a few more, but relatively minor discrepencies:

  • Moving a tab from one tab group to another only triggers the open event. It should also trigger the delete event just before that (at least that's what vscode does)
  • The update event is currently triggered every time a tab is refocused, after moving focus to another part of the application (such as the file explorer). The event is only supposed to trigger on:
    • Focusing a different tab group (within the main panel)
    • Focusing another tab within the same tab group
    • On changes of the pin status
    • On changes of the dirty status
    • On changes of the label of a widget

Maybe you can take a look at the WindowTitleUpdater for some inspiration.

Otherwise this looks already fairly good :)

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/tabs/tabs-main.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/tabs/tabs-main.ts Outdated Show resolved Hide resolved
fix for onTabChange activating after defocusing and focusing tab again;
fix for moving tab between groups, now shows delete event correctly
and no event when dropped to previous tabbar

Signed-off-by: Jonah Iden <[email protected]>
Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some questions reading through the code.

@tsmaeder
Copy link
Contributor

@jonah-iden did you notice the linter job is failing?

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, one last minor regression to fix and afterwards this PR should be good to go:

When the main area is empty and we open an editor for a file, the TAB_GROUP_OPEN and TAB_CHANGE event fire. However, in between them the TAB_OPEN event should happen as well. I believe this worked correctly previously, but has changed due to another change in the code.

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 2, 2023

@msujew can we get a hallelujah on this one from your side or are there still open issues?

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe everything is good now. At least to the point where I can't reasonably find differences between the way the events trigger in this PR compared to vscode.

So therefore, looks good to me :) Good job! I'll merge this PR later today, if there are no objections in the meantime.

@msujew msujew merged commit d59d527 into eclipse-theia:master Feb 2, 2023
tsmaeder pushed a commit to tsmaeder/theia that referenced this pull request Feb 3, 2023
Signed-off-by: Jonah Iden <[email protected]>
Co-authored-by: Jan Bicker <[email protected]>
Co-authored-by: Paul Maréchal <[email protected]>
tsmaeder added a commit that referenced this pull request Feb 3, 2023
Signed-off-by: Jonah Iden <[email protected]>
Co-authored-by: Jonah Iden <[email protected]>
Co-authored-by: Jan Bicker <[email protected]>
Co-authored-by: Paul Maréchal <[email protected]>
@msujew msujew mentioned this pull request Feb 21, 2023
1 task
@vince-fugnitto vince-fugnitto added this to the 1.35.0 milestone Feb 23, 2023
@tsmaeder
Copy link
Contributor

tsmaeder commented May 3, 2024

@jonah-iden am not mistaken this PR does not support more than a single tab group? (https://github.com/eclipse-theia/theia/pull/12109/files#diff-cf9def33dbeeb0cd3fa2f7b8026959104e2461870a8c5c8c1dfd7487fad6ac9eR145). Is there a follow-up issue to track this limitation? If I understand it correctly, Theia does support splitting the main editor area.

@jonah-iden
Copy link
Contributor Author

@tsmaeder yeah seems like I must have overlooked that when implementing this API. I don't think there is an issue somwhere but i can create one of course

@tsmaeder
Copy link
Contributor

tsmaeder commented May 3, 2024

The typescript build-in has started using tab groups extensively to track what the need to compute in the language server. We as a group will need to start cleaning this up.

@jonah-iden
Copy link
Contributor Author

Oh ok yeah then we definitely need to implement this quickly. I'll create an issue maybe i could even start taking a look at it today

@jonah-iden
Copy link
Contributor Author

Now that i looked more at it, tab groups in itself seem to be working fine. Is there any specific problem you are experiencing? I created a simple issue here #13677

@tsmaeder
Copy link
Contributor

tsmaeder commented May 3, 2024

If you mean "they correctly implement the VS Code semantics", then no, they do not work fine, as far as I can tell. Examples:

  1. There is only ever a single "viewColumn" value, even if we have split editors
  2. There always needs to be at least one tab group, even if no editors are open. If no editors are open, it's the active tab group.
  3. The viewColumn values between editors and the tab groups they are in are off by one (2 instead of 1 for the initial tab group)
  4. Editors in secondaryWindows are in no tab group.

In general, we need to observe the semantics VS Code implements and do the same, in particular in the corner cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode: add tab related apis
6 participants