mpv icon indicating copy to clipboard operation
mpv copied to clipboard

osc.lua: open select.lua by clicking buttons

Open guidocella opened this issue 1 year ago • 18 comments

8bf5548589 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 #11878.

guidocella avatar Oct 08 '24 20:10 guidocella

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.

Samillion avatar Oct 08 '24 20:10 Samillion

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?

kasper93 avatar Oct 09 '24 02:10 kasper93

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.

Samillion avatar Oct 09 '24 02:10 Samillion

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.

guidocella avatar Oct 09 '24 04:10 guidocella

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.

na-na-hi avatar Oct 09 '24 05:10 na-na-hi

I can bind MBTN_RIGHT to closing the console or print circles in the playlist selector (this was requested before in https://github.com/mpv-player/mpv/pull/14087#issuecomment-2110889436) if desired.

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

guidocella avatar Oct 09 '24 07:10 guidocella

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.

na-na-hi avatar Oct 09 '24 08:10 na-na-hi

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.

kasper93 avatar Oct 09 '24 12:10 kasper93

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.

Samillion avatar Oct 09 '24 13:10 Samillion

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 https://github.com/mpv-player/mpv/pull/14088 if requested.

guidocella avatar Oct 09 '24 13:10 guidocella

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

Remove it, if you don't like it.

kasper93 avatar Oct 09 '24 14:10 kasper93

I thought we were going to make bindings configurable first?

guidocella avatar Oct 09 '24 17:10 guidocella

I thought we were going to make bindings configurable first?

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

kasper93 avatar Oct 09 '24 17:10 kasper93

I changed the bindings to right click to be usable on laptops and because they are more useful than show-text.

I also added the fade animation when hiding the osc.

Left click on the title now opens the playlist selector and "show playlist position and length and full title" is moved to right click, because left clicking the title is not as useful as left clicking to go to adjacent playlist entries or chapters, and the feature is more discoverable if at least 1 binding is left click.

If we're willing to do a bigger change, we could bind left click to select tracks, because it is more useful to see what tracks you're selecting beforehand than with playlist and chapter navigation. uosc also opens its track menu on left click and doesn't bind cycling tracks at all. Any thoughs before making the bindings configurable?

guidocella avatar Oct 16 '24 13:10 guidocella

lose of cycle backward for audio/sub is concerning to me.

If we're willing to do a bigger change, we could bind left click to select tracks

I'm fine with this and dropping cycle track. Or making it with modification. Cycling 40 subtitle tracks is never good experience...

Side note, we don't have video track selector, we should add it to osc.lua at some point.

kasper93 avatar Oct 16 '24 13:10 kasper93

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

guidocella avatar Oct 16 '24 14:10 guidocella

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

guidocella avatar Oct 17 '24 17:10 guidocella

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 #15145 which I'm fine with.

llyyr avatar Oct 21 '24 19:10 llyyr

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 https://github.com/mpv-player/mpv/pull/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.

kasper93 avatar Oct 22 '24 18:10 kasper93

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?

guidocella avatar Oct 22 '24 18:10 guidocella

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

Samillion avatar Oct 22 '24 19:10 Samillion

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.

guidocella avatar Oct 22 '24 20:10 guidocella

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.

Samillion avatar Oct 22 '24 20:10 Samillion