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

DOCS/man/mpv: improve path docs and clarify config-dir path behavior #15218

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

Conversation

Arcitec
Copy link

@Arcitec Arcitec commented Oct 29, 2024

The previous documentation was pretty scary and misleading, and was also outdated in several sections.

It was also difficult to understand some of the paragraphs.

The path documentation has now been overhauled to be easier to understand and to document the latest mpv behavior.

This change documents the fact that there's no difference between ~~/foo and ~~home/foo in terms of that "scary bug". They both use the exact same code and have the same --no-config "bug" behavior.

References:

Edit: Improved further, as described here: #15218 (comment)

sfan5
sfan5 previously approved these changes Oct 30, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

lgtm

DOCS/man/mpv.rst Outdated Show resolved Hide resolved
@Arcitec
Copy link
Author

Arcitec commented Oct 31, 2024

@sfan5 @na-na-hi I used this scripts/testpath.lua script to discover a few other quirks, and have now rewritten the path documentation to remove a lot of outdated sentences and improve overall clarity. It now uses the admonition style as mentioned by nana.

local mp = require("mp")
local msg = require("mp.msg")

for _, prefix in ipairs({ "~~global/", "~~home/", "~~/" }) do
    for _, n in ipairs({ "", "foo/bar" }) do
        local p = prefix .. n
        local res = mp.command_native({ "expand-path", p })
	    msg.info(p .. ": " .. type(res) .. " = <" .. tostring(res) .. ">")
    end
end

for _, n in ipairs({ "encoding-profiles.conf", "mpv.conf" }) do
	local res = mp.find_config_file(n)
	msg.info("find: " .. n .. ": " .. type(res) .. " = <" .. tostring(res) .. ">")
end

Behavior:

$ mpv movie.mkv
[testpath] ~~global/: string = </etc/mpv>
[testpath] ~~global/foo/bar: string = </etc/mpv/foo/bar>
[testpath] ~~home/: string = </home/johnny/.config/mpv>
[testpath] ~~home/foo/bar: string = </home/johnny/.config/mpv/foo/bar>
[testpath] ~~/: string = </home/johnny/.config/mpv>
[testpath] ~~/foo/bar: string = </home/johnny/.config/mpv/foo/bar>
[testpath] find: encoding-profiles.conf: string = </etc/mpv/encoding-profiles.conf>
[testpath] find: mpv.conf: string = </home/johnny/.config/mpv/mpv.conf>

$ mpv --no-config --script=/home/johnny/.config/mpv/scripts/testpath.lua movie.mkv
[testpath] ~~global/: string = <>
[testpath] ~~global/foo/bar: string = <foo/bar>
[testpath] ~~home/: string = <>
[testpath] ~~home/foo/bar: string = <foo/bar>
[testpath] ~~/: string = <>
[testpath] ~~/foo/bar: string = <foo/bar>
[testpath] find: encoding-profiles.conf: nil = <nil>
[testpath] find: mpv.conf: nil = <nil>

It's a full rewrite of the original commit contents, so check the changes in the new commit here (may need to press Ctrl-R to see the new commit if browser cached):

https://github.com/mpv-player/mpv/pull/15218/commits

I have tried compiling it with rst2html, and the result looks well-formatted.

@Arcitec Arcitec changed the title DOCS/man/mpv: clarify config-dir path behavior DOCS/man/mpv: improve path docs and clarify config-dir path behavior Oct 31, 2024
Copy link
Contributor

@na-na-hi na-na-hi left a comment

Choose a reason for hiding this comment

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

Looks good. Just need to wrap commit message at 72 characters per line.

@Arcitec
Copy link
Author

Arcitec commented Oct 31, 2024

Ah yeah. I've fixed that now. Didn't realize that mpv uses wrapping.

I don't wrap commit messages normally, since it's actively harmful to the Git history and totally pointless and causes tons of extra work of manually wrapping and rewriting the lines until they wrap nicely (and some lines will then be very wide and others short since certain words won't fit on the same line), and the process of rephrasing/adding/removing some words later can waste minutes of moving and rewriting words to adapt the line wrapping again.

And then when you view those patches in anything that won't exactly fit 72 characters wide, you'll get wonderful "double wrapping" where the viewport also wraps your already manually wrapped lines into a mess of very small fragments (especially on mobile or on tiling window managers or in split terminals)... All of that is much better handled by the viewer application than the committer. There's a reason why we don't manually wrap our Word documents. :D

It's a very painful convention that dates back to manually emailed patches and mailing lists. Even there it was dubious, since email clients always wrap text automatically too. And people weren't manually wrapping their email messages back then, just the attached patches, which makes no sense. I wish that had never become a convention. But it's going out of favor. Modern clients like GitKraken don't even support manual wrapping.

But when it's already the convention of a project it should of course be followed. Sorry about that! All done! :)

And hey, at least it encourages people to write very short commit messages, since manual wrapping is so tedious. ;)

DOCS/man/mpv.rst Outdated Show resolved Hide resolved
DOCS/man/mpv.rst Outdated Show resolved Hide resolved
DOCS/man/mpv.rst Outdated Show resolved Hide resolved
DOCS/man/mpv.rst Outdated Show resolved Hide resolved
@Arcitec
Copy link
Author

Arcitec commented Oct 31, 2024

Thanks for the feedback. I have pushed resolutions to all comments. The suboption example was a big miss (didn't realize it referred to : and ,-separated values, which was due to the lack of clarity in the original paragraph), and the other feedback also lead to even more precise documentation. The goal is that average people should be able to read this and understand it.

Changes:

https://github.com/mpv-player/mpv/compare/4a73cdca17f399d10ae55513280072bb964b183b..d64d1fe9600824e42981dbe377bd77a1425596b3

DOCS/man/mpv.rst Outdated Show resolved Hide resolved
@Arcitec
Copy link
Author

Arcitec commented Oct 31, 2024

Per last review comment:

  • Improved clarity of the description of ~/ by mentioning the terminal/shell environment variable equivalents, which are better examples than the old $HOME example. And used the word "terminal" since everyone knows what a terminal is, but not everyone knows that a terminal is the way to interact with a shell.
  • Moved ~/ to the top of the list so that the rest of the list flows better, and replaced the confusing "user home directory root" description with the less confusing "The current user's home directory".
  • Fixed the inconsistent use of "Sentence case." and punctuation in the other list entries.

Changes (shows changes from both force pushes since last review):

https://github.com/mpv-player/mpv/compare/d64d1fe9600824e42981dbe377bd77a1425596b3..f19903099f0e15ac6d370402ae50605dfd5a0983

PS: I wasn't a fan of the exe_dir description violating the principle that (win32 only) should really be a suffix of the sentence, but I don't know how that path works and did not understand what the current description refers to, so I just improved the formatting but left the sentences exactly as-is.

That entry looks like this now. It seems like it's the path to the mpv.exe directory but can be overridden by MPV_HOME environment variable? If someone knows how it works and wants to rephrase it to be clearer, feel free to suggest something, or we can just keep it as-is:

image

DOCS/man/mpv.rst Outdated Show resolved Hide resolved
DOCS/man/mpv.rst Outdated Show resolved Hide resolved
DOCS/man/mpv.rst Outdated Show resolved Hide resolved
DOCS/man/mpv.rst Show resolved Hide resolved
DOCS/man/mpv.rst Show resolved Hide resolved
@Arcitec
Copy link
Author

Arcitec commented Nov 4, 2024

Changes:

https://github.com/mpv-player/mpv/compare/f19903099f0e15ac6d370402ae50605dfd5a0983..9fb2662ba54c9f34785c9de880392f4c34de6dbd

  • Indicate that suboptions aren't just coming from the CLI.
  • Shorten the scripting API filename safety paragraph.
  • Shorten the relative path expansion paragraph in --no-config section.
  • Came up with a solution for the exe_dir phrasing so that it follows the style of the other options.

Hopefully it's ready now! :)

Edit: Change-link above includes the last force-push after this comment too.

@Arcitec Arcitec requested a review from sfan5 November 4, 2024 14:25
The previous documentation was pretty scary and misleading, and was
also outdated in several sections.

It was also difficult to understand some of the paragraphs.

The path documentation has now been overhauled to be much easier
to understand and to document the latest mpv behavior.
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.

4 participants