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

Implements a starting tab reading API #131998

Merged
merged 10 commits into from Sep 2, 2021
Merged

Implements a starting tab reading API #131998

merged 10 commits into from Sep 2, 2021

Conversation

lramos15
Copy link
Member

This PR is part of ##15178

Initial work for a tab reading API. Hoping to get some feedback and it merged in so I can test with proposed API

@lramos15 lramos15 self-assigned this Aug 31, 2021
src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved
src/vs/workbench/api/browser/mainThreadEditorTabs.ts Outdated Show resolved Hide resolved
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

Looking good but the window.tabs.indexOf(window.activeTab) must work (given there is an active tab). Also some nit and more strictness

src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.proposed.d.ts Show resolved Hide resolved
src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.proposed.d.ts Outdated Show resolved Hide resolved
* An {@link Event} which fires when the array of {@link window.tabs tabs}
* has changed.
*/
export const onDidChangeTabs: Event<ReadonlyArray<Tab>>;
Copy link
Member

Choose a reason for hiding this comment

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

readonly Tab[] and consider an event which contains added, changed, removed tabs, similar to WorkspaceFoldersChangeEvent

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder the usefulness of that. I was just going off the onDidChangeVisibleTextEditors which returns the text editors array.

Copy link
Member

Choose a reason for hiding this comment

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

It will likely help you manage diagnostics, eg. remove diagnostics for closed tab. Have this information in the event means user don't need to iterate over the array all the time. Tho, this is P2 and not blocking merging this

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like if we were to add this we would definitely want to add an order as then it would be a lot harder to discern where these tabs were located

Copy link
Member

Choose a reason for hiding this comment

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

You need to explain why that motivates order and what order-value closed tabs should have?

Copy link
Member Author

Choose a reason for hiding this comment

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

For example a rename is basically just an open and close of a tab in the same location. It also could be interesting to know what tab now possesses the location of the previously closed tab. The order value would just be the index it had in the group. So it would be something like [{order: 0, viewColumn: 1}, {order: 1, viewColumn: 1}, {order: 0, viewColumn: 2}, {order: 1, viewColumn: 2}]

Additionally I either think we would want to have Added, Changed, Removed on the onDidChange or break up the events. onDidChange would just be the list, then onDidAdd, onDidRemove, onDidMove, onDidRename etc because there's a lot more than adding and removing a tab

Copy link
Member

Choose a reason for hiding this comment

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

So, you do agree that having knowledge about closed tabs is useful (sample here) but you couple its value to an order attribute? I think that is just artificial complexity and I wanna see a real world use-case

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure of a use case off the top of my head by I know added / removed is a common API pattern and I value the consistency. I will bring it up as a discussion for the API sync and we can discuss there. I'm going to merge this since this is first major chunk of the API and another PR will be used for adjustments

src/vs/workbench/api/browser/mainThreadEditorTabs.ts Outdated Show resolved Hide resolved
@lramos15 lramos15 merged commit c2d0a69 into main Sep 2, 2021
@lramos15 lramos15 deleted the lramos15/tabAPI branch September 2, 2021 14:48
@bpasero
Copy link
Member

bpasero commented Sep 3, 2021

@lramos15 I forgot to mention this in my review: can we please also have integration tests for this new API, should be quite easy to cover, even including custom editors and/or notebooks or even diffs.

@jrieken
Copy link
Member

jrieken commented Sep 3, 2021

code pointer: extensions/vscode-api-tests/src/singlefolder-tests/window.test.ts

@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants