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: show suggestions for completion #10316

Merged
merged 5 commits into from
Oct 7, 2023

Conversation

christoph-heinrich
Copy link
Contributor

@christoph-heinrich christoph-heinrich commented Jun 20, 2022

Tab completion now temporarily shows a colored table of all potential completions
between the log messages and prompt.
The table automatically adjusts its column count depending on the space available.

As there is no way of knowing the exact width in characters, this is an estimate
and can be adjusted with the new option font_hw_ratio.
I expect the default value to work for everyone who doesn't set their own font.

The spacing between the columns also adjusts automatically, depending on
how much space is left.

suggestions_screenshot

@christoph-heinrich
Copy link
Contributor Author

Refactored it a bit, the only new line going over 80 characters is now 272 and I think that can stay that way.
This conflicts with #10315 because both add a new option at the same place.

@christoph-heinrich
Copy link
Contributor Author

Had to rebase due to conflicts.

@haasn
Copy link
Member

haasn commented Nov 3, 2022

image

On my end it looks misaligned, apparently when completing something with - as part of the completion string.

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Nov 3, 2022

No idea why that happens for you. - is treated just like every other character. They aren't in alphabetical order
image

Edit: Actually I have some idea. When looking at the order of your entries, it's like you actually have 4 columns, but it wraps around after 2.
image
Do you have hidpi-scaling or something?

@haasn
Copy link
Member

haasn commented Nov 3, 2022

Do you have hidpi-scaling or something?

I don't know. How would I check?

@christoph-heinrich
Copy link
Contributor Author

print-text ${display-hidpi-scale}

I'm currently rebasing on master and looking at the code it should already handle scaling 🤔

@avih
Copy link
Member

avih commented Nov 3, 2022

I'm guessing he's not using monospace font.

EDIT: but it does look monospace...

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Nov 3, 2022

It looks like a monospace font and everything lines up perfectly when you consider wrapping exactly at the point where the 3rd column would begin.

Edit: I've now tested scaling and it works fine for me, so there must be some other problem...
You can try setting font_hw_ratio to 1. I don't understand why that would be so much different for you (especially because the font doesn't look super wide), but that should help.

Edit2: It does look like that for me when I set font_hw_ratio to 4.
image

@haasn
Copy link
Member

haasn commented Nov 3, 2022

print-text ${display-hidpi-scale}

I'm currently rebasing on master and looking at the code it should already handle scaling thinking

That prints 1.000 for me.

@christoph-heinrich
Copy link
Contributor Author

The table formatting supports utf8 encoding now. That shouldn't really make a difference because currently all commands and properties use ascii characters, but that will avoid problems in case that changes at some point.

@haasn I can only replicate how it looks for you by doubling font_hw_ratio, so you should be able to fix it by halving yours. No idea why that's necessary for you.

@avih Does the default value work for you?

@avih
Copy link
Member

avih commented Nov 3, 2022

@avih Does the default value work for you?

I didn't try it.

@garoto
Copy link
Contributor

garoto commented Feb 10, 2023

I imagine you've contemplated this before, but how much work would be needed for the script to cycle between all possible completions with every TAB keypress?

@christoph-heinrich
Copy link
Contributor Author

I'd expect that to be pretty hard to do, sure would be nice though.
It's out of the scope of this PR, but maybe someone will make it happen after this gets in.

@christoph-heinrich
Copy link
Contributor Author

The rows are now from the bottom up instead of from the top down (first row at the bottom), that's better because the last column isn't always full, and so a when the table gets cut of at the top, less suggestions get cut off.

It's also a little bit simpler and faster. Especially noticeable (when resizing the window) was precalculating the utf8 lengths instead of always calculating them on the fly.

I've also tried filling the table column-wise instead of row-wise so that all but the last row is completely filled up, but as it turns out that often results in long suggestions being next to each other instead of on top of each other, resulting in wider columns. Wider columns lead to less columns fitting on the screen, increasing the total row count and thus fewer visible suggestions (when there are enough that they get cut off at the top).

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Jul 21, 2023

@haasn does this still not work correctly for you? It has worked fine on all setups I've tried it on.

The text styles are now in a table.
The color definitions of the theme where the colors were taken from
are now included in a comment for future reference.
The colors have been converted to BGR as is required by ASS.
@github-actions
Copy link

Download the artifacts for this pull request:

Windows

Tab completion now shows a list of all potential completions
between the log messages and prompt.
The text is colored to differentiate it from regular text.
The color matches the theme and is similar to the mpv logo.
@Dudemanguy
Copy link
Member

Dudemanguy commented Oct 7, 2023

So now that I finally try this, I also seem to have the same issue as haasn with misaligned elements.

2023-10-07-003252_415x66_scrot

Setting font_hw_ratio to random values doesn't really make any difference for me. It's still misaligned. I also don't really care though. This is useful functionality.

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Oct 7, 2023

So now that I finally try this, I also seem to have the same issue as haasn with misaligned elements.

You're using a proportional font, that's why this simple width estimation doesn't work all that well.

Alternatively we could use a more sophisticated width estimation method that I made for uosc. It uses compute_bounds to get the character widths, but it's close to 400 lines of code, so not sure that's worth it here.

Edit: Actually we could make a separate ass event for each column, so their widths don't directly influence each other, but then there will be potential problems with text overlapping. Maybe I'll give that idea a try in the future, but that doesn't have to be part of this PR.

@guidocella
Copy link
Contributor

You can maybe clarify "This has to be a monospaced font for the completion suggestions to be aligned correctly.". I also plan to keep using sans-serif because it looks better and I don't mind if the completions aren't aligned perfectly, and they will often fit in 1 line anyway.

@Dudemanguy
Copy link
Member

If it wasn't for the discussion already in this thread, I wouldn't have even expected the elements to line up.

Completion suggestions are now nicely formatted into a table.
Maximum width of the table is estimated based on OSD size and
font size.
This requires a new scaling factor option `font_hw_ratio`.
A factor of 2.15 works great for me,
but the default is 2.0 to avoid problems with other fonts.
The space between columns is automatically adjusted to be
between 2 and 8 spaces.
It tries to use as few rows as possible.
Lines that would be outside of the visible area are now culled.
The log messages were already culled, however with the introduction
of suggestions, they also needed a small update.
@christoph-heinrich
Copy link
Contributor Author

You can maybe clarify "This has to be a monospaced font for the completion suggestions to be aligned correctly.".

Done.

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.

LGTM. See no reason to leave this in limbo.

@Dudemanguy Dudemanguy merged commit e475e1a into mpv-player:master Oct 7, 2023
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.

6 participants