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

feat: add items to playlist from files menu when holding ctrl #822

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

christoph-heinrich
Copy link
Contributor

Closes #821

@hooke007
Copy link
Contributor

You can use ctrl+enter or ctrl+click to load the whole directory in mpv instead of navigating its contents.

Ctrl has been used. Could the dir be append in playlist in uosc? I never tried that.

@christoph-heinrich
Copy link
Contributor Author

Looks like the readme wasn't updated in 6dfef3d

@tomasklaen
Copy link
Owner

I think shift would be a better modifier here. And other menus could probably use it for more appropriate things as well. It was a mistake to hardcode shift handling into Menu :/

So, we need to remove the click/enter shift handling from Menu, and leaving it for individual menu openers (they can set their menu as keep_open, and close manually when shift is not present), which on our side would be only the main menu.

@hooke007
Copy link
Contributor

hooke007 commented Dec 28, 2023

So, we need to remove the click/enter shift handling from Menu, and leaving it for individual menu openers (they can set their menu as keep_open, and close manually when shift is not present), which on our side would be only the main menu.

It is complex. IMO, Shift is useful with track menu, especial when I use mpv as a music player.

@hooke007
Copy link
Contributor

hooke007 commented Dec 28, 2023

Also I think this feature ideally should be implement after #475 (comment) .
And then we do not need to make dynamic binding so messy.

@tomasklaen
Copy link
Owner

True, track menus could use shift too. Still, it shouldn't be hardcoded into Menu.

The biggest issue I see at the moment is that 3rd party scripts have no access to the modifiers yet, so they can't implement manual closing like this. Are there any scripts that heavily rely on current shift behavior?

turns out passing nil as the last parameter to commandv is not the same
as passing nothing, and results in an error
@christoph-heinrich
Copy link
Contributor Author

I actually think shift as it behaves right now might be useful in addition to the ctrl introduced here.

@tomasklaen
Copy link
Owner

Oh wait, I assumed the file menu already won't close when adding to playlist. I wouldn't want adding to playlist shortcut to close the menu.

@christoph-heinrich
Copy link
Contributor Author

Sorry for the delay on this.

The menu now stays open when appending to the playlist.
There is no feedback to the user that anything happened, which makes it not so great to use. Maybe we should flash the selected item when the menu stays open upon selection, but that is a more general problem and doesn't have to be part of this PR.

@christoph-heinrich christoph-heinrich marked this pull request as draft January 7, 2024 06:51
@christoph-heinrich christoph-heinrich marked this pull request as ready for review January 7, 2024 06:58
@tomasklaen
Copy link
Owner

The more I think about it, the more I want this to be on the shift modifier. It makes way more sense there, and current shift behavior is pointless here. In main menu, shift activates without closing, and this would add to playlist without closing. Fits way better imo.

But I realize that will be a bit more work as we need to move shift handling from menu into individual menu creators.

Or I can just merge this as is, and move it to shift some time later. Up to you.

@christoph-heinrich
Copy link
Contributor Author

I expect to be pretty busy over the next month or so, so I think merging as is and adjusting later is the way to go.

@tomasklaen
Copy link
Owner

All right. But swap the alt<->ctrl modifiers so that directory activation remains on ctrl.

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Jan 8, 2024

You want me to mostly undo 6dfef3d? I can do that tomorrow.

@tomasklaen
Copy link
Owner

Oh, forgot about that :)

@tomasklaen tomasklaen merged commit 0f970b5 into tomasklaen:main Jan 8, 2024
@christoph-heinrich christoph-heinrich deleted the append_to_playlist branch January 8, 2024 21:36
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.

Menu to append files to playlist
3 participants