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.

Sign up for GitHub

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

Jump to bottom

screenkey: 0.2 -> 0.9 #34898

Closed
wants to merge 1 commit into from
Closed

screenkey: 0.2 -> 0.9 #34898

wants to merge 1 commit into from

Conversation

vidbina
Copy link
Contributor

@vidbina vidbina commented Feb 12, 2018

Motivation for this change

The former build was sourced from scs3jb/screenkey which has been stale since 2010, meanwhile wavexx's fork has been under development until 2016 so it made sense to update the package which has come a long way since 0.2.

The freedesktop icon-theme-spec indicates that the XDG_DATA_DIRS variable is second in line for dictating the user's icon theme (after HOME/.icons). Therefore the iconPkg is stored as hicolor
(the fallback theme) inside a path that is prefixed to XDG_DATA_DIRS.

  • updated the source from the scs3jb repository to the wavexx repository
  • including an icon-theme in the packages output
  • wrapping screenkey to point it towards the bundled icon theme
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Passes the linter: nixpkgs-lint -f /my/nixpkgs -p 'python2.7-screenkey*'

@vidbina
screenkey: 0.2 -> 0.9
250cd4b 
Changed from scs3jb/screenkey to wavexx/screenkey since scs3jb has been
stale since 2010, meanwhile wavexx's fork has been under development
until 2016.

Based on https://packages.deban.org/wheezy/all/gnome-icon-theme/filelist
I figured that providing a similar hierarchy may make the icon "findable".

Eventually, it became clear from the freedesktop icon-theme-spec that
the XDG_DATA_DIRS variable was second in line for dictating the user's
icon theme. Hence, the iconPkg is stored as hicolor (the fallback theme)
inside a path that is prefixed to XDG_DATA_DIRS.
@vidbina vidbina requested a review from FRidh as a code owner February 12, 2018 15:09
@jtojnar
Copy link
Contributor

jtojnar commented Feb 12, 2018

See also #34167

@grahamc grahamc mentioned this pull request Feb 12, 2018
Open
@grahamc
Copy link
Member

grahamc commented Feb 12, 2018

@GrahamcOfBorg eval

(master had a meta issue, which exposed a bug in OfBorg where it incorrectly reports this PR broke the meta checks: NixOS/ofborg#73)

@Mic92
Copy link
Member

Mic92 commented Feb 12, 2018 •
edited

That pr should preferred over the older one as I can see from the changes, right?

@jtojnar
Copy link
Contributor

jtojnar commented Feb 12, 2018

@Mic92 I find the older one much cleaner, the only thing that is missing is removing the old derivation from python-packages.nix.

Mic92
Mic92 reviewed Feb 12, 2018
View reviewed changes
pkgs/top-level/python-packages.nix
$(mkdir -p $out/share/icons/hicolor; cd ${iconPkg}/share/icons/Tango; cp -R . $out/share/icons/hicolor)
wrapProgram $out/bin/screenkey \
--unset XMODIFIERS \
--prefix XDG_DATA_DIRS : "$out/share"
Copy link
Member

Choose a reason for hiding this comment

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

Would --prefix XDG_DATA_DIRS : ${iconPkg}/share/ also work here?

Copy link
Member

@Mic92 Mic92 Feb 12, 2018 •
edited

Choose a reason for hiding this comment

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

We also have wrapGAppsHook, which does the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 I believe it would. Also, just read wrapGAppsHook 🤦‍♂️... Learning something new every day

@Mic92 Mic92 mentioned this pull request Feb 12, 2018
Merged
8 tasks
@vidbina
Copy link
Contributor Author

vidbina commented Feb 12, 2018

I guess we can just go ahead and close this then 😉. Sorry for the noise. Only grep-ed master... Didn't check out open PR's (2nd time this happens) 🤦‍♂️

@vidbina vidbina mentioned this pull request Feb 12, 2018
Open
7 tasks
@vidbina
Copy link
Contributor Author

vidbina commented Feb 27, 2018

See #34167 which is cleaner. Closing to keep the PR buffer clean 😉

@vidbina vidbina closed this Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vidbina @jtojnar @grahamc @Mic92 @GrahamcOfBorg