ui: Theme lookup should respect XDG_DATA_DIRS

js/ui/main.js

     if (stylesheet.query_exists(null))
         return stylesheet;
 
-    stylesheet = Gio.File.new_for_path(global.datadir + '/theme/' + name);
-    if (stylesheet.query_exists(null))
-        return stylesheet;
+    let dataDirs = GLib.get_system_data_dirs();

js/ui/main.js

     if (stylesheet.query_exists(null))
         return stylesheet;
 
+    let dataDirs = GLib.get_system_data_dirs();
+    for (let i = 0; i < dataDirs.length; i++) {
+        let path = GLib.build_filenamev([dataDirs[i], 'gnome-shell', 'theme', name]);
+        let stylesheet = Gio.file_new_for_path(path);
+        if (stylesheet.query_exists(null)) {

[Marco Trevisan]

Looks good, but... This should also include global.datadir to me, as it could point to some other prefix not included in XDG dirs per build settings.

[Didier Roche]

Thanks! I didn't saw cases where global.datadir wouldn't be included in the system_data_dirs, but I'm happy to add it as a fallback afterwards.

[Didier Roche]

Rebased on latest master and pushed after including Marco's comment.

[Marco Trevisan]

Looks good now, I can't merge this from gitlab, but feel free to rebase again :)

PS: If you allow edits on the MP I guess others devs can do it from gitlab itself too.

[Florian Müllner]

The commit message has errors:

mods, extensions and others GNOME Shell assets

mModes, extensions and others GNOME Shell assets

lookup into each dir, relative to XDG_DATA_DIRS.

are searched in each directory in XDG_DATA_DIRS

Giving those paths higher priority than global.datadir.

This is not a sentence

The fix is to have the theme finding pattern following the same logic than other elements.

following the same logic asthan other elements.

(I'll also note that our stance has always been that themes are not officially supported (which is why there's no setting and users that absolutely must fiddle with the looks need to install an extension), and I don't want anybody to get the impression that this is changing)

[Didier Roche]

Thanks for the commit review, applied and rebased on latest master!

I aree that themes aren't officially supported (and this is why there has been a lot of agreed css breakages in it, I guess). I don't think this gives that impression of official support more as XDG_DATA_DIRS is already searched through to look for modes/….json files. In addition, having those files living anywhere in XDG_DATA_DIRS and not the themes isn't coherent: the themes can be referenced via stylesheetName key from the modes. Another way would be to only load the theme relative to selected modes/….json mode.

I think the best course of actions for now (to not break modes XDG_DATA_DIRS support), is continuing affirming themes aren't officially supported and respecting XDG_DATA_DIRS for them as well.

[Florian Müllner]

I think the best course of actions for now, is continuing affirming themes aren't officially supported and respecting XDG_DATA_DIRS for them as well.

Agreed. My comment wasn't meant as an objection to the change, but as a clarification to avoid any misunderstandings (like, say, some "GNOME finally recognizes hard-working theme authors!11!!" headline on WorldOfGnome)

Modes, extensions and other GNOME Shell assets are search

searched

are search in each directory, relative to XDG_DATA_DIRS.

"each directory, relative to XDG_DATA_DIRS" sounds like "$XDG_DATA_DIRS/*", which isn't what is searched (and likely not even a valid path). If you want to be more specific than the previously proposed "each directory in XDG_DATA_DIRS", use something like "in appropriate subdirectories of each directory in XDG_DATA_DIRS".

However, this isn't the case for themes (which can be referenced by a mod in a different XDG_DATA_DIR).

mode

We give a higher priority to those paths before fallbacking to global.datadir.

fallbacking -> falling back

This is still unclear, as the preceding sentence talks about themes, for which global.datadir is the only path rather than a fallback. Assuming you are talking about non-theme assets, this would be better as an extension of the first sentence:

"[...] XDG_DATA_DIRS, falling back to global.datadir if necessary. However that isn't the case for themes, which are currently always expected in global.datadir, even when referenced by a mode in a different XDG_DATA_DIR."

[Didier Roche]

Agreed. My comment wasn't meant as an objection to the change, but as a clarification to avoid any misunderstandings (like, say, some "GNOME finally recognizes hard-working theme authors!11!!" headline on WorldOfGnome)

Haha, indeed (or some other similar websites) ;)

"each directory, relative to XDG_DATA_DIRS" sounds like "$XDG_DATA_DIRS/*", which isn't what is searched (and likely not even a valid path). If you want to be more specific than the previously proposed "each directory in XDG_DATA_DIRS", use something like "in appropriate subdirectories of each directory in XDG_DATA_DIRS".

This sounds a little bit like repetitive but more precised, agreed.

This is still unclear, as the preceding sentence talks about themes, for which global.datadir is the only path rather than a fallback. Assuming you are talking about non-theme assets, this would be better as an extension of the first sentence:

"[...] XDG_DATA_DIRS, falling back to global.datadir if necessary. However that isn't the case for themes, which are currently always expected in global.datadir, even when referenced by a mode in a different XDG_DATA_DIR."

Agreed, changed to this.

I just pushed the new changelog with those changes, thanks!

[Florian Müllner]

LGTM, but the branch got behind master and I don't have permissions to rebase ...

[Florian Müllner]

Nit: No braces

[Didier Roche]

Fixed and rebased on master.