major changes #9
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Previously, a fallback would have only happened, when either `hash` was disabled or the `hash()`-function failed, but not when only the (hash) file didn’t exist. Signed-off-by: Christoph Anton Mitterer <[email protected]>
Clarify that no parameter or tilde expansion happens and that absolute pathnames are required. Also do that actually in the example value (which users may just uncomment) by no longer using `$HOME` but rather a value where typically no write permissions should exist. Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
This commit adds another option `global_chapters_by_xattr`, which, if enabled, causes an XATTR of the media files to be used for determining the name of the global chapters file. The idea is, that all media files have an XATTR set that contains some kind of ID (like a hash value of the file or some MusicBrainz ID) that is unique for each media file. In contrast to the current `hash`-option this has some advantages: - The hash value doesn’t need to be recalculated every time. - With the `hash`-option, the hash value is calculated from the media file’s pathname. If this changes (for example, when the file is moved), the chapters file can’t be found anymore. If another file is moved to the pathname from a previously moved/deleted file, a wrong chapters file would be used for it. The XATTRs aren’t affected by this. Because the XATTR method seems less fragile then the method that hashes the pathname, it was given higher priority. But a user can simply not enable it and only use the latter. Future enhancements could allow to select multiple XATTRs (either one after one or combining their values into one ID). Fixes: 6 Signed-off-by: Christoph Anton Mitterer <[email protected]>
Unfortunately `getfattr` doesn’t use proper exit statuses (see attr bug #33417 (https://savannah.nongnu.org/bugs/index.php?33417)) so one needs to match the error message. For that reason, `LC_ALL` is set to `C` (for the `getfattr` process only) in order to get the same error message, regardless of the user’s locale. Signed-off-by: Christoph Anton Mitterer <[email protected]>
Previously, the trailing newline of strings read from other processes’ standard output was stripped off by searching for *all* newlines in the string and removing them. While command names should typically not contain newlines, media fails may do and so their “corrected” pathnames would have not pointed to the actual file. This commit fixes this by properly stripping of the final character from standard output (assuming it to be a newline). Fixes: 8 Signed-off-by: Christoph Anton Mitterer <[email protected]>
This is in order to align it to the previously added `global_chapters_by_xattr`- option and to make it more clear that it will only affect global chapters. Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Currently this option disables both, reading and writing, at once. Future changes could allow to configure this separately. Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
Previously, this script was a single file `chapters.lua`, to be placed in the mpv script directory `~/.config/mpv/scripts`. This commit changes this to use the mode, where a subdirectory of the mpv script directory “belongs” to the whole script. In that mode, the script’s name is determined by the subdirectory’s name. And the script’s main file must be named `main.lua`. The reason for this change is the 2nd part of this commit, which changes the default location for the global directory to be the (dynamically determined) script directory (i.e. the one that also contains `main.lua`). This has the benefit, that we can write chapter files in that directory, without fearing to clash with other scripts. Since all chapter files get the extension `.ffmetadata`, there should be no danger of accidentally having them interpreted by mvp or clash with our own script file. Signed-off-by: Christoph Anton Mitterer <[email protected]>
mar04
approved these changes
Oct 9, 2023
mar04
approved these changes
Oct 9, 2023
mar04
approved these changes
Oct 9, 2023
mar04
previously approved these changes
Oct 9, 2023
|
Could you split those into cosmetic fixes, that xattr thing and bug fixes? I would merge some of those commits right away, others I would like to test first when I have some more time. |
Uhm... well if it really really really needs to be, cause otherwise you wouldn't consider for merging it. The problem is that disentangling this will not be quite a mess as at least some commits touch the same places, IIRC. That's partially the reason why I hadn't made proper branches for each bigger topic, cause they'd get just merge conflicts if they were first made all on Can't you just go through one by one from the earliest commit and merge one after another as soon as you find time for testing each? |
|
Ok, I went through the commits: 3, 12 - drop, I don't like the changes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey.
Sorry for not splitting this up in more branches (was lazy), but at least the changes are all split up nicely in small commits.
Please check them and merge :-)
Thanks,
Chris.