Fix ordering of defaults for unset/empty XDG_DATA_DIRS #171
Conversation
Administrator-owned .desktop files in /usr/local/share/applications/ should be able to override system files in /usr/share/applications/, if no XDG_DATA_DIRS is given. However, `get_search_paths` returned them in the opposite order, so that system .desktop files took precedence. This is confirmed by the XDG Base Directory Specification: (https://specifications.freedesktop.org/basedir-spec/0.8/ar01s03.html) > $XDG_DATA_DIRS defines the preference-ordered set of base directories > If $XDG_DATA_DIRS is either not set or empty, a value equal to > /usr/local/share/:/usr/share/ should be used. Use the correct default so administrator-owned files take precedence.
|
Note that this fixes somewhat of a regression: In r2.18 the default for For r3.0+ there were a lot of refactors which removed the calls to reverse the directories. So, in r3.0+ the order is incorrect when |
There was a problem hiding this comment.
You are correct. j4-dmenu-desktop used to read desktop files in reverse order relative to $XDG_DATA_HOME:$XDG_DATA_DIRS. This was done to facilitate desktop file ID collisions. The old system read desktop files in reverse order and when it encountered a collision, the new incoming desktop file replaced the previously saved one. This implementation is conforming to the XDG specification, but is wasteful, because j4dd had to read, parse and store colliding desktop files which were thrown out when the desktop file with higher priority was encountered.
I have changed this behavior in #132. j4-dmenu-desktop now handles collisions before reading any desktop files and colliding desktop files are simply ignored (but logged). Desktop file collisions don't happen often, so the performance improvement is negligible, but other changes to Applications/AppManager made this change easy.
As you've noticed, I've forgot to fix SearchPath. Thanks for fixing that!
| setenv("XDG_DATA_HOME", "/does/not/exist", 1); | ||
| unsetenv("XDG_DATA_DIRS"); | ||
|
|
||
| std::vector<std::string> result = get_search_path(); | ||
|
|
||
| std::vector<std::string> expected; | ||
| if (is_directory("/usr/local/share/applications/")) | ||
| expected.push_back("/usr/local/share/applications/"); | ||
| if (is_directory("/usr/share/applications/")) | ||
| expected.push_back("/usr/share/applications/"); | ||
|
|
||
| REQUIRE(result == expected); |
There was a problem hiding this comment.
This test heavily relies on and changes the environment. This has to be done with great care. The expected variable has some edge cases, but you've handled them. The overridden $XDG_DATA_HOME and $XDG_DATA_DIRS will leak into other tests. The current tests also do this (they come from the r2.18 version), so I'll tolerate it here, but I should maybe change these tests later. But "if it ain't broke, don't fix it."
There was a problem hiding this comment.
Actually, aside from the awkwardness, I realized that due to depending on the environment, this test does most often not prove what it should. Mainstream distros (Arch, Ubuntu) do not create /usr/local/share/applications/ by default (you have to do it yourself), so on a vanilla install, this test will pass even if you revert the changes in src/SearchPath.cc. I have opened #172 to try to address this.
Administrator-owned .desktop files in /usr/local/share/applications/ should be able to override system files in /usr/share/applications/, if no XDG_DATA_DIRS is given. However,
get_search_pathsreturned them in the opposite order, so that system .desktop files took precedence.This is confirmed by the XDG Base Directory Specification:
Use the correct default so administrator-owned files take precedence.