XDG directory spec

[ghost]

Fix for #109

This is a full implementation that uses $XDG_CONFIG_HOME and $XDG_CONFIG_DIRS.
The rationale is that since CONFIG_HOME corresponds to /etc and DATA_HOME corresponds to /usr/share, everything should go in CONFIG_HOME.

This currently breaks support on Windows and Mac, so I'd like some pointers on what the config directories on those platforms are.

[ghost]

Upon trying this branch,the watch later thing (shift+q) stopped working. There was a memory leak. The code for --config-dir (force_configdir) and "$MPV_HOME" was removed. As you say, it breaks Windows and Mac support. Also, ideally there should be some sort of migration between old and new config dirs. So I guess there's some work to do.

This currently breaks support on Windows and Mac, so I'd like some pointers on what the config directories on those platforms are.

Well, the code is there. There's also osdep/path-win.c and osdep/path-osx.m. One weird thing about Windows is that it supports two config locations, the official Microsoft one, and in the .exe dir. OSX supports both the UNIX convention and the "Bundle" stuff (which I don't quite understand myself).

[ghost]

Unfortunately, I have no way to test the Windows or Mac fixes, so I'd like to request some help with that.

Still need to fix watch_later, memory leaks come last.

Anything else I'm forgetting?

[ghost]

Not sure.

By the way, I think the main reasons why "user" and "system" config dirs were separate are:

1. There needs to be a way to lookup a "user" path even if the file doesn't exist, if the file should be created (this might be what the watch later code uses, not sure, didn't check)
2. Looking up the watch_later data should be fast, because if you e.g. load a playlist with 10000 entries, it will look up 10000 config files, just to see if one of the playlist entries should be resumed.
3. In some cases, you want to load both "system" and "user" config file; like /etc/mpv/mpv.conf and ~/.mpv/config.

[ghost]

It boils down to implementing a list of config directories.

The first directory in the list becomes the user writable directory.

Incremental config files (I don't recommend this, it overcomplicates things) just need to be loaded in reverse order that they're found

I'm not sure why you'd look up 10000 config files at once, just check it when the video loads instead of when the playlist loads

[ghost]

It boils down to implementing a list of config directories.
The first directory in the list becomes the user writable directory.

Incremental config files (I don't recommend this, it overcomplicates things) just need to be loaded in reverse order that they're found

Sounds more complicated than separate functions for "user"/"system" config files, but maybe this is the right approach. It could also be helpful for implementing cascading config files and for simplifying config file handling on MS Windows.

I'm not sure why you'd look up 10000 config files at once, just check it when the video loads instead of when the playlist loads

This is for resuming playlists, i.e. for finding the first file on the playlist that should be resumed. It's pretty nice when playing all files in a directory with mpv * and being able to resume where you stopped last time. One could just hash the playlist contents, but this is IMO simpler and more flexible.

[ghost]

I guess you still want cascading config files after all.

Anyway, couple of small things I'd like to ask:

1. What's mp_get_user_path() for?
2. Does mpv have an official way of finding memory leaks?

[ghost]

I guess you still want cascading config files after all.

I'm just saying that's a possible implementation. Or you can switch back to using separate functions for user/system config files.

What's mp_get_user_path() for?

Users were complaining they can't refer to other config-file-like paths using ~/ (since that is normally expanded by the shell), so this function does that.

Does mpv have an official way of finding memory leaks?

Valgrind, or for the talloc functions MPV_LEAK_REPORT=1 works too.

[ghost]

Okay, that should be everything.

For backwards compatibility, I kept ~/.mpv as a search directory, and /config (as opposed to /mpv.conf) as a valid config file name, you may or may not want to remove them someday in the future.

Also, could someone check if the search dirs on Windows and Mac are correct? It's part of verbose output, so just building and running MPV_VERBOSE=1 /path/to/mpv will do.

[ghost]

Someone tested it on OSX and reported that it appeared to work (including bundle), but nobody did on win32 yet. So screw Windows and leave fixing it for later (probably even after merge), but first I'll review the patch in its current state.

[ghost]

Please no identifiers that start with _. Although in this case it's still legal, some other uses of leading underscores are not allowed, because the C standard reserves them for the standard library. (See section 7.1.3 in the C11 standard.)

[rossy]

I tried building this on Windows and there are two issues:

• mp_get_win_exe_dir doesn't compile. All instances of path should be replaced with w_exedir.
• mp_config_dirs should append "/mpv" to the result of mp_get_win_exe_dir and mp_get_win_app_dir before adding them to the list.

Apart from that, it looks good on Windows, though I agree with what @wm4 is saying about the $HOME variable. $HOME isn't really used on Windows. I keep it set for the benefit of Cygwin programs, but that means that some native programs store their configuration in my Cygwin directory (GIMP and VirtualBox are the main offenders.) IMO native programs should ignore Unix-isms like $HOME (and probably the XDG stuff too) and Cygwin programs should ignore Windows-isms, so when mpv is built for Cygwin it should behave the same as on Linux when searching for config files.

[ghost]

@knthzh: looks generally good to me, but apparently the ms windows specific handling still needs improvement.

[ghost]

mp_config_dirs should append "/mpv" to the result of mp_get_win_exe_dir and mp_get_win_app_dir before adding them to the list

You sure about exe_dir/mpv? Well, if you say so.

Just checking, should app_dir or exe_dir have priority?

[rossy]

You sure about exe_dir/mpv?

It would match the old behaviour. The official builds come with a custom fonts.conf in this directory.

Just checking, should app_dir or exe_dir have priority?

I think exe_dir should, since it would let different versions of mpv run on the same machine with different configs. That's how it currently works, I think.

[rossy]

Just tested 0c6dd72 and it looks good.

[ghost]

Note that if you have foo\mpv.exe, git master will AFAIK try to load both foo\mpv\config and foo\config. This behavior was explicitly added in the past, and then fixed again, so this behavior is definitely wanted. I'm not sure/didn't test which config file is preferred or whether this PR implements this.

[ghost]

Sorry, that huge ifdef (even with a nested ifdef) is a bit ugly.

[ghost]

And especially here, !dir[0] would be more readable.

[ghost]

Note that if you have foo\mpv.exe, git master will AFAIK try to load both foo\mpv\config and foo\config

Done. foo/mpv/config had priority

This looks wrong. We always want to load the config files which we install

XDG_CONFIG_DIRS is meant for overriding that. As an example, maybe a developer wants to test a clean config without mpv defaulting to the ones installed with it.

Maybe we shouldn't even honor XDG_CONFIG_DIRS

Just keep it, it's not like it's hard to implement.

[ghost]

XDG_CONFIG_DIRS is meant for overriding that. As an example, maybe a developer wants to test a clean config without mpv defaulting to the ones installed with it.

What if it's set to something useless by default instead? Did you check what Gnome/KDE/xfce do?

[ghost]

What if it's set to something useless by default instead?

It's the distro's responsibility to organize downstream configuration changes

Did you check what Gnome/KDE/xfce do?

All DEs I know of leave it unset by default

[ghost]

OK then.

Reducing the ifdef ugliness would still be nice, though.

[ghost]

I'm not convinced this looks any better, especially since ifdefs read like regular ifs to me.

I almost certainly broke Windows support again, though, so that needs another round of checking.

[rossy]

Windows build looks good.

[ghost]

Well then, I'll probably merge it now.

[ghost]

OK, on testing, I determined it doesn't properly take the union of all config dirs.

Let mpv1 be mpv compiled without this patch, and mpv2 compiled with this patch. Then if you do:

mpv1 file1.mkv # quit with playback resume
mpv1 file2.mkv # quit with playback resume
mpv2 file1.mkv # resumes properly
mpv2 file1.mkv # quit with playback resume
mpv2 file2.mkv # doesn't resume!

So that means as soon as you quit mpv2 with quit_watch_later, all previously suspended files suddenly won't resume anymore. The underlying reason is that quit_watch_later creates ~/.config/mpv/watch_alter, which then overshadows ~/.mpv/watch_later.

The same problem exists with the lua config subdirectory. According to XDG, it should load all lua scripts in all lua subdirectories in all config paths, but in fact it will load only the one with the highest priority.

[ghost]

Done

[ghost]

Pushed it. I made a bunch of heavy modifications on top of it. I hope this is not considered offensive; some issues I thought better were doing myself (instead of keeping demanding new changes etc.), and some things I noticed only later. Maybe I'm a bit too pedantic and I didn't want to keep making new demands. In any case, you're a great contributor.

Anyway, apart from this, there's still a problem: now it will always write new config files (watch later configs at least) to the directory containing the .exe (instead of the app dir), because that directory always exists and is highest on the priority list. I assume this will annoy windows users?

[ghost]

I hope this is not considered offensive

Of course not, it's your project, lol.

now it will always write new config files to the directory containing the .exe

I'm just following what @rossy2401 said. It doesn't really make sense to dump files all over the place (I know I'd get pissed from having to track multiple locations). Deciding which directory to consolidate all the files to... Well, I'll leave that to you guys. It's just a two line change in osdep/path-win.c

[ghost]

It's just a two line change in osdep/path-win.c

Yeah, one could for example prefer the app dir over the exe dir. But then the app dir would always be created, and maybe someone doesn't like it. I'm not sure what's the best behavior, so I'll leave it to the windows guys.

[rossy]

I think the problem is that different Windows users want different things. Some people understandably want config files to be stored in the proper location, which is why #458 was created. These people could be storing mpv.exe in a read-only directory (like C:\Program Files) so it would be a bad idea to create new files in the exe directory in this case.

Other people (myself included) like portable apps, which are completely self-contained and can be stored on flash drives or network drives. There is a portable version of SMPlayer and I'm fairly sure Baka MPlayer is portable. For these apps it's the opposite. They should write everything to their directory and leave nothing on the host machine.

I'm not sure how to please both groups of users. Perhaps the AppData directory could be preferred for the sake of the first group of users, and the second group could set MPV_HOME to turn mpv into a portable app. Preferring the AppData directory seems more "correct" than trying to store files in the exe directory.