Skip to content

refactor: tooltip positioning logic#543

Merged
Samillion merged 2 commits intoSamillion:mainfrom
nekoxuee:refactor/tooltip-positioning
Mar 15, 2026
Merged

refactor: tooltip positioning logic#543
Samillion merged 2 commits intoSamillion:mainfrom
nekoxuee:refactor/tooltip-positioning

Conversation

@nekoxuee
Copy link
Copy Markdown
Contributor

Improved tooltip positioning. Adds estimate_text_width() function using mpv's osd overlay to calculate actual text widths, enabling proper horizontal boundary clamping so tooltips stay within screen edges.

fixes: #541

@nekoxuee
Copy link
Copy Markdown
Contributor Author

So I’ve been trying combinations to see how it worked before.

It seems the current hovered chapter replaces either the title or the active chapter text, unless both are disabled, in which case it finally falls back to the tooltip.

This replace the title approach was a result mpv stock osc design limitations (thick progress bar where the tooltip is displayed inside), and because early versions of ModernX didn't display chapter names either, this code was adopted.

However, when po5 added thumbnail support to ModernX, they implemented chapter display as a proper tooltip. Even in the thumbnail version of the stock osc, the title replacement behaviour was kept rather than using the tooltip, likely due to the stock layout.

Now that we have a proper tooltip system, the title replacement feels redundant and inconsistent from a UX perspective. So, do we want to keep this legacy behaviour, or move to a tooltip only approach?

@Keith94
Copy link
Copy Markdown
Contributor

Keith94 commented Mar 14, 2026

Now that we have a proper tooltip system, the title replacement feels redundant and inconsistent from a UX perspective. So, do we want to keep this legacy behaviour, or move to a tooltip only approach?

Tooltip only seems way better. It's much easier to follow the tooltip to see content changes compared to the OSC title where it's just a static element.

@Samillion
Copy link
Copy Markdown
Owner

Agreed. Though both should remain, since we have tooltips disable/enable option. This way, no one will be upset.

Let me know when it's ready to merge.

@Keith94
Copy link
Copy Markdown
Contributor

Keith94 commented Mar 15, 2026

Agreed. Though both should remain, since we have tooltips disable/enable option. This way, no one will be upset.

What option do you mean? tooltip_hints? That doesn't affect the seekbar tooltip because it's a critical element.

@Samillion
Copy link
Copy Markdown
Owner

True! Completely forgot about that.

Yup, leaving this up to you guys. My brain is extra pretending to exist today lol. Let me know when it's ready to merge, please.

@nekoxuee
Copy link
Copy Markdown
Contributor Author

nekoxuee commented Mar 15, 2026

Since the seek bar tooltip is always active, and the old title replacement logic was suppressing it based on unrelated user options. I'm removing the replacement code to keep it tooltip only.

Ready to merge.

@Samillion
Copy link
Copy Markdown
Owner

Beautiful. Thank you very much.

Merging.

@Samillion Samillion merged commit 0b16b2e into Samillion:main Mar 15, 2026
@nekoxuee nekoxuee deleted the refactor/tooltip-positioning branch March 15, 2026 16:40
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.

Tooltip runs out of bounds

3 participants