Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • This project
    • Loading...
  • Sign in / Register
G
gnome-shell
  • Overview
    • Overview
    • Details
    • Activity
    • Cycle Analytics
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Charts
  • Issues 120
    • Issues 120
    • List
    • Board
    • Labels
    • Milestones
  • Merge Requests 33
    • Merge Requests 33
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
    • Charts
  • Registry
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • GNOME
  • gnome-shell
  • Merge Requests
  • !70

Merged
Opened Mar 30, 2018 by Didier Roche@didrocks 
  • Report abuse
Report abuse

ui: Theme lookup should respect XDG_DATA_DIRS

mods, extensions and others GNOME Shell assets lookup into each dir, relative to XDG_DATA_DIRS. However, this isn't the case for themes (which can be referenced by a mod in a different XDG_DATA_DIR), hardcoding global.datadir.

The fix is to have the theme finding pattern following the same logic than other elements. Fixes #167 (closed).

Edited Mar 30, 2018 by Didier Roche
×

Check out, review, and merge locally

Step 1. Fetch and check out the branch for this merge request

git fetch https://gitlab.gnome.org/didrocks/gnome-shell.git theme-xdg-dirs
git checkout -b didrocks/gnome-shell-theme-xdg-dirs FETCH_HEAD

Step 2. Review the changes locally

Step 3. Merge the branch and fix any conflicts that come up

git checkout master
git merge --no-ff didrocks/gnome-shell-theme-xdg-dirs

Step 4. Push the result of the merge to GitLab

git push origin master

Note that pushing to GitLab requires write access to this repository.

Tip: You can also checkout merge requests locally by following these guidelines.

  • Discussion 11
  • Commits 1
  • Changes 1
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ resolvedCountText }} resolved
  • Didier Roche @didrocks

    added 1 commit

    • f2ba71c0 - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    Mar 30, 2018

    added 1 commit

    • f2ba71c0 - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    added 1 commit <ul><li>f2ba71c0 - ui: Theme lookup should respect XDG_DATA_DIRS</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/70/diffs?diff_id=5010&start_sha=6fbc7433a4c351b3b8cb2530c186dd2a0ee96ee4)
    Toggle commit list
  • Didier Roche @didrocks

    changed the description

    Mar 30, 2018

    changed the description

    changed the description
    Toggle commit list
  • Marco Trevisan
    @3v1n0 started a discussion on the diff Apr 12, 2018
    Last updated by Didier Roche Apr 13, 2018
    js/ui/main.js
    254 254 if (stylesheet.query_exists(null))
    255 255 return stylesheet;
    256 256
    257 stylesheet = Gio.File.new_for_path(global.datadir + '/theme/' + name);
    258 if (stylesheet.query_exists(null))
    259 return stylesheet;
    257 let dataDirs = GLib.get_system_data_dirs();
    • Marco Trevisan @3v1n0 commented Apr 12, 2018
      Developer

      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.

      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 @didrocks commented Apr 13, 2018

      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.

      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.
    Please register or sign in to reply
  • Didier Roche @didrocks

    added 1 commit

    • 5b5d99b2 - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    Apr 13, 2018

    added 1 commit

    • 5b5d99b2 - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    added 1 commit <ul><li>5b5d99b2 - ui: Theme lookup should respect XDG_DATA_DIRS</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/70/diffs?diff_id=5658&start_sha=f2ba71c0cc72c7494073059451714a00b4d04f8a)
    Toggle commit list
  • Didier Roche @didrocks commented Apr 13, 2018

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

    Rebased on latest master and pushed after including Marco's comment.
  • Marco Trevisan @3v1n0 commented Apr 13, 2018
    Developer

    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.

    Edited Apr 13, 2018 by 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 @fmuellner commented Apr 13, 2018
    Master

    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)

    The commit message has errors: > mods, extensions and others GNOME Shell assets {-m-}{+M+}od{+e+}s, extensions and other{-s-} 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. follow{-ing-} the same logic {+as+}{-than-} 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 @didrocks

    added 126 commits

    • 5b5d99b2...69afe778 - 125 commits from branch GNOME:master
    • c9feb51b - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    Apr 16, 2018

    added 126 commits

    • 5b5d99b2...69afe778 - 125 commits from branch GNOME:master
    • c9feb51b - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    added 126 commits <ul><li>5b5d99b2...69afe778 - 125 commits from branch <code>GNOME:master</code></li><li>c9feb51b - ui: Theme lookup should respect XDG_DATA_DIRS</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/70/diffs?diff_id=5771&start_sha=5b5d99b288560a36c4de5012807fc6ad06c31e9b)
    Toggle commit list
  • Didier Roche @didrocks commented Apr 16, 2018

    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.

    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 @fmuellner commented Apr 16, 2018
    Master

    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."

    > 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 @didrocks

    added 1 commit

    • 955df15d - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    Apr 16, 2018

    added 1 commit

    • 955df15d - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    added 1 commit <ul><li>955df15d - ui: Theme lookup should respect XDG_DATA_DIRS</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/70/diffs?diff_id=5785&start_sha=c9feb51b35e7145cfbc7d70cc473b1f13d0202ff)
    Toggle commit list
  • Didier Roche @didrocks commented Apr 16, 2018

    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!

    > 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 @fmuellner commented Apr 16, 2018
    Master

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

    LGTM, but the branch got behind master and I don't have permissions to rebase ...
  • Florian Müllner
    @fmuellner started a discussion on an old version of the diff Apr 16, 2018
    Last updated by Didier Roche Apr 16, 2018
    js/ui/main.js
    256 256 if (stylesheet.query_exists(null))
    257 257 return stylesheet;
    258 258
    259 let dataDirs = GLib.get_system_data_dirs();
    260 for (let i = 0; i < dataDirs.length; i++) {
    261 let path = GLib.build_filenamev([dataDirs[i], 'gnome-shell', 'theme', name]);
    262 let stylesheet = Gio.file_new_for_path(path);
    263 if (stylesheet.query_exists(null)) {
    • Florian Müllner @fmuellner commented Apr 16, 2018
      Master

      Nit: No braces

      Nit: No braces
    • Didier Roche @didrocks

      changed this line in version 6 of the diff

      Apr 16, 2018

      changed this line in version 6 of the diff

      changed this line in [version 6 of the diff](https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/70/diffs?diff_id=5803&start_sha=955df15d8ae63a6c632c00b94e92749153052da4#11951386089e872969d36b9c0122193dcdc3d4d8_263_263)
      Toggle commit list
    Please register or sign in to reply
  • Didier Roche @didrocks commented Apr 16, 2018

    Fixed and rebased on master.

    Fixed and rebased on master.
  • Didier Roche @didrocks

    added 3 commits

    • 955df15d...f1b1501f - 2 commits from branch GNOME:master
    • d6d09fd3 - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    Apr 16, 2018

    added 3 commits

    • 955df15d...f1b1501f - 2 commits from branch GNOME:master
    • d6d09fd3 - ui: Theme lookup should respect XDG_DATA_DIRS

    Compare with previous version

    added 3 commits <ul><li>955df15d...f1b1501f - 2 commits from branch <code>GNOME:master</code></li><li>d6d09fd3 - ui: Theme lookup should respect XDG_DATA_DIRS</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/70/diffs?diff_id=5803&start_sha=955df15d8ae63a6c632c00b94e92749153052da4)
    Toggle commit list
  • Marco Trevisan @3v1n0

    merged

    Apr 16, 2018

    merged

    merged
    Toggle commit list
  • Marco Trevisan @3v1n0

    mentioned in commit 3v1n0/gnome-shell@7128763f

    Apr 17, 2018

    mentioned in commit 3v1n0/gnome-shell@7128763f

    mentioned in commit 3v1n0/gnome-shell@7128763f8fd42bf59baf993dad8b20219addd6b7
    Toggle commit list
  • Marco Trevisan @3v1n0

    mentioned in commit 3v1n0/gnome-shell@8823f8f3

    Apr 18, 2018

    mentioned in commit 3v1n0/gnome-shell@8823f8f3

    mentioned in commit 3v1n0/gnome-shell@8823f8f33505f7505b9d9a8d476eb580bb1b4674
    Toggle commit list
  • Marco Trevisan @3v1n0

    mentioned in commit 3v1n0/gnome-shell@3d25821e

    Apr 18, 2018

    mentioned in commit 3v1n0/gnome-shell@3d25821e

    mentioned in commit 3v1n0/gnome-shell@3d25821e286d43fd30a7a4b08ac7278a0d1ca4c0
    Toggle commit list
  • Write
  • Preview
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment
Assignee
No assignee
Assign to
None
Milestone
None
Assign milestone
Time tracking
0
Labels
None
Assign labels
  • View project labels
Reference: GNOME/gnome-shell!70