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

thunderbird: fix missing icon issue #107016

Merged
merged 1 commit into from Dec 29, 2020

Conversation

tobiasBora
Copy link
Contributor

@tobiasBora tobiasBora commented Dec 16, 2020

Motivation for this change

Since the recent update, the icon of thunderbird was not showing up in the systray. The reason is that the code refers to $out in the makeDesktopItem derivation, which was pointing to the folder created by makeDesktopItem instead of the thunderbird folder.

Moreover, the old code hardcoded the path to the icon, which would make it not changeable by any theme.

Maintainers for a potential review: @lovesegfault @eelco @pierron @vcunat @taku0

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nh2
Copy link
Contributor

nh2 commented Dec 17, 2020

the icon of thunderbird was not showing up in the systray

@tobiasBora How do you get Thunderbird to show up in the systay? In other words, how does one test this change?

Don't you need a plugin to get a Thunderbird systray on Linux as described on http://kb.mozillazine.org/Minimize_to_system_tray_%28Thunderbird%29?

@tobiasBora
Copy link
Contributor Author

Oh sorry, I meant the task bar. Basically without this commit I don't have these icons:
image

The reason is that makeDesktopItem creates a new deviation containing only the .desktop files, so the $out that was in the line icon = "$out/lib/thunderbird/chrome/icons/default/default256.png"; points to the new (nearly empty) folder created by makeDesktopItem instead of the one where thunderbird is installed.

So to test, one solution is to clone this PR, and install it with nix-env (it you don't install it then the icons won't be made available since you need to have the appropriate XDG_DATA_DIR set, so should look like (maybe there is a faster solution, I'm curious if there is one):

$ export NIXPKGS=/tmp/nixpkgs # Import to export for nix-env after
$ git clone https://github.com/NixOS/nixpkgs/ $NIXPKGS
$ cd $NIXPKGS
$ git fetch origin pull/107016/head:2020_thunderbird_add_icon
$ git checkout 2020_thunderbird_add_icon
$ nix-env -f $NIXPKGS -iA thunderbird

@taku0
Copy link
Contributor

taku0 commented Dec 19, 2020

It seems reasonable to me. The firefox package already uses an abstract icon name.

Should we copy all icons in chrome/icons/default/? The firefox package copies default16.png, ..., and default128.png.
We should also fix thunderbird-bin.

@andersk
Copy link
Contributor

andersk commented Dec 28, 2020

While it would be reasonable for thunderbird-bin to do the same thing, it doesn’t currently have the missing icon issue, since it doesn’t use makeDesktopItem.

Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

@tobiasBora Ah, I see. I was asking because in some environments it seems to already work, e.g. in my xfce4-appfinder I get even without this PR:

image

I don't know why it works there though.

In any case, this seems reasonable to me, so you decide whether you want to add more icons or not.

@lovesegfault lovesegfault merged commit 0ca60d3 into NixOS:master Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants