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

osc.lua: open select.lua by clicking buttons #15031

Merged
merged 8 commits into from
Oct 22, 2024

Conversation

guidocella
Copy link
Contributor

@guidocella guidocella commented Oct 8, 2024

8bf5548 added mouse support to selectors, so open them from OSC buttons.

It's up for debate where middle or right click should be bound. By binding middle click show_message(get_tracklist()) is no longer bound. By binding right click, it no longers cycles tracks backwards, though scrolling with the wheel still does.

Closes #5067, closes #6291, closes #10621, closes #11878.

Copy link

github-actions bot commented Oct 8, 2024

Download the artifacts for this pull request:

Windows
macOS

@Samillion
Copy link
Contributor

Samillion commented Oct 8, 2024

I think right now choosing middle button is the right way, since it would be considered a sudden change.

It's very unfortunate, but many are not aware of select.lua, even when I tried to explain it, there is hesitation (fear of change), but once they see it in action, they never go back. Hopefully this gives it the attention it deserves.

In time, can easily switch it to right click. Hell, lazy future scenario, make it a user_opts. :D

Edit:
For me though, you know my take. I'm planning to make select the main use on my OSC.

Left, right and middle.

@guidocella guidocella force-pushed the osc-select branch 3 times, most recently from 36032f5 to 51af459 Compare October 8, 2024 21:17
@kasper93
Copy link
Contributor

kasper93 commented Oct 9, 2024

Now that I think about it, mbtn_mid is not that discoverable, especially on mobile devices, like laptops. But making breaking changes for OSC, like changing mbtn_right might not be welcome by users, but maybe it is better thing to do?

@Samillion
Copy link
Contributor

It's a tough decision to make, any choice will have its downsides and caveats.

Getting it out there in a mid safe way is honestly a wise choice. Then wait for feedback and hear the dominating request, or more realistically, once it's actively used, the "choice" will become obvious.

Because, and I'm assuming here, this will be a solid replacement for get_*list() or show-text *list in the future.

If anything, this is a temporary confusion of a choice.

@guidocella
Copy link
Contributor Author

Rebinding mbtn_mid is still a breaking change, and changing defaults multiple times will annoy users more. Going to next/prev playlist entry/chapter is useful, but we could bind select to mbtn_left on the title where left click is less useful and move the current show-text function to mbtn_mid, and on tracks where seeing what you're selecting beforehand is more useful lthan with playlist and chapter.

@na-na-hi
Copy link
Contributor

na-na-hi commented Oct 9, 2024

It's up for debate where middle or right click should be bound.

Personally, I'm used to using the middle button to show playlist and available tracks. The new behavior is also annoying that after the middle click brings up the console, I can't dismiss it without using the keyboard. Previously the list will disappear by itself after a few seconds. And the playlist selector doesn't show the current item: it only highlights initially, and it changes with the slightest mouse movement.

I think it's better to make these bindings user configurable instead. There are too much hard coding in OSC in this regard.

@guidocella
Copy link
Contributor Author

I can bind MBTN_RIGHT to closing the console or print circles in the playlist selector (this was requested before in #14087 (comment)) if desired.

Making everything configurable would be tricky since most clicks call internal functions rather than string commands and there are dozens of handlers.

@na-na-hi
Copy link
Contributor

na-na-hi commented Oct 9, 2024

I can bind MBTN_RIGHT to closing the console or print circles in the playlist selector (this was requested before in #14087 (comment)) if desired.

Sounds reasonable.

Making everything configurable would be tricky since most clicks call internal functions rather than string commands and there are dozens of handlers.

Lots of them already have associated script messages which can be invoked with script-message-to. In fact the replaced functionalities here are already covered by osc-chapterlist, osc-playlist, and osc-tracklist script messages. You can make these open_selector actions as script messages too.

@kasper93
Copy link
Contributor

kasper93 commented Oct 9, 2024

Rebinding mbtn_mid is still a breaking change

I guess when changing UI code we cannot avoid changes. Else we will keep the same osc.lua forever.

@Samillion
Copy link
Contributor

Samillion commented Oct 9, 2024

I guess when changing UI code we cannot avoid changes. Else we will keep the same osc.lua forever.

Hear, hear. Embrace it. This is the same hesitation I was talking about when users fear a new change, select.lua is a far superior function to what we have now, which is a simple show-text essentially. It turns the whole feature to be interactive.

In fact, the only thing that would stop this from being "main" from the get-go, is the font size scale to window issue. I simply added a note about it.

@guidocella
Copy link
Contributor Author

Lots of them already have associated script messages which can be invoked with script-message-to. In fact the replaced functionalities here are already covered by osc-chapterlist, osc-playlist, and osc-tracklist script messages. You can make these open_selector actions as script messages too.

I think we can add osc-hide and bind script-binding select/select-playlist; script-message-to osc osc-hide and so on. But in general I never understood why OSC has custom printing for playlists and chapters instead of just calling show-text (at least for the tracks it only prints tracks of that type). I see now that it even has custom printing after changing track in set_track (??) I'd rather just let the OSD show the text of cycle sid than to convert that to a script message as well. The track count can be added to that message.

In fact, the only thing that would stop this from being "main" from the get-go, is the font size scale to window issue. I simply added a note about it.

I can reopen #14088 if requested.

@kasper93
Copy link
Contributor

kasper93 commented Oct 9, 2024

But in general I never understood why OSC has custom printing for playlists and chapters

Remove it, if you don't like it.

guidocella added a commit to guidocella/mpv that referenced this pull request Oct 9, 2024
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
@guidocella guidocella mentioned this pull request Oct 9, 2024
Copy link
Contributor

@kasper93 kasper93 left a comment

Choose a reason for hiding this comment

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

Works, as expected.

@guidocella
Copy link
Contributor Author

I thought we were going to make bindings configurable first?

@kasper93
Copy link
Contributor

kasper93 commented Oct 9, 2024

I thought we were going to make bindings configurable first?

Ok, if you prefer to do this in this pr.

guidocella added a commit to guidocella/mpv that referenced this pull request Oct 9, 2024
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
guidocella added a commit to guidocella/mpv that referenced this pull request Oct 9, 2024
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
guidocella added a commit to guidocella/mpv that referenced this pull request Oct 9, 2024
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
Samillion added a commit to Samillion/ModernZ that referenced this pull request Oct 9, 2024
After a discussion at mpv that started with this:
mpv-player/mpv#15016

It has lead to a complete removal of redundant functions that mpv is already capable of doing from the osc.

References:
mpv-player/mpv#15031
mpv-player/mpv#15038

Also, increased title max character limit to fix #9
guidocella added a commit to guidocella/mpv that referenced this pull request Oct 10, 2024
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
guidocella added a commit to guidocella/mpv that referenced this pull request Oct 11, 2024
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
kasper93 pushed a commit that referenced this pull request Oct 14, 2024
Requested in
#14087 (comment) and
#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
mp.command("script-binding select/select-" .. type)

if user_opts.visibility == "auto" then
osc_visible(false)
Copy link
Contributor

@Samillion Samillion Oct 15, 2024

Choose a reason for hiding this comment

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

This is merely an opinion that might match the use case for you guys:

On my OSC fork, I'm hiding the osc osc_visible(false) without checking if user_opts.visibility == auto

Reason:
Like we talked before, the OSC is at the bottom and select/console also renders at the bottom. I instantly got reports that OSC interferes (ie: hovering over area)

Thought I'd let you know, just in case. Since it would make sense to hide osc on open_selector regardless of visibility mode, because you want the focus to be on select once it is triggered without interference until ESC/leave select.

local function open_selector(type)
    mp.command("script-binding select/select-" .. type)
    state.selector = true
    osc_visible(false)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that what the osc/margins user_property is for? To allow console/select to draw text above the OSC?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed it is, just thought I'd share it. I've opted for completely hiding osc while selector is on.

Like I said, merely an opinion that might match your use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSC with visibility always does not interfere because the console is rendered above it so I don't understand your point. If users intentionally make the OSC always visible why should we override their preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 To each their own, just sharing an opinion

This will be used to hide it when opening select.lua.
@guidocella
Copy link
Contributor Author

Done, but you can still cycle tracks with the wheel.

@guidocella
Copy link
Contributor Author

Right click on sub tracks could be bound to select a secondary subtitle but I don't know how many use those.

@llyyr
Copy link
Contributor

llyyr commented Oct 21, 2024

Some preliminary thoughts:

  • I don't like that select.lua is bound to left click for some elements and right click for other elements. I think it'd be better to be consistent and bind it to right click for all elements. This means current title, and audio/subtitle track buttons should activate select.lua with right click. Addressed in IRC.

  • I don't see much value in listing available tracks with right click on audio/subtitle tracks. As mentioned above, I think select.lua should activate with right-click (and as such it'll list all the tracks anyway), and the left click behavior from master should be kept as-is. Addressed in IRC.

  • I don't know how hard it would be to implement this as I'm not familiar with input.lua/select.lua, but clicking outside the select.lua region should cancel the current action. Currently this is only possible with ESC afaiu. I'd be very hesitant to merge this if there's no mouse-only way to cancel the current select.lua action. Partially addressed by console.lua: exit when left clicking outside of selectable items #15145 which I'm fine with.

@guidocella guidocella force-pushed the osc-select branch 3 times, most recently from 717605a to cdf3ae8 Compare October 22, 2024 18:15
@kasper93
Copy link
Contributor

Works fine for me, I'm sure there are details that we can bikesheed, but there is not much interest in this. So I'm inclined to merge it to gather more feedback and we can work iteratively on this.

I'd be very hesitant to merge this if there's no mouse-only way to cancel the current select.lua action. Partially addressed by #15145 which I'm fine with.

there is also a right click that cancels select.

@guidocella: what do you think about adding click action on cache element? Could be bind to stats.lua page 3.

@guidocella guidocella force-pushed the osc-select branch 2 times, most recently from 67c809d to 9d2fd93 Compare October 22, 2024 18:34
@guidocella
Copy link
Contributor Author

guidocella commented Oct 22, 2024

Showing cache stats is cute but I'm not sure it would be useful if the cache seconds and the cached regions are already shown?

@Samillion
Copy link
Contributor

I'd be very hesitant to merge this if there's no mouse-only way to cancel the current select.lua action.

I was going to suggest to render controls similar to window controls with a [ X ] Close, prepended, always top.

I'm not sure how viable that would be, especially because you can predict that once this becomes more popular, one of requests would be opts for background-box/opacity for visibility in console/select and maybe even stats.

Poor Guido has been flooded with notes lately xD, though it's probably because it's a great addition

@guidocella
Copy link
Contributor Author

Well you can now close by clicking anywhere on the top line so an X would only be an indication.

You can already give the console and stats a background with --osd-border-style=background-box.

@Samillion
Copy link
Contributor

Samillion commented Oct 22, 2024

You can already give the console and stats a background with --osd-border-style=background-box.

Yup, one of the first things I've added for osd since I started using select/console a lot.

osd-level=1
osd-font-size=32
osd-shadow-offset=4
osd-back-color=0.0/0.0/0.0/0.50
osd-border-style=background-box
osd-playing-msg="${!playlist-count==1:${playlist-pos-1}/${playlist-count}}"

What I meant was, they'll request it specifically and only for console/select. Though this is me overthinking things yet again.

Now that the playlist counter is shown by default there is no use in
showing it again, so repurpose the binding to show the stats page with
file and track info.
8bf5548 added mouse support to the console's select menu, so open it
from OSC buttons.

Left click on the title opens the playlist selector and showing the
stats is moved to right click.

Right click on chapter buttons opens the chapter selector.

Left click on track buttons opens the track selectors because seeing
which track you're selecting beforehand is useful.

Fixes mpv-player#10621.
This makes it consistent with most other bindings and lets you cancel
the operation by moving the cursor away (the seekbar bind mouse down so
you can keep seeking to different timestamps).
Give buttons nicer names before exposing script-opts for their bindings.
In particular, rename cy_audio and cy_sub to audio_track and sub_track
because their left click binding no longer cycles.
There is a consensus that showing these list when navigating is
distracting. Also if you set --osd-playing-msg, the playlist is briefly
displayed and then overwritten by the msg. The chapter list can even
contain spoilers:
mpv-player#4675 (comment)

The next commit will remove these completely in favoring of making the
commands customizable.
This adds several script-opts to configure what OSC buttons do when
clicked. It lets you restore the bindings present before they were
changed to call select.lua.

The script-opts are listed one per line in the manual to not make that
section huge.

skip_backward and skip_forward script-opts are omitted to lower the
script-opts number because they are only in box layout and undocumented.

I'm not sure if it's worth adding script-opts for the wheel on the
seekbar.

script-opts for the current and remaining time and fullscreen are not
added to not add more script messages.

Closes mpv-player#6291 and mpv-player#11878.
@kasper93 kasper93 merged commit dab5cf4 into mpv-player:master Oct 22, 2024
24 checks passed
@guidocella guidocella deleted the osc-select branch October 22, 2024 20:45
guidocella added a commit to guidocella/mpv that referenced this pull request Oct 27, 2024
Make the console easier to read because the current default is too
small. See for example
mpv-player#14903 (reply in thread)
or mpv-player#15036 (comment)
or mpv-player#15145 (comment)
or mpv-player#15031 (comment).

This also prevents libass from decreasing performance by printing many
lines.
kasper93 pushed a commit that referenced this pull request Oct 27, 2024
Make the console easier to read because the current default is too
small. See for example
#14903 (reply in thread)
or #15036 (comment)
or #15145 (comment)
or #15031 (comment).

This also prevents libass from decreasing performance by printing many
lines.
Pentaphon referenced this pull request in po5/thumbfast Nov 8, 2024
When hovering certain elements over the OSC, using the mouse wheel can
result in special commands (such as seeking, changing audio tracks,
etc.) Not everyone neccessarily wants this feature, so add an option to
make it possible to disable all of it. Maybe more fine-tuned control
would be more ideal, but probably not worth it. Fixes #13096.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants