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

Support contributions to the shell layout #10981

Closed
wants to merge 1 commit into from

Conversation

msujew
Copy link
Member

@msujew msujew commented Apr 1, 2022

What it does

Allows downstream apps to contribute widgets to the layout without extending the ApplicationShell itself. I had the idea while reviewing #10731, where overriding the ApplicationShell to contribute a new widget to the layout seemed a bit overengineered.

So far it only supports adding panels under the top panel, although we could extend this for all areas of the app. Although, I'm not totally sure that's even useful.

Aside from the toolbar, I can imagine that we reuse this API to also add support for banners in the future like in VSCode:

image

How to test

The test case for the introduced ShellLayoutContribution is the toolbar. Simply confirm that it behaves as usual.

Review checklist

Reminder for reviewers

@msujew msujew added shell issues related to the core shell toolbar issues related to the toolbar labels Apr 1, 2022
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This looks good to me. It increases the extensibility of the platform and reduces the need for overrides, which is definitely positive. One concern I have - and this applies as much to the existing toolbar as to this PR - is that the shell's list of expected areas is static (top, bottom, left, right, main), but now some widgets are outside of those (toolbar - at the top, but not in the top panel). To really achieve the flexibility this introduces, we'd need to have the shell be more dynamic in its expectations so that it can handle widget interactions (adding, removing) from more areas.

@@ -581,13 +585,24 @@ export class ApplicationShell extends Widget {
const panelForSideAreas = new SplitPanel({ layout: leftRightSplitLayout });
panelForSideAreas.id = 'theia-left-right-split-panel';

const contributedTopPanels = this.getContributedPanels('top');
const contributedTopStretch = contributedTopPanels.map(() => 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters much, but a more explicit way to do this would be New Array(contributedTopPanels.length).fill(0). Possibly there's some saving on not running a .map that may reserve space for (pointers to) the panels?

@msujew
Copy link
Member Author

msujew commented Sep 15, 2022

I'll maybe come back to this later. For now, I don't really have an approach on how to fix the underlying issue, i.e. the rigid shell structure.

@msujew msujew closed this Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell issues related to the core shell toolbar issues related to the toolbar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants