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

Allow @ keyind to be used (mpv Next Chapter) #705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Assistant
Copy link
Contributor

This allows the default keybind for Next Chapter in mpv to be used when the option to automatically chat is enabled.

@Et0h
Copy link
Contributor

Et0h commented Dec 10, 2024

Thanks for this contribution. I use mpv, but I don't use the instant chat feature. It wold be nice to hear from at least one other person who would be impacted by this change about what they think the expected/ideal behaviour is.

@Assistant
Copy link
Contributor Author

Of course.
For context the current behavior is if you press @ instead of skipping to the next chapter it would open the chat and type "@" to it.
With this change pressing @ will skip to the next chapter, while allowing "@" to be typed if the chat is opened first.
I've been using this patch for a long time so I don't remember if it's possible to use the keybind bypassing the instant chat, but if it is I believe it wasn't very obvious.

@Et0h
Copy link
Contributor

Et0h commented Dec 10, 2024

I've been using this patch for a long time so I don't remember if it's possible to use the keybind bypassing the instant chat, but if it is I believe it wasn't very obvious.

According to https://syncplay.pl/guide/client/ "In some cases when in the ‘instant chat’ mode you might actually want to trigger one of the keys from the alphabet rows, such as Control+T to set set 'stay on top' to Yesand Control+Shift+T to set it to No. To do this you first have to press ‘Tab’ to enter into the ‘Alphabet rows temporarily behave as normal’ mode. You press tab again to exit that mode."

Pressing tab is also mentioned in the tooltip for the option within the configuration and within mpv input screen when the feature is enabled. It also tells you within mpv to press tab to let you know you have temporarily disabled it and to press tab again to return to instant chat mode.

@Assistant
Copy link
Contributor Author

Reading might not be my strong suit.
Regardless, personally the case where I want to go to the next chapter is a lot more frequent than the case where I want to start a chat message with "@", so I think that should be the easier option.

@Et0h
Copy link
Contributor

Et0h commented Dec 27, 2024

Before coming to a decision I'll ask some people listed in the Syncplay change logs as contributing to our mpv support in the past two years to give them an opportunity to share their perspective if they wish to do so: @mbalandis @notpeelz @soredake @ahmubashshir @ducreyna @hunbernd @Abu-AM

@mbalandis
Copy link

mbalandis commented Dec 30, 2024

I personally don't use neither '@' to skip chapters nor the chat feature (very rarely use it) so it wouldn't affect me (probably) however I can't speak for others... and there is a small risk imho. I think the best practice is to add an option in settings (use '@' for mpv to skip chapters). Should be relatively simple instead of just hardcoding it like in this PR. Again... might be fine as it is but this is how as a dev would do it.

@Et0h
Copy link
Contributor

Et0h commented Jan 23, 2025

I personally don't use neither '@' to skip chapters nor the chat feature (very rarely use it) so it wouldn't affect me (probably) however I can't speak for others... and there is a small risk imho. I think the best practice is to add an option in settings (use '@' for mpv to skip chapters). Should be relatively simple instead of just hardcoding it like in this PR. Again... might be fine as it is but this is how as a dev would do it.

Thanks for sharing your usage and perspective. The option you suggest is valid, and I see a pro and a con to that approach:

  • Pro: Making it an option that you have to enable will minimise disruption for those who start with @ and give users more control.
  • Con: This would add more complexity to the dialogue and require not just coding work but translation work too.

So far we've had nobody say they actually start with an @. Maybe someone does it if they are using it to identify someone, or maybe someone in a channel has the name @. So there might be a use case where people want it, but there might not be.

It is the case that:

  • Syncplay only has a finite number of users
  • Of those who use Syncplay, only some of them use mpv
  • Of those who use Syncplay and mpv, only some of them use chat
  • Of those who use Syncplay and mpv and chat, only some of them use direct input chat or have a reason to start chat with a @, and it is possible that nobody does both.

As such, this could be a corner case which exists theoretically but not in practice, and the one person who does use it could just manually change the file to add back the removed keybinding (or see the change in the changelog and request we make the modification optional).

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.

3 participants