-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add chapter title to hovered time #93
Add chapter title to hovered time #93
Conversation
2790c04
to
11b01e1
Compare
Chapter ranges now work too. Took me a while to wrap my head around those. |
At first I wanted to make the change as unintrusive as possible to how to original code worked, but I think the second commit is much better. If you agree I'll squash it. |
fe6b699
to
5677e33
Compare
That's unavoidable with the character width estimation, happens with latin characters too. Unfortunately we don't know how wide the text really is, so the best we can do is estimate it. |
Although I do wonder... since it is possible to use this to get the exact bounds, maybe we could call that once for each string and store the result for future use. This wouldn't just be useful for hovered time and chapters, but also the width estimation in menus and I believe the title at the top also uses width estimation, so a more general solution that could be used in all those places would be useful. I'll look into it once all my PRs that touch anything to do with width estimation have been merged, otherwise it will just lead to a bunch of conflicts. |
That's sound diffcult. maybe better to make things simple, for now. |
The line Edit: Would you mind if I add that code to console.lua (and maybe other places in mpv) at some point? |
Sure, no problem. |
I meant a PR for this repo, because it's not specific to this change (although it does currently depend on it, so @darsain needs to be careful when merging). |
How about do this purely in ass tags, completely agnostic to the width of the text? Just do a bottom center alignment with There is no reason we should render chapter titles on a single line. And if there is a chapter title rendered, the time tooltip should also inherit the same edge limits, so that it is directly below the title. |
I don't know how I can get it to wrap. I've added the Edit: If i give it a really long line it ends up wrapping eventually, but it seems like it wraps based on the display width. How can i reduce the line width? |
Damn. Was hoping So now we need to wrap ourselves. Best place for that to happen is during chapters serialization. Add an additional Wrapping should happen at every space closest to the 25th character in the current line. Then in rendering we use Though I'm not 100% sure how to do wrapping for asian text without breaking it 😶. I'm going to merge the improved text width estimation PR so you can use it here. |
|
c7be234
to
e7a94b6
Compare
I've made an iterator out of the code from @natural-harmonia-gropius and used that for the text wrapping thing. I made the byte count for double width > 2 , because otherwise the russian text would look like this: The |
e7a94b6
to
49c141d
Compare
Looks good. The wrapping function does look a bit over-complicated, but I'm too lazy to come up with something myself :) The only nitpick I have, I'd like an extra space between hovered time and chapter title, otherwise it looks like it's just another line of the title. |
I prefer to render time and chapters separately,The time on two sides jumpping when width of chapter in timeline smaller than hover warpper. |
Hm... that's true, there will be an annoying jumping for chapters at the edges. Makes sense to decouple them then. |
49c141d
to
2d314cd
Compare
Time is once again separate from the chapter title and there is some extra spacing between them. Edit: I've looked into the word wrapping thing a bit and I don't really see a way of making it better. Everything I can think of that would make the code easier to read, would also make the algorithm less efficient (currently it passes over the string once, without any additional look ahead). |
Tried it out, and apart of hopefully the last two nitpicks below it works great! 1. It looks better when chapter title is forced bold, so just replace the optional 2. I found a title that wraps correctly, but the shorter line is returned as text width and used to calculate limits:
... or at least that's what I'm guessing is going on, since the upper line |
You're right, that string doesn't behave correctly and I already found the mistake. Was a pretty obvious one tbh, I'm surprised it never came up during testing. I'll squash the previous attempt because since nobody said anything I think we all agree that the second approach is better. |
Long titles are wrapped at a width of roughly 25 characters and the titles are always shown in bold with a small additional spacing between time and title. Closes tomasklaen#85
2d314cd
to
52d262f
Compare
👍 |
Thanks for the change, very cool. |
Closes #85