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.

Sign up for GitHub

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

Jump to bottom

rename various options for consistency reasons #12641

Merged
merged 6 commits into from
Oct 25, 2023
Merged

rename various options for consistency reasons #12641

merged 6 commits into from
Oct 25, 2023
+54 −41

Conversation

Dudemanguy
Copy link
Member

This better reflects what it actually does. As a bonus, script writers won't be misled into thinking that fps displays the actual video or display fps.

@Dudemanguy Dudemanguy force-pushed the rename-fps branch 3 times, most recently from edb12ed to 8b7ced6 Compare October 15, 2023 16:08
@Dudemanguy Dudemanguy changed the title f_decoder_wrapper: rename fps option to force-container-fps rename various options for consistency reasons Oct 15, 2023
@Dudemanguy
Copy link
Member Author

So @guidocella pointed out some inconsistencies in the option naming in IRC, so I went ahead and included additional commits to change those. Feel free to to comment.

@guidocella
Copy link
Contributor

We should pluralize --sub-ass-style-override to --sub-ass-style-overrides because the other list options that aren't abbreviated are plural, looks good otherwise. Is hwdec-current more problematic to rename because there is no OPT_REPLACED mechanism?

@Dudemanguy
Copy link
Member Author

We should pluralize --sub-ass-style-override to --sub-ass-style-overrides

Fair point.

Is hwdec-current more problematic to rename because there is no OPT_REPLACED mechanism?

Nah, it's more that I wasn't sure. There's already a hwdec-interop property for example so it creates a different inconsistency if you change it to current-hwdec.

@guidocella
Copy link
Contributor

IMHO it's not an inconsistency because "hardware decoding interop driver" is correct English while "hardware decoding current" is just nonsense.

@Dudemanguy
Copy link
Member Author

That's true, but it's weird imo to have the property names not start the same. Like most video-related properties are video-something.

@github-actions
Copy link

github-actions bot commented Oct 15, 2023 •
edited
Loading

Download the artifacts for this pull request:

Windows

@N-R-K
Copy link
Contributor

N-R-K commented Oct 21, 2023

While we're at it, can we find a better name for demuxer-hysteresis-secs ?

@Dudemanguy
Copy link
Member Author

Seems surprisingly not controversial? I thought people were going to get upset at me for changing --screenshot-directory.

While we're at it, can we find a better name for demuxer-hysteresis-secs ?

Name seems OK to me? Haven't actually looked at you PR yet though.

@N-R-K
Copy link
Contributor

N-R-K commented Oct 24, 2023

Name seems OK to me?

I was hoping we can use something that's in people's everyday vocabulary. Something like demuxer-sleep-until or demuxer-wake-when-remaining etc (better suggestions welcome).

@Dudemanguy
options: rename --fps to --container-fps-override
9d22b23 
This better reflects what it actually does. As a bonus, script writers
won't be misled into thinking that fps displays the actual video or
display fps.
@Dudemanguy
options: rename --override-display-fps to --display-fps-override
897911b 
Other similar options are in the form of --foo-override not
--override-foo. The display-fps one was backwards so flip it around the
other way for consistency reasons.
@Dudemanguy
options: rename --sub-ass-force-style to --sub-ass-style-overrides
69b6acf 
This option has exactly the same semantics are other mpv options that
override a particular thing with something from the user. So instead of
the "force-style" name, use "-overrides" which is more consistent.
The plural form is used since it's a list option.
@Dudemanguy
options: rename --screenshot-directory to --screenshot-dir
fec71b4 
Less characters is better? Other options use -dir for directory so
consistency I guess.
@Dudemanguy
options: rename --watch-later-directory to --watch-later-dir
a2be52c 
Same logic as the previous commit.
@Dudemanguy
options: rename --play-dir to --play-direction
7bae311 
--play-dir sounds like it has something to do with directories so change
it. The play_dir variable is used a bunch everywhere internally so
whatever just leave it alone instead of renaming that.
@Dudemanguy Dudemanguy merged commit 2783d5a into mpv-player:master Oct 25, 2023
14 checks passed
@Dudemanguy Dudemanguy deleted the rename-fps branch October 25, 2023 16:16
@dexeonify dexeonify mentioned this pull request Dec 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasper93 kasper93 kasper93 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Dudemanguy @guidocella @N-R-K @kasper93