-
Notifications
You must be signed in to change notification settings - Fork 3.1k
options: make --write-filename-in-watch-later-config write title as well #16530
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the patch, I also thought this would be useful for web URLs and this was requested on IRC. Some thoughts:
I chose jsonl for history because it is only appended too, it is slow if you rewrite the whole file. That is why the single-file shader cache was replaced with multiple files. In fact doing the opposite was requested in |
Also maybe we should prefix |
Unhelpfully I don't have an opinion on how this all might integrate with mpv's new menuing system, which is the crux of this PR. I've not really had much chance to make use of it yet, although I do intend to eventually as the playlist menu has been extremely handy. As for my own history on this topic, here is what I can share: my old PR #13476 did something similar but preserved the redirect path instead of adding a title. I abandoned this approach because it would end up overburdening mpv's Instead I wrote my own "watch later" command as lua script. Here I can include all the data I want. It is bound as (It does use JSONL as its database format, but it's managed with an external menuing script instead: superwatchlater). As an aside, |
Ok I guess the Also unlike with json this all breaks with paths and titles containing newlines, but they are not common anyway. |
Newlines aren't the problem with json, however json does require valid UTF-8 which is not required by unix filenames. |
cf6ea86
to
cef46ac
Compare
Thank you for all the feedback!
You're right! Storing filename without title certainly seems pointless. I could imagine someone wanting to be able to choose between displaying titles and filenames in the menu, but it is a separate thing. I've rolled the change into
Done.
I haven't added a prefix as per your discussion. It seems to me that a prefix would make it a bit nicer to add more metadata down the line, but I'm uneducated about how playlists work and what additional metadata you might want to store. Let me know if you have a contrary preference. |
Download the artifacts for this pull request: Windows |
const char *title = mp_find_non_filename_media_title(mpctx); | ||
if (title) { | ||
char write_title[1024] = {0}; | ||
for (int n = 0; title[n] && n < sizeof(write_title) - 1; n++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is copied from the function above but is - 1
necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need write_title[1023]
to be reserved for the null byte, so sizeof(write_title) - 1
populates the array up to write_title[1022]
.
Dunno I can't think of other metadata we may need. If you or someone else thinks a prefix is better go ahead and add it. |
3245d7b
to
d0fcc85
Compare
One last change — apologies @guidocella — 6a0454c makes select.lua check all lines after url line for |
Ok I don't think that needs to be done preemptively but whatever you prefer. Do you have other metadata to store in mind? |
Nothing specific in mind. Thumbnail paths or arbitrary user metadata via the Lua API could be cool, but is perhaps unneeded. I'm not familiar enough with the project to know what is possible already and what makes sense to add. 🙂 I do think that it beats the point of a prefix to assume specific pieces of metadata are coming on specific lines (beyond perhaps better readability). If hardcoding the line number is the way to go, perhaps I should get rid of the prefix all together. In my opinion, this way gives more forward flexibility for little cost though. |
6a0454c
to
cdb6743
Compare
write_title() writes the media title to the media's watch_later file as a comment if 'write-filename-in-watch-later-config' is set. Prior to this change, setting the option would only write a commented filename at the top of the file. Now the media title is commented below the filename, with the prefix "title: " Before: # https://example.com/watch?v=foobar start=35 gamma=1 After: # https://example.com/watch?v=foobar # title: Foo Bar (HD Video) start=35 gamma=1
If --write-filename-in-watch-later-config is set and a title is written in the watch later file, display it in the Watch later menu.
cdb6743
to
fe132d1
Compare
The History menu (CTRL-P -> History) displays the titles of previously played files, but the Watch later menu displays a list of filenames.
This PR adds
--write-title-in-watch-later-config
. With the option set,save-position-on-quit
inserts the playing file's title in a comment below the filename in the file's watch_later entry. The Watch later picker then displays these titles instead of filenames when they are available.Examples
mpv --write-filename-in-watch-later-config
:mpv --write-filename-in-watch-later-config --write-title-in-watch-later-config
:mpv --write-title-in-watch-later-config
:Notes:
--write-title-in-watch-later-config
silently no-ops if--write-filename-in-watch-later config
is not set. The Watch later menu is disabled without--write-filename-in-watch-later-config
anyway, so this seems like a reasonable behaviour to me. If it is preferred that--write-title...
automatically enables--write-filename...
or emits an error, let me know.These changes are backwards compatible with older versions of MPV, since, forcing
--write-filename...
to be set ensures that a title comment will never be inserted without a filename comment above it. Title comments will never be misinterpreted as paths to a file by older versions.Titles are displayed instead of filenames in the Watch later menu if
--write-title-in-watch-later-config
is set, otherwise existing behaviour is preserved. Perhaps this should be separated out into its own option, applying to both the History and Watch later menus?I acknowledge this is a hacky solution. If breaking backwards compatibility is an option, it might be nice to replace the watch_later system with a structured data format in line with
watch_history.jsonl
.