Styles for status bar elements

[vitaliy-guliy]

Different theme dependent styles for status bar elements.
Issue #1839

Signed-off-by: Vitaliy Guliy vgulyy@redhat.com

This is my status bar element with style INFO

and with style ERROR for offline mode

[spoenemann]

You could simplify this selector to

#theia-statusBar .element.warning

so you don't need to duplicate for left and right. Same for the other styles.

[vitaliy-guliy]

Done, thanks!

[benoitf]

@vitaliy-guliy is your Signed-Off correct (codenvy email ?)

[spoenemann]

The selectors are still duplicated. You can remove lines 56, 61, 66, 71.

[vitaliy-guliy]

Fixed. Thank you!

[vitaliy-guliy]

@benoitf squashed again and changed to RedHat email. Thanks.

[spoenemann]

Looks good to me.

[akosyakov]

VS code does not allow to change the background color of status bar entries only of text: https://code.visualstudio.com/docs/extensionAPI/vscode-api#StatusBarItem. Why do you need it?

[vitaliy-guliy]

@akosyakov To have status bar element with custom background as on screenshots.

[vitaliy-guliy]

@akosyakov press Ctrl+M in VS code to test

[akosyakov]

VS code changes the background color of the whole status bar, not entries. You can do it with StatusBar.setBackgroundColor in Theia as well without any API changes. Was it your original intention to color the individual entry?

If we allow it why should we limit us with such set of predefined styles? Why not for example backgroundColor?: string and then you can pass backgroundColor: 'var(--theia-error-color0)'? Or maybe we should not allow it and have API to mark a single entry as important? Imagine everybody start using different background colors, the status bar is going to look like a christmas tree and one won't already know which element is really important.

[svenefftinge]

I agree with @akosyakov that coloring status bar items individually doesn't look good. Why do you want to do that?

[vitaliy-guliy]

In my case I need to add a status bar element with green background. Having it in green makes it more contrast. You can see my cases on attached screenshots.
Sure, I can do it without changing Theia core. Just find theia-statusBar html element, then find an appropriate child and apply the background.
But I think this functionality is necessary not only to me. So I decided to add it to the core.

What about backgroundColor?: string. I did not think about that.
I would rather use style?: string to add an additional style to the element.

[gorkem]

Adding Vitaliy's demo of the new hosted development instance feature which shows the case https://youtu.be/5P5o0bHQlCo

[svenefftinge]

For that use case, vs code would change the background color of the full bar. We don't always need to do it exactly like VS code, but if we don't we should have good reasons. I don't see it in this case.

Regarding the use case, it seems like you are working on specific extensions to support developing plug-ins. Couldn't that be handled through the generic launch support you are working on as part of the debug adapter support? I would like to be able to launch a "development host" from command line, as well.

That said, I am fine with adding the style property. I just don't think it should be used lightly.

[gorkem]

Once the debug adapter support is in place it will work with the development host. I do not think development host from command line will make much sense as it stands today it is a fork of the running nodejs instance with additional plugins. @mmorhun has the details

That said, I am fine with adding the style property. I just don't think it should be used lightly.

I am looking forward to the "clown" status bar myself, that should get me points from my 5 year old

[vitaliy-guliy]

Another part of the issue is displaying the popup menu when clicking on the status bar element.
With the current architecture I need to set command ID (as string) when adding a status bar element. This command must be registered in CommandRegistry. This command can be found in the command palette by pressingCtrl+Shift+P and called.
Is it possible to add onclick(e: MouseEvent) property to SattusBarEntry and show the menu without using additional command?

        let entry = {
            text: `$(cog~spin) Hosted Plugin: Running`,
            alignment: StatusBarAlignment.LEFT,
            priority: 100,
            // additional style as string
            style: "hostedPluginRunning",
            // click handler to show a popup
            onclick: e => {
                this.showMenu(e.clientX, e.clientY);
            }
        };
        statusBar.setElement("entry-id", entry);

To test I temporary added onclick property in my code and it works fine.
Another problem with using a command to show the popup menu is determining cursor position and click target. Having onclick property we can pass mouse event to the handler directly.

[svenefftinge]

The general UX pattern here would be to make use a command and make use of the quick open.
But allowing a context menu for an item would be fine, too. But it should be activated on right click.

[vitaliy-guliy]

The pattern can be improved. In my case I display the menu by left clicking, not right.

[svenefftinge]

Actually, the reason why we don't allow handlers is that we store the entries as JSON between reloads.

[vitaliy-guliy]

I don't understand why do we need to store status bar state. I don't know a case which requires storing the status bar. I have already played a lot with it and I would say storing the state raises bugs and confuses the user/developer. For instance, your extension adds a status bar element and then after removing the extension your element will be present. It's only one example.
By the way, I have commented following line in my branch and everything is still working fine.

https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/shell/application-shell.ts#L514

[svenefftinge]

Yes, I agree. At the time of that decision it wasn't clear but now it seems as if all the important events are happening on load, such that the status bar is initialized properly. So I'm ok with removing the status bar state persistence and allow hooking up listeners.

[vitaliy-guliy]

Issue on disabling the status bar persistence #1859

[akosyakov]

In my case I need to add a status bar element with green background. Having it in green makes it more contrast. You can see my cases on attached screenshots.

What about reconsidering requirements and consider solutions preserving consistent look & feel of the status bar? For example to drop a req to color entries for now and use the quick open palette instead of the dialog to have behaviour matching other entries.

Sure, I can do it without changing Theia core. Just find theia-statusBar html element, then find an appropriate child and apply the background.
But I think this functionality is necessary not only to me. So I decided to add it to the core.

I don't think it is true. Look at other applications you will find rare cases. In the attached screenshot in the offline mode it does not make much sense to color entries: the offline mode is what is important. In the main Theia it would be better to color the whole status bar to show that something is running similar to VS code.

What about backgroundColor?: string. I did not think about that. I would rather use style?: string to add an additional style to the element.

It is questionable that we should allow such flexibility, removing it later won't be possible. It would be nice at least wait for real issues with the current approach.

[vitaliy-guliy]

I have a solution how to not add style property to the API but being able to customize status bar elements. We can set entry ID to the html element and then it become possible to use this id is css selectors.

[vitaliy-guliy]

@akosyakov @svenefftinge @spoenemann
I added class name field to set extra style for the elements.
Does it look well for now?

[svenefftinge]

There is still a DCO check failing, i.e. one of the commits doesn’t contain the sign-off information.

[svenefftinge]

And please don't merge master into branch, but rebase the changes onto master.

[vitaliy-guliy]

@svenefftinge Done with DCO and rebase.

[svenefftinge]

Thanks!