Add global toolbar 🛠

[kenneth-marut-work]

Description

Fixes: #5037, Fixes: #7540
This PR adds an fully customizable and optional toolbar to the application shell as discussed here #10501.

Features include:

Main Functionality

• Toolbar that can be hidden/shown via alt+t, by right clicking the toolbar, or through a new preference (hidden by default)

• A set of default command toolbar items including back and forth, split editor, new files/directories

• A new toolbar.json file that lives in the user's .theia directory to manage toolbar customizations

• A contribution point for packages to contribute custom React toolbar items

• Ability to drag and drop individual command and contributed icons to the LEFT, CENTER, or RIGHT areas of the toolbar, or anywhere in existing toolbar groups

• Ability to add a new group (right click on item), which adds a vertical separator | to the left or right of the toolbar item

• Add/remove toolbar items by right-clicking, or by editing the JSON file directly (probably not recommended, but possible)

• Reset toolbar to defaults by right clicking an empty area of the toolbar

• Add custom items to the toolbar, which opens a quickpick with all available commands, and new dialog to select a custom icon (codicon or font awesome). Will show a default icon option if one is available

• 1 contributed custom toolbar item (easy-search-toolbar-item) to demonstrate custom React toolbar widgets

Easy Search Toolbar Item

• Makes different search features discoverable including command palette, file search (ctrl+p), global text search, workspace root search (custom functionality which prepopulates filemasks in Search-in-workspace widget), symbol search

---

How to test

• Build this branch
• Start application and enable the toolbar using ctrl+t
• Experiment with its features (add, remove, rearrange toolbar items)
• Explore the toolbar.json file which is accessible via right-clicking the toolbar

Breaking Changes

• This PR moves the shell initialization from its constructor into the postConstruct so that extenders (including the toolbar) can more easily override the shell's layout

Not yet included:

• Toolbar customization is only available in user scope (there has been interest of adding workspace scope support as well)

---

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

I'll test the changes more closely in the coming days, two things I noticed which I think would make the feature more consistent with the UX of the rest of the framework would be:

• a similar hover behavior as editor toolbar items and buttons (hover background) when
  hovering over items in the main toolbar. Ex:

hover-item.mp4

• a secondary color (not the main foreground color) for separators
• the icon placement may need to be adjusted, it seems as though there is more space at the bottom:

• in the future we may want to make the toolbar items keyboard accessible, using tab to cycle through the toolbar

[msujew]

I agree with Vince in regard to the UX changes. I also think that we should remove the dropshadow effect, it feels a bit out of place.

Extensive review following later today.

[msujew]

This is looking really good in general already, good job! 👍

I have a few suggestion to improve on the i18n of this feature, most of it can be easily addressed by introducing new translation keys. I also have a more general discussion topic, where I would like to get some input from other contributors:

I don't think that overriding the ApplicationShell to add toolbar support is an optimal solution to this problem. In my opinion, the toolbar UI is a "contributed" feature, rather than an "inherited" one, if that train of thought makes sense. Also, in my experience, downstream users of Theia often tend to override the ApplicationShell themselves, making this change a bit more annoying to integrate into existing apps.

Going with the "contributed feature" thought leads me to believe that there should be a way to customize the ApplicationShell (i.e. adding panels beside/above/below existing panels) using contributions. An example API I had in mind:

enum PanelLayoutDirection {
  Left,
  Right,
  Above,
  Below
}

interface ApplicationShellPanelRegistry {
  // `relativeId` is the id of a panel the new panel should be placed in relation to
  // e.g. for the toolbar it would be used like this: 
  // `registerPanel(toolbar, PanelLayoutDirection.Below, 'top-panel');`
  registerPanel(panel: Panel, direction: PanelLayoutDirection, relativeId: string)
}

interface ApplicationShellContribution {
  registerPanels(registry: ApplicationShellPanelRegistry)
}

By the way, I'd be alright with merging this PR in its current state first and refactoring it later, given that this would be quite a large change in the API.

[kenneth-marut-work]

I agree with Vince in regard to the UX changes. I also think that we should remove the dropshadow effect, it feels a bit out of place.

Extensive review following later today.

@msujew I had added the shadow to distinguish the left/right panels from the toolbar. Removing it blends the two together in a somewhat awkward way since the icon sizing is different:

Alternatively I can add a solid border below (ignore icon alignment for now, just a prototype 😉):

Though this can cause some visual clutter around active editor tabs:

[vince-fugnitto]

Alternatively I can add a solid border below (ignore icon alignment for now, just a prototype 😉):

@kenneth-marut-work I'm thinking defining a color that is a slightly darker version of the sidepanels would likely suffice, even just using darken on the existing variable? (In the screenshot it would look something I guess like the tree-view parts headers).

[kenneth-marut-work]

Alternatively I can add a solid border below (ignore icon alignment for now, just a prototype 😉):

@kenneth-marut-work I'm thinking defining a color that is a slightly darker version of the sidepanels would likely suffice, even just using darken on the existing variable? (In the screenshot it would look something I guess like the tree-view parts headers).

@vince-fugnitto great suggestion. I think that worked out nicely

[kenneth-marut-work]

I agree with Vince in regard to the UX changes. I also think that we should remove the dropshadow effect, it feels a bit out of place.

Extensive review following later today.

This is looking really good in general already, good job! 👍

I have a few suggestion to improve on the i18n of this feature, most of it can be easily addressed by introducing new translation keys. I also have a more general discussion topic, where I would like to get some input from other contributors:

I don't think that overriding the ApplicationShell to add toolbar support is an optimal solution to this problem. In my opinion, the toolbar UI is a "contributed" feature, rather than an "inherited" one, if that train of thought makes sense. Also, in my experience, downstream users of Theia often tend to override the ApplicationShell themselves, making this change a bit more annoying to integrate into existing apps.

Going with the "contributed feature" thought leads me to believe that there should be a way to customize the ApplicationShell (i.e. adding panels beside/above/below existing panels) using contributions. An example API I had in mind:

enum PanelLayoutDirection {
  Left,
  Right,
  Above,
  Below
}

interface ApplicationShellPanelRegistry {
  // `relativeId` is the id of a panel the new panel should be placed in relation to
  // e.g. for the toolbar it would be used like this: 
  // `registerPanel(toolbar, PanelLayoutDirection.Below, 'top-panel');`
  registerPanel(panel: Panel, direction: PanelLayoutDirection, relativeId: string)
}

interface ApplicationShellContribution {
  registerPanels(registry: ApplicationShellPanelRegistry)
}

By the way, I'd be alright with merging this PR in its current state first and refactoring it later, given that this would be quite a large change in the API.

@vince-fugnitto @msujew
Thanks for the review, I have implemented your suggestions and have identified other areas that for i18n. I also think the UI comments that both of you had suggested are a big improvement for consistency with the rest of the UI.

@msujew
I agree with your ideas for adding an API to extend the shell, I think it would come in handy for a lot of extenders (and definitely would have been nice for me when designing the toolbar). But I also agree that this should be done in a follow-up PR.

I have also pushed a minor change that I believe makes the default layout much easier to customize for extenders. Previously I had expected MainToolbarContributions to declare a column and priority field, and then proceed to go through some steps of sorting, prioritizing, and appending contributions to the main-toolbar-defaults. I have removed that code in favor of extenders solely overriding the main-toolbar-defaults and manually setting the layout. I think that this gives more control to downstream IDEs and removes an unnecessary layer of confusion.

[vince-fugnitto]

A couple of quick comments:

• please include a readme for the extension following the template we have for other extensions (it would be the first thing that consumers would see when looking at npm.
• we should probably think about what toolbar items should be present by default, I think the custom easy-search should either be added to @theia/api-samples or replaced by a generic more (... icon) toolbar item that downstream applications can contribute context-menus to - the new terminal can also likely be removed but I'm open to suggestions.
• disabled toolbar items should not have the pointer cursor.
• we should try to aim to have consistent sizes for toolbar items (at the moment the easy-search seems bigger - I understand it is custom).
• I don't think that search-in-workspace-root-quick-input-service.ts should belong in a generic toolbar extension, perhaps it can be factored out - this might be related to removing the easy-search out to @theia/api-samples.
• we should prefix the files by toolbar- and not main-toolbar- since the extension and folder is named toolbar (nitpick)

Regarding what should be included by default I'm open to suggestions :)

[kenneth-marut-work]

@vince-fugnitto

• please include a readme for the extension following the template we have for other extensions (it would be the first thing that consumers would see when looking at npm.

• Added a readme, please let me know if there's anything else that should be added

• we should probably think about what toolbar items should be present by default, I think the custom easy-search should either be added to @theia/api-samples or replaced by a generic more (... icon) toolbar item that downstream applications can contribute context-menus to - the new terminal can also likely be removed but I'm open to suggestions.

• I've moved the easy-search item to api-examples and also have added a Command Palette item to the list of defaults. At this point we have: back, forward, split editor, and Command Palette. The only one I would maybe consider removing is the Split Editor (but I like that it demonstrates the "group separator". So I'm happy to swap it out with something else.

• disabled toolbar items should not have the pointer cursor.

• Done

• we should try to aim to have consistent sizes for toolbar items (at the moment the easy-search seems bigger - I understand it is custom).

• Added a couple fixes to make sure sizes are enforced, also fixed some sizing issues I noticed when adding Font Awesome icons

• I don't think that search-in-workspace-root-quick-input-service.ts should belong in a generic toolbar extension, perhaps it can be factored out - this might be related to removing the easy-search out to @theia/api-samples.

• See above

• we should prefix the files by toolbar- and not main-toolbar- since the extension and folder is named toolbar (nitpick)

• I've done a mass renaming but kept the toolbar's CSS id as #main-toolbar just for specificity

Thanks for the suggestions 👍

[msujew]

I noticed that disabling/enabling toolbar elements lags behind, until the toolbar is forcefully updated by clicking on a command. It seems like the change of isEnabled cannot be really propagated to the toolbar (there's no event for that).

For example, when closing all editors the "Split Editor" button is disabled. When opening a file, the button stays disabled. Clicking on it "enables" the button and executes the command.

I found that to be a bit confusing and leads me to believe that we maybe shouldn't render disabled buttons as being truly disabled. On the other hand, this could lead to confusion as well. What do the others think about that?

[vince-fugnitto]

I found that to be a bit confusing and leads me to believe that we maybe shouldn't render disabled buttons as being truly disabled. On the other hand, this could lead to confusion as well. What do the others think about that?

@msujew I agree, we should probably keep them disabled but be able to know when an underlying command is enabled/disabled so that the toolbar item updates accordingly. The toolbar items (for views and the editor) seems to be able to make this update, perhaps it is something we can follow.

If it is something not feasible for the moment we can also consider it a future improvement and make the items always enabled.

[kenneth-marut-work]

I found that to be a bit confusing and leads me to believe that we maybe shouldn't render disabled buttons as being truly disabled. On the other hand, this could lead to confusion as well. What do the others think about that?

@msujew I agree, we should probably keep them disabled but be able to know when an underlying command is enabled/disabled so that the toolbar item updates accordingly. The toolbar items (for views and the editor) seems to be able to make this update, perhaps it is something we can follow.

If it is something not feasible for the moment we can also consider it a future improvement and make the items always enabled.

@msujew
Are you seeing the same behavior for back/forward? This might only be an issue for the split editor command. I remember seeing this a while back and it may be related to that particular command only

[vince-fugnitto]

@msujew Are you seeing the same behavior for back/forward? This might only be an issue for the split editor command. I remember seeing this a while back and it may be related to that particular command only

@kenneth-marut-work back and forward do seem to work well 👍 I confirmed it seems to be the split-editor which doesn't, although the command itself does seem to update from the command-palette (but of course the command-palette is not persistent).

[kenneth-marut-work]

I found that to be a bit confusing and leads me to believe that we maybe shouldn't render disabled buttons as being truly disabled. On the other hand, this could lead to confusion as well. What do the others think about that?

@msujew I agree, we should probably keep them disabled but be able to know when an underlying command is enabled/disabled so that the toolbar item updates accordingly. The toolbar items (for views and the editor) seems to be able to make this update, perhaps it is something we can follow.
If it is something not feasible for the moment we can also consider it a future improvement and make the items always enabled.

@msujew Are you seeing the same behavior for back/forward? This might only be an issue for the split editor command. I remember seeing this a while back and it may be related to that particular command only

@msujew Are you seeing the same behavior for back/forward? This might only be an issue for the split editor command. I remember seeing this a while back and it may be related to that particular command only

@kenneth-marut-work back and forward do seem to work well 👍 I confirmed it seems to be the split-editor which doesn't, although the command itself does seem to update from the command-palette (but of course the command-palette is not persistent).

Ah, I can see that this may be an intermittent issue depending if/how a regular TabBarToolbarItem has registered an onDidChange. I may be able to hook into the TabBarToolbarRegistry's onDidChange event to update the toolbar in response. I'll give that a try

[kenneth-marut-work]

@vince-fugnitto @msujew
I've hooked into the toolbar registry's onDidChange which seems to solve the enabled/disabled issue

[msujew]

While your recent changes have improved the behavior of the disabled/enabled buttons @kenneth-marut-work, it still has some hiccups. Opening a file enables the Split Editor button, but no actions afterwards seem to influence it (again, clicking on the toolbar updates the enabled state, but nothing else).

I'd be in favor of removing the ability to disable buttons on the toolbar. I think the feature looks and works well enough for a release today. We will have to polish it anyway in the upcoming weeks, e.g. I plan to add the proposed shell contribution API.

Also a minor, general comment: We should probably remove the localize/localizeByDefault calls from the toolbar api-samples, this makes automatic translation (key extraction) a bit more difficult.

[kenneth-marut-work]

While your recent changes have improved the behavior of the disabled/enabled buttons @kenneth-marut-work, it still has some hiccups. Opening a file enables the Split Editor button, but no actions afterwards seem to influence it (again, clicking on the toolbar updates the enabled state, but nothing else).

I agree there is still some funny business going on with the split editor command. It looks like that command has not registered an onDidChange so we are likely seeing different results (likely as a side effect of some other update). For example, I do see the enabled state update when opening and closing editors, but only when opening via Ctrl+P (apologies for the bad GIF compression). Still not enough to say that it works flawlessly.

It's worth noting that commands that are registered in the tabbarToolbarRegistry with an onDidChange behave as expected. For example Search: Refresh.

I'd be in favor of removing the ability to disable buttons on the toolbar. I think the feature looks and works well enough for a release today. We will have to polish it anyway in the upcoming weeks, e.g. I plan to add the proposed shell contribution API.

@vince-fugnitto any thoughts? I'm happy to always show items as "enabled" if you agree.

Alternatively, maybe there is some compromise, for example that defaults toolbar items to always enabled unless they have registered an onDidChange with the toolbar registry. Though this would not account for some of the accidental/desirable updates we see as side effects.

I'm realizing now that I've picked 3 very finicky commands to showcase as defaults :)

[vince-fugnitto]

@kenneth-marut-work

@vince-fugnitto any thoughts? I'm happy to always show items as "enabled" if you agree.

I'm fine with showing them as enabled as a first contribution to the repo in today's release, we can think of a strategy to fix the problem in subsequent improvements.

I'm realizing now that I've picked 3 very finicky commands to showcase as defaults :)

It happens :) If you feel like it's problematic or not all that useful we can remove it as one of the defaults, keep the default toolbar simple.

[kenneth-marut-work]

@msujew thanks for the additional suggestions. I've addressed your comments including renaming, removing internationalization in api-samples, and defaulting all commands to enabled.

[vince-fugnitto]

The changes look good to me for today's release 👍
There are still things I'd like to clean up afterwards and some styling as well but we can tackle it in subsequent improvements.

[msujew]

Thanks for the changes Ken! I agree with Vince, this is looking good enough for merging 👍

[JonasHelming]

Thanks @kenneth-marut-work for contributing this and thanks @vince-fugnitto and @msujew for the quick review cycle! I am very happy to see this feature in the next release!