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

options: watch-later-options changes #12532

Merged
merged 3 commits into from
Nov 12, 2023
Merged

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Oct 1, 2023

--border is there, so for consistency --title-bar should be too.

@github-actions
Copy link

github-actions bot commented Oct 1, 2023

Download the artifacts for this pull request:

Windows

@kasper93 kasper93 changed the title options: add --title-bar to default watch-later-options options: remove --ontop and --border from watch-later-options Oct 1, 2023
@Dudemanguy
Copy link
Member

Should be mentioned in interface-changes.

@kasper93
Copy link
Contributor Author

kasper93 commented Oct 1, 2023

sure, added.

@christoph-heinrich
Copy link
Contributor

fullscreen and pause could also be removed.

@low-batt
Copy link
Contributor

low-batt commented Oct 3, 2023

Seems inconsistent that sub-visibility is included, but secondary-sub-visibility is not.

I do not understand the requirement cited for removing ontop, namely that saved options be "content related". To me "Watch Later" is about resuming viewing where you left off. If you had the window set to be on top, why would you want that setting to be dropped? And as already pointed out, if this is the requirement then fullscreen and pause should be removed as well.

If fullscreen is removed then the RESUMING PLAYBACK section of the manual will need to be updated as it mentions restoring the fullscreen state.

@kasper93
Copy link
Contributor Author

kasper93 commented Oct 3, 2023

I initially only wanted to fix inconsistency between title-bar and border which are tightly connected options and one is saved and the other not. But as always with such changes a lot voices suggested another options and we are again at the point of no return.

I do not understand the requirement cited for removing ontop, namely that saved options be "content related". To me "Watch Later" is about resuming viewing where you left off. If you had the window set to be on top, why would you want that setting to be dropped? And as already pointed out, if this is the requirement then fullscreen and pause should be removed as well.

The question is if you watched something few days ago, while doing something else and enabled ontop, is this really relevant now to restore it? It is global player state and indeed same as fullscreen should be set in config. We want to "resume playback" and not "restore player state".

The most basic would be watch-later-options=start, but of course it makes sense to restore also track selection, image correction and few other video/audio specific parameters. The option is even called save-position-on-quit, but it does lot's more.

In short resuming playback in my opining should start playback where you left of, but not necessarily restore player config as this is not really related to the content you are resuming playing.

There is of course a line and depends where you want to put it. I for one like pause, because it makes easy to restart player to reload config and start at the same frame, but I guess I just got used to it and should use --pause instead.

EDIT:

Also note while we have fullscreen we don't save fs-screen and so on. So this restore is already broken.

EDIT2:

I will update PR with suggested changes.

@kasper93 kasper93 changed the title options: remove --ontop and --border from watch-later-options options: watch-later-options changes Oct 7, 2023
@kasper93
Copy link
Contributor Author

kasper93 commented Oct 7, 2023

Rebased with added suggestions. I don't have strong opinion on this topic, so will let you decide what to do with this. I only wanted to fix inconsistency with --border. But other changes makes sense too, so we can have them if all agree.

@guidocella
Copy link
Contributor

I don't think it makes sense to save the video-margin- options because they are meant for the OSC, with --script-opts=osc-boxvideo=yes,osc-visibility=always video-margin-ratio-bottom gets saved and it is restored the nextime even with different/default OSC options, which is annoying.

@kasper93
Copy link
Contributor Author

kasper93 commented Oct 8, 2023

I don't think it makes sense to save the video-margin- options because they are meant for the OSC, with --script-opts=osc-boxvideo=yes,osc-visibility=always video-margin-ratio-bottom gets saved and it is restored the nextime even with different/default OSC options, which is annoying.

Removed.

@hooke007
Copy link
Contributor

Is there any use case of osd-level for single file?

@kasper93 kasper93 force-pushed the win32_t branch 2 times, most recently from 21e3820 to 31b5804 Compare October 28, 2023 20:13
@kasper93
Copy link
Contributor Author

Removed osd-level too.

Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

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

This got lost, but I think it's a strict improvement so might as well pull this in now after a rebase.

@low-batt
Copy link
Contributor

low-batt commented Nov 7, 2023

In RESUMING PLAYBACK the manual says:

mpv also stores options other than the playback position when they have been modified after playback began, for example the volume and the fullscreen state

As this commit removes fullscreen from the watch-later-options default, that example should be updated.

In watch-later-options the manual shows this example:

  • --watch-later-options-remove=fullscreen The fullscreen state won't be saved to watch later files.

That example should also be updated.

@hooke007
Copy link
Contributor

hooke007 commented Nov 7, 2023

Ideally, add all the props it support into the manual. Users don't know which prop can & cannot be used.

@Jules-A
Copy link

Jules-A commented Nov 7, 2023

What about creating a new command that specifically tries to recreate the player's state? I'm not really a fan of removing such options unless there's a solution in place to restore that functionality...

@christoph-heinrich
Copy link
Contributor

I'm not really a fan of removing such options unless there's a solution in place to restore that functionality...

e.g. --watch-later-options-append=fullscreen

@Jules-A
Copy link

Jules-A commented Nov 7, 2023

e.g. --watch-later-options-append=fullscreen

Sure but it gets really messy when you have to re-add like 5+ items. Also it's not really easy for people new to MPV. Another thing is what do you do if you want to restore playback state but not the playback position?

@hooke007
Copy link
Contributor

hooke007 commented Nov 7, 2023

Another thing is what do you do if you want to restore playback state but not the playback position?

remove start

@Jules-A
Copy link

Jules-A commented Nov 7, 2023

remove start

You mean you still use save-position-on-quit but remove start? That's rather hilarious

@hooke007
Copy link
Contributor

hooke007 commented Nov 7, 2023

Another thing is what do you do if you want to restore playback state but not the playback position?

I'm confused. Isn't the 'hilarious' question asked by you?

@guidocella
Copy link
Contributor

Ideally, add all the props it support into the manual. Users don't know which prop can & cannot be used.

You can see it with mpv --help=watch-later-options, though it's not very discoverable.

In watch-later-options the manual shows this example:

  • --watch-later-options-remove=fullscreen The fullscreen state won't be saved to watch later files.

That example should also be updated.

If you update that kasper you might as well replace --watch-later-options-clr with --watch-later-options=start since I wrote the clr example when start was implicitly included and removing start is undesirable for most users.

@Jules-A
Copy link

Jules-A commented Nov 7, 2023

I'm confused. Isn't the 'hilarious' question asked by you?

The option is called save-position-on-quit even if you specifically don't want to save position, which is what I find funny.

@guidocella
Copy link
Contributor

The option is called save-position-on-quit even if you specifically don't want to save position, which is what I find funny.

--save-position-on-quit existed long before --watch-later-options.

@hooke007
Copy link
Contributor

hooke007 commented Nov 7, 2023

You are not the first one point out the option's name is unreasonable.
When I was the newbie, I was annoyed why it restored so many irrelevant props.(I wanted position only)

@kasper93
Copy link
Contributor Author

kasper93 commented Nov 8, 2023

For some reason, I was unsubscribed from this thread, didn't see your comments. Will try to address them now.

When I was the newbie, I was annoyed why it restored so many irrelevant props.(I wanted position only)

We all did... It is one of those options, when you find it you just keep --watch-later-options=start in your config to not be annoyed by some weird option restores.

EDIT:

Updated. Suggestions welcome.

Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge later if nobody has any further comments.

Removes:
--border
--fullscreen
--ontop
--osd-level
--pause

Those options are not really content related. I don't see much gain to
save them per each watch later entry.
Adds:
--secondary-sub-visibility
--video-aspect-method
--video-unscaled
--video-pan-x
--video-pan-y
--video-rotate
--video-crop
--video-zoom
--video-scale-x
--video-scale-y
--video-align-x
--video-align-y

Those properties are related to playback state and are likely expected
to be restored when resuming playback.
@Dudemanguy Dudemanguy merged commit ad02db8 into mpv-player:master Nov 12, 2023
14 checks passed
@kasper93 kasper93 deleted the win32_t branch November 12, 2023 21:37
@mlindner
Copy link
Contributor

This is a bad change given right now it's cumbersome to add these options back in. You have to use --watch-later-options-add and you get spammed with warnings about the option being deprecated.

@christoph-heinrich
Copy link
Contributor

You have to use --watch-later-options-add and you get spammed with warnings about the option being deprecated.

You could use watch-later-options-append instead.

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.

8 participants