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

feat(ux/#2496): Configurable mode indicator #3391

Conversation

andr3h3nriqu3s11
Copy link
Contributor

Adds a way to configure the mode indicator position in the status bar.
Also added an entry in the docs for this configuration.

<breaking>configurealbe mode indicator position in the status bar</breaking>

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

This is awesome @andr3h3nriqu3s11 ! Works great:

2021-04-09 14 04 16

Thank you for adding the documentation for it, too 👍

@bryphe bryphe added B-license-key Bounty: License Key N-screenshot Notes: Screenshot labels Apr 9, 2021
@z0al
Copy link

z0al commented Apr 9, 2021

Thank you @andr3h3nriqu3s11 for implementing this 🙇🏻‍♂️

Since I mentioned that already in #2496 today, is feasible to make this a bit more generic? as a user, I tell Oni where to put the status bar items. I personally don't like the positioning of a few items currently in Oni and it makes it hard to switch back and forth between VS Code.

On the top of my head, I imagine something like:

{
  "workbench.statusBar.items": {
    "start": ["item1", "item2", "..."],
    "center": ["item3"],
    "end": ["...", "item4"]
  }
}

Note: The "center" section is added for the sake of completeness only. I don't really care about it

The ... is a special syntax to tell Oni where to put any other extra items. Omitting the ... means, I don't want more items to be added.

WDYT?

Thanks again for taking care of this, I really miss this feature.

</View>;
};
};

module Configuration = {
open Config.Schema;
let visible = setting("workbench.statusBar.visible", bool, ~default=true);
let modeIndicator =
setting("workbench.statusBar.modeIndicator", string, ~default="rigth");
Copy link

Choose a reason for hiding this comment

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

Suggested change
setting("workbench.statusBar.modeIndicator", string, ~default="rigth");
setting("workbench.statusBar.modeIndicator", string, ~default="right");

@andr3h3nriqu3s11
Copy link
Contributor Author

That's a great idea @z0al, and I am already working on it.

Although I found some problems:

  1. It's not as simple as "start", "center", "end". I had to create "start", "notification.start", "notification.center", "notification.end", "end", because there are items that supposed to be hidden by the notification popup...
  2. There are items that are automatically generated, both to the right and left right now I am grouping those under "rightGeneratedItems" and "leftGeneratedItems".
  3. I couldn't find a way to read the JSON objects without creating a decoder/encoder(but I am probably wrong, @bryphe please correct me if I am wrong), so right now I have the settings like (which is not really nice)
  "workbench.statusBar.items.start": ["notificationCount"],
  "workbench.statusBar.items.notification.start": ["notificationPopups", "macro", "leftGeneratedItems", "diagnosticCount", "scmItems"],
  "workbench.statusBar.items.notification.center": [],
  "workbench.statusBar.items.notification.end": ["rightGeneratedItems", "lineEndings", "indentation", "fileType", "position" ],
  "workbench.statusBar.items.end": ["modeIndicator"],

@z0al
Copy link

z0al commented Apr 10, 2021

That's a great idea @z0al, and I am already working on it.

You're a lifesaver 🎉 . Happy to help with whatever I can.

It's not as simple as "start", "center", "end". I had to create "start", "notification.start", "notification.center", "notification.end", "end", because there are items that supposed to be hidden by the notification popup...

I figured it wouldn't be that simple but I don't quite understand what kind of items are "supposed to be hidden by the notification popup." Would you be so kind to elaborate?

There are items that are automatically generated, both to the right and left right now I am grouping those under "rightGeneratedItems" and "leftGeneratedItems".

By these automatically generated items do you by extensions or some other source? In any case, that's what I had in mind when it comes to the ... notion. So

// Having "..." means place any items that I didn't mention in its position. Could be used for "leftGeneratedItems"?
"workbench.statusBar.items.start": ["modeIndiactor", "git", "..."]
// This means the notification count will always be at the very right, anything else, including "rightGeneratedItems" will be at the place of "..."
"workbench.statusBar.items.end": ["...", "notificationCount"]

I couldn't find a way to read the JSON objects without creating a decoder/encoder

I don't know, I guess people more familiar with the code would help here, but I don't mind the shape of the config itself as long as it does the job :)

Again, happy to jump in and help with whatever I can. It will be my first time coding in Reason/OCaml but I guess it shouldn't be super hard.

Also maybe this PR isn't the right place to discuss. Don't mind switching to the Discord server or in the referenced issue.

@bryphe
Copy link
Member

bryphe commented Apr 11, 2021

Thanks @andr3h3nriqu3s11 and @z0al for all the thinking around here. Sorry for the slow reply; Friday/weekend ended up being busier than I expected - I'll take a more in-depth look on Monday.

I like the idea of having a holistic solution that covers both the mode indicator and the general status bar configuration, very cool.

I figured it wouldn't be that simple but I don't quite understand what kind of items are "supposed to be hidden by the notification popup." Would you be so kind to elaborate?

Good question - I was curious here too. Once the notification is showing, we only show a small subset of the status bar - really just the notification count and mode, at the moment:

image

Basically - we have a couple of status bar UI elements that are hardcoded to show with the notification.

The positioning, in general, would be the same between the 'non-notification' display and notification display. So I wonder if we could streamline the syntax too:

  "workbench.statusBar.items.start": ["notificationPopups", "macro", "...", "diagnosticCount", "scmItems"],
  "workbench.statusBar.items.end": ["...", "lineEndings", "indentation", "fileType", "position", "modeIndicator" ],
  "workbench.statusBar.items.showInNotification": ["notificationPopups", "modeIndicator"],

The workbench.statusBar.items.showInNotification would be a list of IDs that should stay showing when a notification is active, and the positioning would be implied from workbench.statusBar.items.start/.end

EDIT: I just saw the discussion in #2496 - the workbench.statusBar.items.hideOnNotification looks good to me, it accomplishes the same goal 👍

(I like the idea of using ... to show the remaining / unprioritized items)

One open question I need to look into is if there is a good way to specify where extension-provided status bar items should live. I believe that we actually get the extension id when a status bar item is created, so that could mean we could do something like this:

...
  "workbench.statusBar.items.start": ["microsoft.vscode-typescript", "notificationPopups", "macro", "...", "diagnosticCount", "scmItems"], // Show typescript status bar stuff at the far left, always!
...

to override the positioning of the status bar items.

I'll take a look at this a bit more Monday and see if it's doable, that would be a neat addition. But even without that, the ... construct would help.

I couldn't find a way to read the JSON objects without creating a decoder/encoder(but I am probably wrong, @bryphe please correct me if I am wrong), so right now I have the settings like (which is not really nice)

Right, it's a little confusing! We have an obj decoder that can help here - some examples:

  1. obj(({field, _}) =>

(We'd probably use field.withDefault for most of these, instead of field.required, so that we could wire in defaults).

@z0al
Copy link

z0al commented Apr 11, 2021

The positioning, in general, would be the same between the 'non-notification' display and notification display. So I wonder if we could streamline the syntax too:

  "workbench.statusBar.items.start": ["notificationPopups", "macro", "...", "diagnosticCount", "scmItems"],
  "workbench.statusBar.items.end": ["...", "lineEndings", "indentation", "fileType", "position", "modeIndicator" ],
  "workbench.statusBar.items.showInNotification": ["notificationPopups", "modeIndicator"],

The workbench.statusBar.items.showInNotification would be a list of IDs that should stay showing when a notification is active, and the positioning would be implied from workbench.statusBar.items.start/.end

Yep, 💯 agree. And I think we are all convinced that the whitelisting approach (showInNotification) here makes more sense compared to hideOnNotification.

However, we still need another configure to decide the behavior of the notification. I see three cases:

  1. stretch : The notification width stretches to the available width . (close to this demo)
  2. full: The notification width stretches to the available width + highlights the full status bar background (compared to only its content in stretch or content compact) - This is basically what Oni does as of today
  3. content compact : takes as minimum space as possible (close to this demo

For that, I was thinking of having something like:

"workbench.statusBar.notification.mode": "full|stretch|compact"

I think that should be more than enough to handle all kinds of customizations.

One open question I need to look into is if there is a good way to specify where extension-provided status bar items should live. I believe that we actually get the extension id when a status bar item is created, so that could mean we could do something like this:

Are you referring to statusBarItem.id? Anyway, I think the ... syntax will include everything including the extension items. Essentially anything that's not specifically specified in start/end.

I copied code from @andr3h3nriqu3s11's branch and tried to implement the above points on this branch. Obviously, didn't do much since I was literally Googling every line hahah. I'm surprised it compiles. But here is the summary of the changes:

    let (itemsStart, itemsEnd) =
      getVisibleStatusBarItemIds(
        statusBar.items,
        startItemsConfig,
        endItemsConfig,
      );

    let itemsStart =
      itemsStart |> List.map(idToStatusBarElement) |> React.listToElement;

    let itemsEnd =
      itemsEnd |> List.map(idToStatusBarElement) |> React.listToElement;

    <View ?key style={Styles.view(background, yOffset)}>
      <section align=`FlexStart> itemsStart </section>
      <section align=`Center />
      <section align=`FlexEnd> itemsEnd </section>
    </View>;
  • There is no section specific to the notification items anymore.
  • getVisibleStatusBarItemIds takes statusBaritems + config and produces a list of startItemIds & endItemsIds (including the hard coded stuff e.g. modeIndicator ..etc). The code I have there is buggy though (Side note: Seriously, How to console.log in Ocaml? I'm trying to debug my code xD )
  • We then turn these into elements and render them.
  • (Not implemented) When using the showInNotification config we simply need to iterate one more time over the result of getVisibleStatusBarItemIds and only keep the IDs in showInNotification.

P.S. Sorry about the mess. When I suggested we continue the discussion on the referenced issue I thought this PR is going to be merged very soon and didn't think @andr3h3nriqu3s11 would work on the suggestions right away :D

@andr3h3nriqu3s11
Copy link
Contributor Author

andr3h3nriqu3s11 commented Apr 11, 2021

Right, it's a little confusing! We have an obj decoder that can help here - some examples:

Thank you, @bryphe I was able to do it! 😁

One open question I need to look into is if there is a good way to specify where extension-provided status bar items should live. I believe that we actually get the extension id when a status bar item is created, so that could mean we could do something like this:

That would be awesome, right now I did that filtering based on the Features_StatusBar.Item.t.id(this is probably wrong, I haven't really tested it...)

@andr3h3nriqu3s11
Copy link
Contributor Author

However, we still need another configure to decide the behavior of the notification. I see three cases:

I don't think this is necessary because you can control that by just putting the items you want on showOnNotification...
if you want the minimum space you just put that on

    let (itemsStart, itemsEnd) =
      getVisibleStatusBarItemIds(
        statusBar.items,
        startItemsConfig,
        endItemsConfig,
      );

    let itemsStart =
      itemsStart |> List.map(idToStatusBarElement) |> React.listToElement;

    let itemsEnd =
      itemsEnd |> List.map(idToStatusBarElement) |> React.listToElement;

    <View ?key style={Styles.view(background, yOffset)}>
      <section align=`FlexStart> itemsStart </section>
      <section align=`Center />
      <section align=`FlexEnd> itemsEnd </section>
    </View>;

Are you referring to statusBarItem.id? Anyway, I think the ... syntax will include everything including the extension items. Essentially anything that's not specifically specified in start/end.

I think this is not really a good idea because it could break how the people o created extensions, put their items on the bar...
Plus I think what @bryphe meant was to be able to chose one item and place it in one location

Well I did some work based on your ideas...

And it was not easy as looping one more time to add the notifications because the notification popup would look weird, also it was not simple as looping one more time because you have to that would leave the notification popup message weirdly where it could not display the message because the View that the message is in does not expand for the message to appear(this could lead to some weird situations), but I was able to do it...

Here is a demo, and it also shows kinda everything including that weird not expanding thing.
out

@z0al
Copy link

z0al commented Apr 12, 2021

I don't think this is necessary because you can control that by just putting the items you want on showOnNotification...

I will reply later after work on why I think showOnNotification is not sufficient in a few cases

@z0al
Copy link

z0al commented Apr 12, 2021

And it was not easy as looping one more time to add the notifications because the notification popup would look weird, also it was not simple as looping one more time because you have to that would leave the notification popup message weirdly where it could not display the message because the View that the message is in does not expand for the message to appear(this could lead to some weird situations), but I was able to do it...

Alright, below is how I see the abstract problem & the solution. I will leave what needs to be done code-wise for the experts (You both & other folks more familiar with the code). Thank you for taking the time to work on this btw.


TL;DR

  • We need to introduce items.left and items.right
  • We also need showInNotication & notificationMode if we want to continue to support the current notification behavior. Or the other way around, if we want to support compact mode (I want it, at least).

The core problem

As an Oni user, I want to be able to adjust how the editor status bar looks. In particular, I want to have the possibility to do the following:

  • Rearrange all items
  • Hide any item

The proposed solution

Expose new status bar configurations:

Note: I replaced the terms start & end with left and right because it aligns more with the way Extension authors set the alignment for items.

{
  "workbench.statusBar.items": {
    // List of item ids to display on the left side
    "left": [],
    // List of item ids to display on the right side
    "right": [],
    // List of item ids to hide always
    "hidden": []
  }
}

What is an item?

  • built-in e.g. modeIndicator OR
  • An extension item e.g. esbenp.prettier-vscode

Note: extensions don't always assign an ID for the status bar items they create (e.g. Prettier doesn't). In that case, VS Code assigns the extension ID as a value for the item ID. I don't know about Oni but we probably want to do the same.

Syntax notes:

  • Item Id must be known. Ignored otherwise.
  • An item that doesn't match an active item will be ignored. E.g. esbenp.prettier-vscode is only available in specific languages, in others it will be hidden by default.
  • It's possible to use ... syntax to refer to all items that are not explicitly defined in either: left, right or hidden configurations depends on the context.
    • if the ... appears only in the left configurations then it contains all items.
    • similarly, if the ... appears only in the right configurations then it contains all items.
    • if the ... appears in both left & right configurations then it will contain the items that have their alignment set to the matching value. E.g. ... in left configuration in this case will contain (both built-in & extension) items that have alignment===left.
  • Items placed via ... will always follow the default ordering rules.
  • The ... syntax is only supported in left & right configurations. E.g. You can't use it in hidden
  • items defined as hidden are never shown and are excluded from the ....

Usage Examples

These examples focus on left & right configurations since hidden is pretty straightforward.

1. Static items
{
  "workbench.statusBar.items": {
    // Show only mode indicator on the left side. No matter what.
    "left": ["modeIndicator"],
    // Show only the position on the left side. No matter what.
    "right": ["position"]
  }
}
2. Static items to the left and everything else goes to the right
{
  "workbench.statusBar.items": {
    // Show only mode indicator on the left side. No matter what.
    "left": ["modeIndicator"],
    // Everything else goes to the right but keep the notification count the last item.
    // The "..." here refers to: left & right aligned items by default (Including built-in items).
    "right": ["...", "notificationCount"]
  }
}
3. Show everything but rearranged
{
  "workbench.statusBar.items": {
    // Show mode indicator, notification then everything that's left aligned by default (Including built-in items).
    "left": ["modeIndicator", "notification", "..."],
    // The "..." here refers to: right aligned items by default (including built-in items).
    "right": ["...", "notificationCount"]
  }
}

The Challenge(s)

The Notification Popup

One thing that Oni does differently from other editors like VS Code is that it shows Notification Popus as a banner in the status bar itself. When shown, it hides all items except few specific ones.

I discussed the solution above assuming the Notification will be shown just like any other status bar item, literally. Its position can be determined by using the item notification (or notificationMessage whatever makes more sense). For example, using the following configs:

{
  "workbench.statusBar.items": {
    "left": ["modeIndicator", "notification"],
    "right": ["...", "notificationCount"]
  }
}

Should produce something like (notice that the notification ({..}) doesn't take up the full available width).

+-----------------------------------------------------------------------------------------------------------------+
| INSERT | { Hi, there. I will go away in 1..2..3. Okay, Bye } | <------- empty space -------> | ...items | | 🔔2 |
+-----------------------------------------------------------------------------------------------------------------+

Why I think this is better than the full-blown popup?

  • It helps the developer to focus on the message quickly. I'm no UX guy but having a full-width yellow box showing everything now and then is pretty distracting. On the other hand, taking only the needed space and highlighting (yellow background) the message itself means as a user, my eyes don't have to scan the full status bar to read the message.
  • I don't see something special about the current Notification pop to be shown that way. In a sense, any status bar item is a notification. For example, the Prettier status bar item is already showing me a "notification" by providing regular feedback on the status bar and reporting any errors. if I want to see all problems, there is Prettier panel for that.
  • Doesn't require introducing more configurations e.g. showInNotification (see below).
  • It gives the possibility in the future to have the message shown most of the time e.g. last message (doesn't necessarily mean it needs to be highlighted always, it will make sense for it to blink for a second and then use normal background). Which I'd be personally in favor of since it means I don't have to navigate around the Notification panel to see the last message in case I didn't catch it the first time.
  • If it really needs to be a popup then it maybe makes sense to make an actual popup that doesn't interfere with the status bar similar to VS Code (ofc, doesn't have to have the same style/location though).

But, I understand that such behavior may not be favorable by all users. There must be a reason the current behavior is the way it's, right?. So, to continue supportting that we need to have two more configurations.

{
  "workbench.statusBar.items": {
    // List of item Ids to be shown when a notification message is visible.
    // ONLY relevant when the mode is set to "default", and has no effect in "compact".
    "showInNotification": [],
    // Either "default" or "compact".
    "notificationMode": "default"
  }
}

Note: I could also imagine some people preferring a middle ground between the default and compact modes, as in, showing the message similar to compact but taking the available width on the status bar instead of the size of the message content. If this makes sense (now or in the future) we can add stretch mode to achieve that. I'm leaving it out for now since compact is what I'm looking for.

How it works?

  • The default mode indicates the current behavior: Showing the notification message as a full-blown banner that hides all the elements except a few (previously hardcoded values, now, whatever the user defines in showInNotification).
  • The positioning of the items defined in showInNotification is determined by items.left/right configurations. Nothing changed there.
  • Again, since the showInNotification doesn't determine the location of the items. Supporting ... doesn't make sense. Only explicit IDs should be allowed.
  • The compact mode is not affected by showInNotification and introduces the behavior that's discussed in the previous paragraphs. In short, treating the notification just like any status bar item taking the message width.

To achieve the current behavior of Oni, the default configurations can look something like this:

{
  "workbench.statusBar.items": {
    "left": ["notificationCount", "diagnosticCount", "scm", "notification", "..."],
    "right": [
      "...",
      "indentation",
      "language",
      "position",
      "modeIndicator"
    ],
    "showInNotification": [
      "notificaitonCount",
      "diagnosticCount",
      "modeIndicator"
    ],
    "notificationMode": "default",
    "hidden": []
  }
}

I hope I made what I want clear enough now. Looking forward to hearing what you think @bryphe and @andr3h3nriqu3s11

Edit: fixed some typos and JSON code

@andr3h3nriqu3s11
Copy link
Contributor Author

Love All the ideas from there I will start working on them 👍

And I finally understood what you meant by if there is no ... then the items extensions do not appear

Also, the ... getting all the items it's a good idea

And I think that a possible improvement on your proposal is instead of having a separated item for the notificationMode and showInNotification is to have them in one entry maybe notificationBehaviour or just notification and if it's a list then threat it as the showInNotification and if it's a string then we assume we are setting notificationMode

@bryphe
Copy link
Member

bryphe commented Apr 13, 2021

Thanks for the great summary / write-up @z0al and thank you @andr3h3nriqu3s11 for diving in and implementing it in #3402 - it looks like #3402 gets us really close.

@andr3h3nriqu3s11
Copy link
Contributor Author

This pull request no longer make sense because of #3402, so I am going to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-license-key Bounty: License Key N-screenshot Notes: Screenshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants