command: normalize paths for path and track-list/N/external-filename #15780

Dudemanguy · 2025-01-30T22:01:37Z

Normalize path and track-list/N/external-filename
Rename track-list/N/external-filename to track-list/N/external-path because it actually returns a path
Add track-list/N/external-filename and track-list/N/external-filename/no-ext properties.

kasper93 · 2025-01-30T22:20:27Z

Rename track-list/N/external-filename to track-list/N/external-path because it actually returns a path

This is breaking change. It even breaks first party clients see

mpv/osdep/win32/smtc.cpp

Line 192 in 62c3aeb

mpv_node *file = node_map_get(list->values + i, "external-filename");

and

mpv/osdep/mac/remote_command_center.swift

Line 197 in 62c3aeb

    
           if let filename = track["external-filename"] as? String, externalCoverPath == nil {

And probably all non-first part scripts.

Add track-list/N/external-filename and track-list/N/external-filename/no-ext properties.

I don't understand why we need to duplicate those? We even export utils.split_path in lua and javascript to handle pathes.

Dudemanguy · 2025-01-30T22:30:44Z

This is breaking change. It even breaks first party clients

Didn't realize this was so popular. Nobody really complained about something named filename returning a path and not a filename? I didn't realize it had that behavior until recently and was surprised. I guess we can just deprecate it then.

I don't understand why we need to duplicate those?

I mean it's not really critical but we have path and filename already so it felt like a natural thing to do.

kasper93 · 2025-01-30T22:45:43Z

Nobody really complained about something named filename returning a path and not a filename? I didn't realize it had that behavior until recently and was surprised.

Well, the property name is only cosmetic and having returning only filename for external file would be rather useless, because how would you reference that file?

Also stream-open-filename is also a path...

I guess we can just deprecate it then.

I don't agree to break something that has hundreds of matches on github alone https://github.com/search?q=external-filename+mpv&type=code in scripts and applications. Do we really want to force everyone to change just for the sake of renaming it? It will be deprecated for years at minimum and still some clients won't be updated.

github-actions · 2025-01-30T22:57:20Z

Download the artifacts for this pull request:

Windows

macOS

Dudemanguy · 2025-01-30T23:04:03Z

I think it's bizarre that property names don't actually give back what they say they do but fine I guess if everyone is used to it. This is just the path normalization stuff now.

kasper93 · 2025-01-31T04:43:42Z

options/path.c

+                             const char *path)
+{
+    assert(talloc_ctx && "mp_normalize_user_path requires talloc_ctx!");
+    char *expanded = mp_get_user_path(NULL, global, path);


If talloc_ctx is required, why not use it here?

It's only required because mp_normalize_path below requires it. I thought it was better to free the unneeded memory after the path is constructed.

Maybe we should fix mp_normalize_path to clean internal allocations. Because it is getting awkward. In principle talloc_ctx should only contain output allocation. And it should work with NULL, as any other function like we have.

I've added a commit that fixes that as simplifies several calls of it as well.

kasper93 · 2025-01-31T04:44:41Z

player/command.c

@@ -507,7 +507,10 @@ static int mp_property_path(void *ctx, struct m_property *prop,
    MPContext *mpctx = ctx;
    if (!mpctx->filename)
        return M_PROPERTY_UNAVAILABLE;
-    return m_property_strdup_ro(action, arg, mpctx->filename);
+    char *path = mp_normalize_path(mpctx, mpctx->filename);


use temporary talloc_ctx, it will leave some allocations that won't be deallocated if attached to mpctx

kasper93 · 2025-01-31T04:45:13Z

player/command.c

@@ -1997,6 +2000,9 @@ static int get_track_entry(int item, int action, void *arg, void *ctx)
    struct MPContext *mpctx = ctx;
    struct track *track = mpctx->tracks[item];

+    char *external_filename = mp_normalize_user_path(mpctx, mpctx->global,


kasper93 · 2025-01-31T04:46:20Z

player/command.c

@@ -507,7 +507,10 @@ static int mp_property_path(void *ctx, struct m_property *prop,
    MPContext *mpctx = ctx;
    if (!mpctx->filename)
        return M_PROPERTY_UNAVAILABLE;
-    return m_property_strdup_ro(action, arg, mpctx->filename);
+    char *path = mp_normalize_path(mpctx, mpctx->filename);


Also I though you want to normalize mpctx->filename itself, not only the property.

Nah, I decided that was too risky.

kasper93 · 2025-01-31T04:48:41Z

I think it's bizarre that property names don't actually give back what they say they do but fine I guess if everyone is used to it. This is just the path normalization stuff now.

The renaming stuff can be revisited in separate PR, but we need to be careful doing changes like that. We have bad track record of breaking our api users.

kasper93 · 2025-01-31T05:01:12Z

I think our normalize function does more than expected. Normalization is fine, but making this resolve symlinks may not be expected as we never did that in our properties.

guidocella · 2025-01-31T09:27:19Z

Normalization doesn't resolve symlinks unless they contain ../ (because otherwise normalization would be wrong).

player/command.c

misc/path_utils.c

Instead of requiring the caller to pass an appropriate talloc_ctx, mp_normalize_path will make its own internal allocations and cleanup as needed.

If a talloc_ctx was being made solely for a onetime usage of getting a normalized string, we can now just pass NULL instead and free the string directly to make things easier.

Expanding a path and then normalizing it is probably a common shorthand. The reason to not incorporate this directly into mp_normalize_path is because it requires the global struct which is out of scope for path_utils so use this wrapper instead.

Now that mp_normalize_path can accept NULL and we have a shorthand for expand + normalize, now is a good chance to simplify this to make it less awkward.

It's better for API users to actually get predictable results.

kasper93 · 2025-01-31T21:45:46Z

should be fine.

sfan5 · 2025-02-09T22:29:43Z

player/command.c

@@ -1997,6 +2000,9 @@ static int get_track_entry(int item, int action, void *arg, void *ctx)
    struct MPContext *mpctx = ctx;
    struct track *track = mpctx->tracks[item];

+    char *external_filename = mp_normalize_user_path(NULL, mpctx->global,
+                                                     track->external_filename);


this keeps spamming [ 1.966][d][global] user path: 'folder.jpg' -> 'folder.jpg' in my logs. depends on circumstances track-list may (apparently) be read a few times per osc update.
obviously the name should only be normalized once

why does this go through "user_path" stuff at all? it's not like the external filename is supposed to contain e.g. ~~global/ or so

mp_get_user_path logs everytime it's called. Anytime the property is read, this will get called. One way to avoid that would be to save the track's external filename as expanded directly, but not sure if that would have some other unforeseen consequence or so.

It needs it in order to expand ~/path which works now. mp_normalize_path on its own is too dumb to handle that.

One way to avoid that would be to save the track's external filename as expanded directly, but not sure if that would have some other unforeseen consequence or so.

Well, normalizing the filename once when you open it and remembering what the path was sounds like the correct behavior to me.

Dudemanguy force-pushed the filename-properties branch from b04e522 to 6f6b63a Compare January 30, 2025 22:13

Dudemanguy force-pushed the filename-properties branch from 7b85de1 to 8a98188 Compare January 30, 2025 23:02

Dudemanguy changed the title ~~reorganize path and external-filename properties~~ command: normalize paths for path and track-list/N/external-filename Jan 30, 2025

kasper93 reviewed Jan 31, 2025

View reviewed changes

Dudemanguy force-pushed the filename-properties branch 4 times, most recently from 04bccb8 to b0a95a0 Compare January 31, 2025 19:40

kasper93 reviewed Jan 31, 2025

View reviewed changes

player/command.c Outdated Show resolved Hide resolved

kasper93 reviewed Jan 31, 2025

View reviewed changes

misc/path_utils.c Outdated Show resolved Hide resolved

Dudemanguy added 5 commits January 31, 2025 14:59

misc/path_utils: allow passing NULL to mp_normalize_path

577c69c

Instead of requiring the caller to pass an appropriate talloc_ctx, mp_normalize_path will make its own internal allocations and cleanup as needed.

various: simplify mp_normalize_path usage

e4302b7

If a talloc_ctx was being made solely for a onetime usage of getting a normalized string, we can now just pass NULL instead and free the string directly to make things easier.

command: simplify find_track_with_url handling

fbc9d5c

Now that mp_normalize_path can accept NULL and we have a shorthand for expand + normalize, now is a good chance to simplify this to make it less awkward.

command: normalize paths for path and track-list/N/external-filename

bee35b8

It's better for API users to actually get predictable results.

Dudemanguy force-pushed the filename-properties branch from b0a95a0 to bee35b8 Compare January 31, 2025 21:02

Dudemanguy merged commit 38ad1ed into mpv-player:master Feb 1, 2025
28 checks passed

Dudemanguy deleted the filename-properties branch February 1, 2025 16:12

sfan5 reviewed Feb 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

command: normalize paths for path and track-list/N/external-filename #15780

command: normalize paths for path and track-list/N/external-filename #15780

Dudemanguy commented Jan 30, 2025

kasper93 commented Jan 30, 2025

Dudemanguy commented Jan 30, 2025

kasper93 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 •

edited

Loading

Dudemanguy commented Jan 30, 2025 •

edited

Loading

kasper93 Jan 31, 2025

Dudemanguy Jan 31, 2025

kasper93 Jan 31, 2025

Dudemanguy Jan 31, 2025

kasper93 Jan 31, 2025

kasper93 Jan 31, 2025

kasper93 Jan 31, 2025

Dudemanguy Jan 31, 2025

kasper93 commented Jan 31, 2025

kasper93 commented Jan 31, 2025

guidocella commented Jan 31, 2025

kasper93 commented Jan 31, 2025

sfan5 Feb 9, 2025

Dudemanguy Feb 12, 2025

sfan5 Feb 12, 2025

command: normalize paths for path and track-list/N/external-filename #15780

command: normalize paths for path and track-list/N/external-filename #15780

Conversation

Dudemanguy commented Jan 30, 2025

kasper93 commented Jan 30, 2025

Dudemanguy commented Jan 30, 2025

kasper93 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 • edited Loading

Dudemanguy commented Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasper93 commented Jan 31, 2025

kasper93 commented Jan 31, 2025

guidocella commented Jan 31, 2025

kasper93 commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2025 •

edited

Loading

Dudemanguy commented Jan 30, 2025 •

edited

Loading