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

Improve conditional profile #12

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

christoph-heinrich
Copy link
Contributor

I have used the method with two profiles for a while now and it works fine, but I wanted the ability to exclude multiple paths with that method, and this is the result of that. It's probably quite a bit slower then before, but that shouldn't cause any problems.

Better support for URLs would also be great, but we'd have to add an URL check for that.
URLs actually already work, but are treated as relative paths, so the working-directory gets prefixed, which might result in false positives. I could port my URL-check from quality-menu if you think that's worth it.

find is faster then match, especially when plain find is used.
Plain find also prevents users from accidentally using any of the
pattern matching.
@po5
Copy link
Owner

po5 commented Aug 11, 2023

The auto profile example is only an example. I like the first commit, and how the third commit moves the string that actually matters to the user closer to the beginning of the line with an assignment.
If the goal is to make it easier to handle common yet convoluted exclusions, without removing auto-profile functionality, I'd rather make a memo-profiles.lua that toggles the script-opt based on common user-configurable scenarios. There's too much jank to work around with auto-profiles.

Your URL check wouldn't work for memo, as it returns false on http://localhost/thing.mkv and mpv's custom protocols e.g. slice://0m@https://github.com/bower-media-samples/big-buck-bunny-1080p-30s/raw/master/video.mp4.
We already have a simpler, "better" check in the script, which catches even magnet links (because protocol handlers without double forward slashes exist, and there are scripts for playing those in mpv).
It will fail to get the full path when playing a local file named dd:d.mp4 on Linux with a relative path, but this will never happen in practice as all file managers under the sun pass absolute paths.
If we want to do it "right", we need to mirror mpv's own logic (already mostly implemented) for turning paths into absolute paths for a very similar purpose to memo's, and mp_is_url.
I don't like their URL check, as it doesn't work with protocol handlers that don't take a double forward slash like magnet links.
We could of course maintain a list of special magnet-like protocols but we'd still get tripped by files named magnet:lol.mp4, I'm happy with the current tradeoffs.

I don't want something even more complex in readme, and the URL check should be lax.
Here's mp_is_url ported to lua... path:find("^%a[%w%.%-%+]-://") (and pretend it's boolean) I'm pretty sure it produces exactly the same output. May end up replacing memo's ^%a[%a%d-_]+: with ^%a[%w%.%-%+]-: too.

Unrelated thought, maybe should add a keybind to manually log an entry for the current file, if someone wants to use it for bookmarks. I don't know how you use memo, curious to hear.

christoph-heinrich added a commit to christoph-heinrich/memo that referenced this pull request Aug 11, 2023
@christoph-heinrich christoph-heinrich force-pushed the improve_conditional_profile branch from a0b57f8 to 2e08119 Compare August 11, 2023 14:35
@christoph-heinrich
Copy link
Contributor Author

I've dropped the two profile thing and made the function a bit faster.
The URL check has been added in a separate commit.

So far I've been using memo mostly to open files that are in the same folder as a file I had open recently, instead of going there via the file manager. In that sense a sort of "recent directories" would actually be more useful then a "recent files" 😆, but it has also been useful for opening recent URLs without having to find them again on the web.

@po5
Copy link
Owner

po5 commented Aug 11, 2023

You forgot to remove or "MyCunnyFolder2".
You can kind of achieve a recent directory listing with hide_same_dir=yes and press uosc's open-file keybind.

Paths to be excluded can be simply added to the list.
That makes the profile more complicated, but user only have to adjust
the list, so it's not so bad.
@christoph-heinrich christoph-heinrich force-pushed the improve_conditional_profile branch from 2e08119 to 9f1d5b8 Compare August 11, 2023 14:57
@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 11, 2023

You forgot to remove or "MyCunnyFolder2".

Oops that got accidentally dropped during the rebase, I wanted to keep it in to make it clear to users that this is a comma separated list (probably not obvious for people unfamiliar with lua).

You can kind of achieve a recent directory listing with hide_same_dir=yes and press uosc's open-file keybind.

hide_same_dir=yes is a good tip. The other files in the folder get added to the playlist via autoload.lua for me, but it's basically the same workflow.

Unrelated thought, maybe should add a keybind to manually log an entry for the current file, if someone wants to use it for bookmarks.

Sounds like a good idea (and easy to implement). In that case they would permanently have enabled=no I'd assume.
Something like a "remove last entry" might also be useful, but that might not play nice with multiple concurrent instances.

Edit: Random thought, but a "recent folders" would work well for continuing e.g. TV Shows, as the episodes themselves don't always have descriptive names (e.g. "Episode 1.mp4", but from which show?).

@po5
Copy link
Owner

po5 commented Aug 16, 2023

tomasklaen/uosc@f857d46 reminded me that magnet links start with a query string we can match against.
^%a[%w%.%-%+]-:[/?] would be my preferred way to detect if we're playing a link.
Will merge as-is and deal with unifying everything later.

Something like a "remove last entry" might also be useful, but that might not play nice with multiple concurrent instances.

I don't think it's possible without introducing reliability or privacy issues, see #7. The only thing that could fit the current model would be hiding the entry, but it can't be erased from the actual history file.

a "recent folders" would work well for continuing e.g. TV Shows, as the episodes themselves don't always have descriptive names (e.g. "Episode 1.mp4", but from which show?).

Then a user with files named for Plex comes along and says "Cool Show/Season 01/Episode 01.mp4" only shows the season. This kind of issue is why hide_same_dir does what it says, skipping files from the same directory as known files. Other scripts like recent-menu can have fancy (and expensive) checks based on text distance and assumptions of specific file naming conventions... I can't trust it and I don't need it.
Don't want to add a new menu for it, the most I'm willing to add if requested is to prepend the directory to the filename for display. Dir/File.mp4 with a truncation bias for the folder. If it can't fit then surely your filename was descriptive enough and you don't need to know the dir.

@po5 po5 merged commit 98f68e8 into po5:master Aug 16, 2023
@christoph-heinrich
Copy link
Contributor Author

fancy (and expensive) checks based on text distance and assumptions of specific file naming conventions

The check could actually be quite simple, no need for text distances. Assuming a directory structure like <show name>/<season>/<episode> is reasonable and the check could be purely based on that. Something along the lines of this should already work pretty well imo.

local shows = {}
for file in recent_files do
    local season_path, filename = utils.split_path(file.path)
    if season_path:find(".+/Season %d+$") then
        local show_path, season = utils.split_path(season_path)
        local _, show_name = utils.split_path(show_path)
        shows[#shows + 1] = { label = show_name, file = file}
    end
end

Obviously I didn't actually look at your code to see if that fits your data structures, but that should be able to share pretty much all the code paths with the regular menu, the only difference being the filtering at the beginning.
If someone were actually implement an additional keybinding for that, does that have any chance of landing?

the most I'm willing to add if requested is to prepend the directory to the filename for display. Dir/File.mp4 with a truncation bias for the folder

While that would technically work, I imagine that would end up being pretty bad ux.

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.

2 participants