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

console.lua: update the select layout #15711

Merged
merged 9 commits into from
Feb 17, 2025
Merged

Conversation

guidocella
Copy link
Contributor

console.lua: improve the hovered item calculation with background-box

Follow up to c438732 which improved the calculation with the default outline-and-shadow. We can make the calculation accurate for background-box too without making semitransparent rectangle backgrounds overlap by adding margin between items.

We could calculate the height of each item to make them perfectly adjacent by rendering each one with compute_bounds, but that takes 15ms per render, so 20 lines would take 300ms, which is too slow.

Instead add a fixed 0.1 * opts.font_size to each item's height. This ensures the backgrounds don't overlap with tall CJK text or track circles and --osd-shadow-offset=0. Add this to outline-and-shadow too since it looks better with some margin between the items. Then add the rectangle offset to the height depending on the border style.

Copy link

github-actions bot commented Jan 19, 2025

Download the artifacts for this pull request:

Windows
macOS

@na-na-hi
Copy link
Contributor

Instead add a fixed 0.1 * opts.font_size to each item's height. This ensures the backgrounds don't overlap with tall CJK text or track circles and --osd-shadow-offset=0.

How do you ensure that the "tall characters" are at most 10% larger than normal characters, but not more?

@guidocella
Copy link
Contributor Author

You don't, it's a heuristic. It works with the fonts I tried and can be increased if it doesn't work with some others.

@na-na-hi
Copy link
Contributor

You don't, it's a heuristic. It works with the fonts I tried and can be increased if it doesn't work with some others.

"Works for me" isn't a good enough justification when it changes the appearance significantly:
select
It now looks very different from console, especially when the gaps and item background width don't even have the same size. No other menu-like UI do anything like this.

@guidocella
Copy link
Contributor Author

Well it was kasper's request to do it like this. I don't use background-box personally.

@kasper93
Copy link
Contributor

LGTM

@kasper93
Copy link
Contributor

You don't, it's a heuristic. It works with the fonts I tried and can be increased if it doesn't work with some others.

"Works for me" isn't a good enough justification when it changes the appearance significantly: select It now looks very different from console, especially when the gaps and item background width don't even have the same size. No other menu-like UI do anything like this.

It's is improvement over current status, feel free to come up with better ideas for this problem. @guidocella said that compute_bounds is too slow to work with, so we are quite limited in what we can do here. Otherwise we could match the spacing or draw background separately, but we would need a bounding box for this, which is apparently no-go.

@na-na-hi
Copy link
Contributor

It's is improvement over current status

It has appearance regression and not 100% improvement.

Otherwise we could match the spacing or draw background separately, but we would need a bounding box for this, which is apparently no-go.

Here is a bounding box that doesn't require compute_bounds:

You already know the total height of the box because you know the number of lines. The width can simply be the whole window width.

@kasper93
Copy link
Contributor

You already know the total height of the box because you know the number of lines. The width can simply be the whole window width.

It has appearance regression and not 100% improvement.

See, I find separate buttons with separate background just as nice.

We could compute bounds of longest string in selections, but it still means that we need to draw custom background and disable the style of default background, but I guess this can be done, wit tags for the events.

@guidocella
Copy link
Contributor Author

This looks fine to me too by making them look like separate buttons and not drawing more background width than necessary for each item. We will also add --osd-selected-back-color to give a different background color only to the selected item, this will be simpler if we don't have to draw ourselves. Also if we don't override libass' border style and libass/libass#688 is ever closed it will have rounded corners for free.

Computing the longest width is too slow with something like 10k properties. And if you compute the width only of current matches, the background width would change as you scroll.

@na-na-hi
Copy link
Contributor

It has appearance regression and not 100% improvement.

Which one has larger appearance change, especially compared to regular console?

See, I find separate buttons with separate background just as nice.
This looks fine to me too by making them look like separate buttons and not drawing more background width than necessary for each item.

It's not nice when they have different width. As I already said no other menus look like this.

We will also add --osd-selected-back-color to give a different background color only to the selected item, this will be simpler if we don't have to draw ourselves. Also if we don't override libass' border style and libass/libass#688 is ever closed it will have rounded corners for free.

I don't see how is this complicated. You draw the selected item background over the whole background. And if you really think rounded corners are important enough here (and not just bringing it up for the sake of argument), you can draw them already, there is no need to wait for libass.

Computing the longest width is too slow with something like 10k properties. And if you compute the width only of current matches, the background width would change as you scroll.

There is no problem if you make the background fixed width, like I originally mentioned.

I will end my remark by pointing out that the uosc playlist selection menu uses the drawing methods I mentioned and looks much better than this PR. It additionally calculates the longest item's width, but we don't have to do that and instead keep it a constant width to avoid width calculation.

@kasper93
Copy link
Contributor

It additionally calculates the longest item's width, but we don't have to do that and instead keep it a constant width to avoid width calculation.

I would prefer to not cover whole screen in background-box, just because we are too lazy to calculate the width of it. It will look obnoxious for short menu items.

@guidocella
Copy link
Contributor Author

guidocella commented Jan 30, 2025

Updated as request. It disables the item shadow with background-box and draws a rectangle as wide as the longest item (the longest calculation is not perfect with unicode and proportional fonts but works in practice). The rectangle coloring is actually handled by background-box automatically. Also the background width no longer changes as you type or scroll.

I don't see how is this complicated. You draw the selected item background over the whole background.

I meant that we now have to draw 3 rectangles to not make the semitransparent selected rectangle overlap with the default one, but that's fine.

@guidocella guidocella force-pushed the console-box branch 5 times, most recently from bb63a9b to fc6be30 Compare January 30, 2025 13:57
@kasper93
Copy link
Contributor

Doesn't work for me
image

@guidocella
Copy link
Contributor Author

It was just missing the closing ASS tag with non-nil font after I added the font. Also made it calculate the width even with outline-and-shadow so I can use it to position a scrollbar later.

@kasper93
Copy link
Contributor

Works.

@na-na-hi: is this solution acceptable to you?

@na-na-hi
Copy link
Contributor

na-na-hi commented Jan 31, 2025

Background looks OK, but the current width calculation is a little buggy and doesn't cover the whole width when it needs to. You can easily test with keepaspect-window=no.

Also because the default font is proportional, it causes text overflow especially with Unicode.

I would prefer to not cover whole screen in background-box, just because we are too lazy to calculate the width of it. It will look obnoxious for short menu items.

We can copy what uosc does if we don't want to be lazy: keep a length cache for items (including regular and bold styles) so that the bounds don't need to be recomputed while selecting/scrolling.

Alternatively we can always cover like 2/3 of the screen width and clip long lines.

@na-na-hi
Copy link
Contributor

To clarify my final thought: using a default rounded corner means a change in mpv's design language, which would be a commitment to implement rounded corner changes to at least OSD bar and background-box.

OSD bar is a part of the default user experience and is something in mpv's control, so that can be changed now. There will be no other suggestions from me for the rounded corner change. Feel free to ignore this and merge when it's ready otherwise if it's not considered a blocker.

@kasper93
Copy link
Contributor

OSD bar is a part of the default user experience

In my opinion OSD bar is not compatible with OSC. It is good to have if you use mpv without OSC, but it is already not compatible with interface.

@guidocella
Copy link
Contributor Author

I'd definitely like rounded corners on the OSD bar, even already suggested it in the previous PR, but it's work that needs to be done.

@na-na-hi
Copy link
Contributor

I already said it was kasper's specific request to do it like this and not my solution. I don't use background-box, so I didn't care that much to change it.

Have you evaluated yourself whether that request makes sense and whether if there are better solutions for it? I don't think blindly taking requests without evaluation is a good idea. Otherwise mpv will become even a bigger mess because everyone's ideas can go in no matter how good or bad they are.

What? I did come up with using clip.

Not before I mentioned uosc in the PR. Otherwise, the text overflow issue should be fixed already before the layout change.

What again? I did come up caching the width of the longest line. It's your solution of caching multiple lines as you scroll which was broken because the menu's width would change as you scroll.

As mentioned several times before it's not the longest display width. And menu width changing literally isn't a problem for now because the status quo has the same problem, so no regression. Again, I have never asked the menu to be center aligned.

Yeah because it's masters of design that suggest rounded corners on a bar aligned to a corner. I literally linked a tweet from a designer which influenced the decision to not add borders between the items or around the menu and to distantiate them.

It's likely that you followed that tweet just because borderless "looks nice", because you have not taken a hint from other points outlined in that tweet, all of which enhance usability and readability. Where is the different background color for search bar? Where are the extra paddings? I noticed both of these issues immediately after the new layout was first posted, long before you posted that tweet. I didn't mention it because there were other more important issues need to be addressed first.

And I changed it as soon as kasper agreed, unlike you who insisted on square corners after 4 other people preferred them rounded.

Stop going "kasper this, kasper that". Evaluate suggestions on their own merit. There is no need for his agreement to see why that suggestion makes sense.

If it looks nice to everyone to you that's a perfectly good reason to do it. Changes everyone likes don't have to be justified to you.

Those who dislike it often don't show up immediately. It has happened countless of times before that there really isn't any consensus when disagreements show up only after the changes are merged and shoved down to their throats.

As if you gave more detailed reasons against the inital layout other than "it's not nice".

Consistency/familiarity was the reason mentioned. That screenshot by itself should be apparent.

@na-na-hi
Copy link
Contributor

In my opinion OSD bar is not compatible with OSC. It is good to have if you use mpv without OSC, but it is already not compatible with interface.

IMO the same can be said for other OSD text because they use borders instead of a background by default, like OSC.

The OSD bar also needs a redesign to fit into the OSC design theme.

@guidocella
Copy link
Contributor Author

Have you evaluated yourself whether that request makes sense and whether if there are better solutions for it? I don't think blindly taking requests without evaluation is a good idea. Otherwise mpv will become even a bigger mess because everyone's ideas can go in no matter how good or bad they are.

I don't decide which ideas get in, evaluation is already what PRs are for, and I don't personally care about background-box, those who use it can argue it better which is exactly what happened.

Not before I mentioned uosc in the PR. Otherwise, the text overflow issue should be fixed already before the layout change.

Well duh? There was no custom background drawing before you mentioned uosc, so no need for clipping, which I added along with center alignment.

As mentioned several times before it's not the longest display width. And menu width changing literally isn't a problem for now because the status quo has the same problem, so no regression. Again, I have never asked the menu to be center aligned.

It's only the status quo if one was using the non-default background-box. It looked bad to someone like me not used to it.

It's likely that you followed that tweet just because borderless "looks nice"

I followed all of his advices since 2017, they are gold.

because you have not taken a hint from other points outlined in that tweet, all of which enhance usability and readability. Where is the different background color for search bar?

Not being implemented yet doesn't mean I haven't considered the hint. This PR is already huge without more additions, and it is not as simple as doing it in a web page when font and background colors are configurable which can make the input line look mismatched or decrease readability.

Where are the extra paddings?

In the opts.font_size * 1.1 factor present since the first commit.

Stop going "kasper this, kasper that". Evaluate suggestions on their own merit. There is no need for his agreement to see why that suggestion makes sense.

Disagreeing is an evaluation. It is normal to be skeptical until more people agree.

Those who dislike it often don't show up immediately. It has happened countless of times before that there really isn't any consensus when disagreements show up only after the changes are merged and shoved down to their throats.

No idea what the point here is as there will obviously people disliking whatever we do and whether it has good reasons or not. I've seen e.g. users complaining that left clicking track buttons should show a menu which was reverted. The majority of developers' preferences is the best guess of what is better.

@guidocella guidocella force-pushed the console-box branch 3 times, most recently from 78dbba7 to afbf7b3 Compare February 12, 2025 20:55
Add a scrollbar to give a visual representation of the size and position
in the items. It is not clickable.

The counter is moved above the scrollbar to free up a line for an
another item, simplify calculations, and prevent the selected item from
jumping from the second to the first line as you type.

In terminal output there is no easy way to draw a scrollbar to the right
due to Unicode widths. This just moves the counter into the prompt.
Draw a background behind selectable items even with outline-and-shadow.
This makes sense for a menu because you want it to be readable, select
something and move on. It doesn't stay open while watching like OSD
messages and subtitles, so it can cover more of the video. In fact this
was probably the only menu without a background by default. Also the
scrollbar without a background looked weird, and the background shows
the new horizontal hit box.

--osd-outline-color determines the background color with
outline-and-shadow, while --osd-back-color determines the background
color with background-box. Some transparency is added because using pure
black is not recommended because it causes eye strain; alternatively
--osd-outline-color could default to #222222.

Drawing the background ourselves also allows making the corners rounded.

Free-form text mode keeps using only background-box backgrounds if
configured as covering the whole screen while searching stats key
bindings would be bad. Searching history also doesn't add a background
to not change the layout abruptly. When searching history with
background-box, it preserves the alpha component of --osd-back-color.
This reverts commit ad0c29e.

This should be unnecessary now that the menu has a visible horizontal
and vertical hitbox, and was not discoverable anyway. Right click can be
useful even while the console is open to pause or open a context menu.
Requested in
mpv-player#15145 (comment).
The default item also has the same background color but with
transparency.

Also stop bolding selections since inverted black and white backgrounds
should be visible even with color blindness. It was annoying with
proportional fonts because it misalignes similar strings.

As mpv's default text colors are white on black border or background,
--osd-selected-color's default of a bright yellow meant to be used with
a black border becomes unreadable with the inverted white background.

We could default to a dark --osd-selected-color and a a light
--osd-selected-outline-color and use --osd-selected-outline-color as the
selected back color. However in show-text commands having only the
selected item with a different white border doesn't look good.

This therefore adds indipendent selected_color and selected_back_color
script-opts. --osd-selected-color is only used for completions and for
the selected item when searching the command history with
outline-and-shadow.
It's fine to call them menus now that they actually look like ones.
Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

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

I think all the actual issues were ironed out by now.

@sfan5 sfan5 added this pull request to the merge queue Feb 17, 2025
Merged via the queue into mpv-player:master with commit 49a578f Feb 17, 2025
27 checks passed
@dyphire
Copy link
Contributor

dyphire commented Feb 22, 2025

Although the new select-menu is more beautiful, there are some problems:

The background color of the current active item is difficult to identify, it should be more eye-catching, and its font color and background color should also provide customizable options: active_color and active_back_color
image
The same indistinguishable problem exists with the track's indicators
image

@guidocella
Copy link
Contributor Author

Well I would agree the default item background could use a bit more opacity, I had it before but kasper asked to decrease it. But I think adding unique colors to it would be annoying because that's 2 extra things you have to configure when you change the colors compared to reusing --osd-color and selected_back_color.

@dyphire
Copy link
Contributor

dyphire commented Feb 22, 2025

Well I would agree the default item background could use a bit more opacity, I had it before but kasper asked to decrease it. But I think adding unique colors to it would be annoying because that's 2 extra things you have to configure when you change the colors compared to reusing --osd-color and selected_back_color.

I think that as long as the outstanding effect of the activity item can be improved, the additional customizations are no longer necessary.

@kasper93
Copy link
Contributor

I have not experiment with it myself to dial perfect balance between current item highlight and selected item highlight. I think the current item (under mouse pointer) should be more visible, because this is what user want to chose. Selected item otoh is just an information. Both are important, but if they have the same visual impact it makes the list bloated and not readable in the sense, that user cannot differentiate between highlights.

That's my opinion. Also remember that initial implementation was not readable as we can see in #15711 (review) . Hence why it was reduced to make the contrast better.

I don't mind improving this, but we need to keep in mind to keep it visually pleasing and readable at the same time.

@guidocella
Copy link
Contributor Author

Yeah the initial one was unreadable but we then increased the alpha further from cc to dd, I think cc or slightly lower would be better.

@dyphire
Copy link
Contributor

dyphire commented Feb 22, 2025

I think it's a good idea to look at uosc's handling of this, it does a good job of it.
image
We can see that uosc does a good job of highlighting the active and selectable items. Although the select-menu displays the two in the opposite way, its opacity and the processing of adding vertical line marks can be referenced.

guidocella added a commit to guidocella/mpv that referenced this pull request Feb 22, 2025
This needs to strike a fine balance between being visible and having
contrast with the text color and not taking attention away from the
selected item. Make it slightly more opaque to differentiate it more
easily.

Fixes mpv-player#15711 (comment)
guidocella added a commit to guidocella/mpv that referenced this pull request Feb 24, 2025
This needs to strike a fine balance between being visible and having
contrast with the text color and not taking attention away from the
selected item. Make it slightly more opaque to differentiate it more
easily.

Fixes mpv-player#15711.
kasper93 pushed a commit that referenced this pull request Feb 24, 2025
This needs to strike a fine balance between being visible and having
contrast with the text color and not taking attention away from the
selected item. Make it slightly more opaque to differentiate it more
easily.

Fixes #15711.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants