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

Add Settings checkbox to disable EOF restart (addresses #3785) #4568

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Aug 8, 2023


Description:

NOTE: If testing this, do not use key bindings! Use either the Playback menu or the Play button in the OSC. There are currently a couple of open bugs which are causing this key binding to be unreliable.

Trying to push this along. Not in love with the text, but would love suggestions.
SCR-20230808-crzq

Also encountered an issue with the Play button. Because it's implemented as a Cocoa toggle button with 2 states, clicking on it will always toggle the icon. Couldn't find an easy way to tell it not to toggle, which presented a problem when at the end and the Play button does nothing. Added a quick & dirty fix by adding code to the syncUI mechanism to bring it back in sync with the actual play state, but there may be numerous other ways to do it and seemed like that could be a whole issue by itself.

…. Add code to syncUI so that pressing play at EOF when this feature is disabled doesn't result in a UI lie
@low-batt
Copy link
Contributor

low-batt commented Aug 8, 2023

I too am struggling with a name for this setting. As there are multiple ways to restart playback I would go generic and avoid using controls names such as "play" and "resume". Also, possibly we can rely upon the context of being a sub-option of Keep window open after playback finishes. Maybe: Restarting playback seeks to the beginning? I'm not entirely happy with that.

On this:

Because it's implemented as a Cocoa toggle button with 2 states, clicking on it will always toggle the icon. Couldn't find an easy way to tell it not to toggle, which presented a problem when at the end and the Play button does nothing.

The correct solution is to disable the button once EOF is reached if the user has turned off the setting to restart from the beginning. This also means the Resume menu item should be disabled. With the feature of automatically restarting from the beginning disabled you can't start playback while at EOF. The Apple way to indicate controls and menu items are not applicable is to disable them. If the user changes the playback position then these controls would be enabled again.

With this PR not addressing the issues with the spacebar, which is what issue #3785 is reporting we can't say this PR fixes that issue. I'm thinking this needs to be put in draft mode until the key binding problems are sorted out. IINA is too far behind on reviewing and merging. I will see if I can get the attention of the other developers.

@svobs
Copy link
Contributor Author

svobs commented Aug 9, 2023

The correct solution is to disable the button once EOF is reached if the user has turned off the setting to restart from the beginning. This also means the Resume menu item should be disabled. With the feature of automatically restarting from the beginning disabled you can't start playback while at EOF. The Apple way to indicate controls and menu items are not applicable is to disable them.

Yeah, definitely a rabbit hole of its own...

So I actually attempted this, and it's harder than it seems. We can't generalize that the play button should be disabled at EOF, because it may still be legitimate to play the next item in the playlist. Or loop back from the beginning of the playlist. Or something else. It will require careful consideration of all the possibilities so that we don't risk introducing a new bug where it worked fine before.

Also, what about the Playback > Resume menu item? And what about the Next and Prev buttons? I would think they should match the behavior of the Play button too.

Considering the effort that would go into that, let's not have perfect be the enemy of the good? It's a pretty minor issue to worry about, considering it's blocking a fix which multiple users have expressed interest in getting resolved.

Maybe: Restarting playback seeks to the beginning? I'm not entirely happy with that.

That could work. Will continue to ponder...

I'm thinking this needs to be put in draft mode until the key binding problems are sorted out

Looks like that should happen soon, but I'll put in draft for now as per request.

@svobs svobs marked this pull request as draft August 9, 2023 23:18
@low-batt
Copy link
Contributor

Thanks for putting this into draft mode. I hope the devs will find some time for reviewing and merging soon.

Also, what about the Playback > Resume menu item?

I mentioned that in my comment. Yes, that menu item also needs to be disabled. I don't think the next and prev operations are tied to this.

Disabling is definitely the correct way to handle this. It is not a case of disabling at EOF. It is disabling only when pressing space does not start playback. Exactly the same conditions the code already has to check.

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.

None yet

2 participants