Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adwaita-icon-theme: 46.0 -> 46.2 #316416

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Jun 1, 2024

Description of changes

https://gitlab.gnome.org/GNOME/adwaita-icon-theme/-/compare/46.0...46.2

This marks the theme hidden and inherits from AdwaitaLegacy.
The AdwaitaLegacy theme contains icons required by icon-naming-spec that were removed from Adwaita icon theme proper few releases back.
Try installing adwaita-icon-theme-legacy in addition to gnome.adwaita-icon-theme if some apps are missing icons.
See https://gitlab.gnome.org/GNOME/adwaita-icon-theme/-/issues/288 for more info.

Closes #315451

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@JohnRTitor
Copy link
Contributor

I would suggest adding both icon themes to the list of default installation of GNOME.

adwaita-icon-theme is enabled by default on GNOME currently and apps that rely on DEs default icon theme like Kate and Dolphin, see poor experience of users. Ideally GNOME should fix it themselves, but that's not gonna be happening as you can see in the upstream thread.

GNOME desktop itself does not make it clear that one needs to install the "legacy" icon theme.

@bobby285271
Copy link
Member

Per icon-theme-spec adwaita-icon-theme should probably just propagate adwaita-icon-theme-legacy so the hicolor-icon-theme setup-hook picks it up, though I don't know if GNOME devs reach agreement on it 🤷

@jtojnar
Copy link
Member Author

jtojnar commented Jun 2, 2024

It is not clear to me what marking the theme as Hidden will actually do in different libraries and apps. According to the spec, it is supposed to hide it from selection but what should a settings app display in the case such theme is selected/enabled by default. Looks like GNOME Tweaks, at least, only checks for presence of index.theme file so Hidden key is ignored.

@JohnRTitor JohnRTitor mentioned this pull request Jun 25, 2024
13 tasks
@JohnRTitor JohnRTitor changed the title gnome.adwaita-icon-theme: 46.0 → 46.2 gnome.adwaita-icon-theme: 46.0 → 46.2; adwaita-icon-theme-legacy: init at 46.2 Jun 25, 2024
@JohnRTitor
Copy link
Contributor

It is not clear to me what marking the theme as Hidden will actually do in different libraries and apps.

Even though it is marked as Hidden, GNOME apps continue to use it. Apps like KDE's Kate, which rely on a theme adhering to the theme, have missing icons due to no other icon themes available to fallback to.

According to the spec, it is supposed to hide it from selection but what should a settings app display in the case such theme is selected/enabled by default. Looks like GNOME Tweaks, at least, only checks for presence of index.theme file so Hidden key is ignored.

GNOME ignores the spec, and does not implement it.

This is already an issue in GNOME 46 according to #315451 (comment), so let's not delay it any further and merge this.

I still think on the distro side we should ship adwaita-icon-theme-legacy by default so that issues like these never occur.

@JohnRTitor JohnRTitor marked this pull request as ready for review June 25, 2024 05:18
Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's merge this

@jtojnar
Copy link
Member Author

jtojnar commented Jun 25, 2024

This is already an issue in GNOME 46 according to #315451 (comment), so let's not delay it any further and merge this.

This has been an issue long before GNOME 46 – in fact, GNOME 46 was actually one of the few releases that did not remove any icons. So I do not think there is much point in rushing it now.

GNOME ignores the spec, and does not implement it.

There are multiple points where spec can be implemented: on “supplier” side (in icon themes) and on consumer side in apps (through GTK or ¿Qt/KDE libs?).

I am mostly interested in how the latter behave in the absence of inherited theme. The spec does not say anything on that topic.

I still think on the distro side we should ship adwaita-icon-theme-legacy by default so that issues like these never occur.

We could do that for gnome.nix but we probably do not want that globally (e.g. in config/xdg/icons.nix).

And there is also the question of adwaita-icon-theme in buildInputs, which I still do not fully understand. We might need the propagation as Bobby suggests.

Copy link
Contributor

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I think this PR should probably be targeting the staging branch rather than master.

@jtojnar jtojnar marked this pull request as draft July 9, 2024 21:50
@JohnRTitor JohnRTitor mentioned this pull request Jul 16, 2024
13 tasks
@JohnRTitor
Copy link
Contributor

LGTM, although I think this PR should probably be targeting the staging branch rather than master.

Not that many rebuilds so it could skip staging.

@github-actions github-actions bot removed the 6.topic: GNOME GNOME desktop environment and its underlying platform label Jul 16, 2024
@JohnRTitor
Copy link
Contributor

@jtojnar if this adwaita-icon-theme update is blocked on something, can we split the legacy theme part to a seperate PR and merge that? It would not break anything AFAIK.

@JohnRTitor JohnRTitor added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Jul 17, 2024
@Pandapip1
Copy link
Contributor

Any updates here?

@github-actions github-actions bot removed the 6.topic: GNOME GNOME desktop environment and its underlying platform label Aug 28, 2024
@JohnRTitor JohnRTitor changed the title gnome.adwaita-icon-theme: 46.0 → 46.2; adwaita-icon-theme-legacy: init at 46.2 gnome.adwaita-icon-theme: 46.0 → 46.2 Aug 28, 2024
@JohnRTitor JohnRTitor changed the title gnome.adwaita-icon-theme: 46.0 → 46.2 adwaita-icon-theme: 46.0 -> 46.2 Aug 28, 2024
@JohnRTitor JohnRTitor added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Aug 28, 2024
@Pandapip1
Copy link
Contributor

Is there any reason that this PR is still a draft? The changes look pretty trivial to me.

@jtojnar
Copy link
Member Author

jtojnar commented Aug 29, 2024

See above #316416 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Package request: gnome.adwaita-icon-theme-legacy
5 participants