libinput-gestures: init at 2.32 #34687

nightkr · 2018-02-06T23:53:39Z

Motivation for this change

xf86-input-synaptics is unmaintained, and touchegg doesn't work with
the latest synaptics driver or libinput. Ideally DEs would implement this
themselves, but at least KDE/Plasma doesn't (yet)

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.

peterhoeg · 2018-02-07T09:57:00Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

@@ -0,0 +1,35 @@
+{lib, stdenv, fetchFromGitHub, bash, fakeroot, libinput, xdotool, makeWrapper}:


You don't need bash as you can use stdenv.shell

Please add space after { and before }.

peterhoeg · 2018-02-07T09:57:31Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+    sha256 = "1by6sabx0s8sd9w5675gc26q7yccxnxxsjg4dqlb6nbs0vcg81s7";
+  };
+
+  buildInputs = [ bash fakeroot makeWrapper ];


These should be nativeBuildInputs (and bash should go ref the comment above)

peterhoeg · 2018-02-07T09:57:44Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+  installPhase =
+    ''
+      mkdir -p $out
+      fakeroot -- bash libinput-gestures-setup -d "$out" install


Do you really need fakeroot?

Yes, libinput-gestures-setup bails if it's not executed as root, see https://github.com/bulletmark/libinput-gestures/blob/f3c0e61f5b85073eee3ddfe0216f8dd040ddf64d/libinput-gestures-setup#L145-L148

I would probably just patch that out.

Turns out I was a moron and it's not required. Removing it now.

peterhoeg · 2018-02-07T09:59:03Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+    ''
+      mkdir -p $out
+      fakeroot -- bash libinput-gestures-setup -d "$out" install
+      mv $out/usr/bin $out/bin-unwrapped


When you call makeWrapper/wrapProgram later, it will take care of renaming instead you having to define your own scheme.

I used that at first, but the config files are discovered depending on argv[0]. Using wrapProgram means it will look for {/etc,~/.config}/.libinput-gestures-wrapped.conf, rather than {/etc,~/.config}/libinput-gestures.conf.

wrapProgram takes an --argv SOME_NAME argument that should deal with that.

Looks like wrapProgram already sets --argv0. I tried setting it manually, but that still didn't work.

peterhoeg · 2018-02-07T09:59:29Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+      mkdir -p $out
+      fakeroot -- bash libinput-gestures-setup -d "$out" install
+      mv $out/usr/bin $out/bin-unwrapped
+      mv $out/usr/share $out/share


fixupPhase will handle this for you, you don't need to do it manually

That doesn't seem to happen for me, unless I'm missing something.

peterhoeg · 2018-02-07T10:01:30Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+      rmdir $out/usr
+      mkdir $out/bin
+      sed -i "s,/usr/bin/,,g" $out/share/applications/*.desktop
+      makeWrapper $out/bin-unwrapped/libinput-gestures $out/bin/libinput-gestures --prefix PATH : ${lib.makeBinPath [ libinput xdotool ]}


If you use wrapProgram instead of makeWrapper it will handle the renaming for you.

peterhoeg · 2018-02-07T10:01:51Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+      mkdir $out/bin
+      sed -i "s,/usr/bin/,,g" $out/share/applications/*.desktop
+      makeWrapper $out/bin-unwrapped/libinput-gestures $out/bin/libinput-gestures --prefix PATH : ${lib.makeBinPath [ libinput xdotool ]}
+      makeWrapper $out/bin-unwrapped/libinput-gestures-setup $out/bin/libinput-gestures-setup --prefix PATH : ${lib.makeBinPath [  ]}


What is the point of prefixing with an empty list?

None, I forgot to remove this after finding out that -setup is pretty much useless in NixOS anyway.

peterhoeg · 2018-02-07T10:02:16Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+      makeWrapper $out/bin-unwrapped/libinput-gestures-setup $out/bin/libinput-gestures-setup --prefix PATH : ${lib.makeBinPath [  ]}
+    '';
+
+  meta = {


If you change this to meta = with stdenv.lib; { ...

peterhoeg · 2018-02-07T10:02:32Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+  meta = {
+    homepage = https://github.com/bulletmark/libinput-gestures;
+    description = "Gesture mapper for libinput";
+    license = stdenv.lib.licenses.gpl3Plus;


... you can get rid of the stdenv.lib. part here

peterhoeg · 2018-02-07T10:02:39Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+    homepage = https://github.com/bulletmark/libinput-gestures;
+    description = "Gesture mapper for libinput";
+    license = stdenv.lib.licenses.gpl3Plus;
+    platforms = stdenv.lib.platforms.linux;


nightkr · 2018-02-19T23:05:20Z

@peterhoeg I believe I've dealt with all your feedback now.
The only remaining concern is the manual renaming, which I can't seem to get rid of; even with wrapProgram --argv0 it still thinks that it's called .libinput-gestures-wrapped rather than libinput-gestures.

peterhoeg · 2018-02-20T00:18:24Z

Super, for the last item, just patch this line: https://github.com/bulletmark/libinput-gestures/blob/master/libinput-gestures#L13 instead of using --argv0.

peterhoeg · 2018-02-20T00:38:37Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+      rmdir "$out/usr"
+      mkdir "$out/bin"
+      substituteInPlace "$out/share/applications/libinput-gestures.desktop" --replace "/usr" "$out"
+      chmod +x "$out/share/applications/libinput-gestures.desktop"


.desktop files without a shebang shouldn't be executable.

I agree, but KDE doesn't seem to. If it's not there then running kioclient5 exec result/share/applications/libinput-gestures.desktop gives the following confirmation prompt:

Clicking Continue fails with the following message:

Since /nix/store is read-only. If libinput-gestures.desktop is executable then everything works fine.

FWIW, Firefox seems to install its .desktop file, which also causes it to become marked executable.

I did not know that. Thanks!

Yeah.. I was surprised too when I found out.

peterhoeg · 2018-02-20T00:39:20Z

pkgs/tools/inputmethods/libinput-gestures/default.nix

+
+  nativeBuildInputs = [ makeWrapper ];
+
+  installPhase =


Assuming you don't have to install manually, move the clean-up to the postInstall phase.

Everything after invoking libinput-gestures-setup?

nightkr · 2018-02-20T01:08:05Z

@peterhoeg Done.

peterhoeg · 2018-02-20T12:17:00Z

Have a look at this: peterhoeg@c2d6711

I made some changes:

a) This is a python program so use python3Packages.buildPythonApplication which means the shebang is fixed for us (yours doesn't actually work without python3 in the global environment)

b) Patch the paths to the various programs

c) Patch the installation path to avoid the shuffling from /usr

d) Add a reference to wmctrl

e) Make it look for the default config file in the correct place

f) Drop the reference to xdotool which isn't used

nightkr · 2018-02-20T12:30:08Z

a) Fair enough. I didn't know buildPython{Package,Application} was compatible with packages that didn't use distutils/setuptools. Good to know!

b,c) Fair enough. Personally I prefer wrapping over patching since it doesn't need to be maintained when the underlying package changes.

d) Good catch. Didn't think about that since I don't use libinput-gestures to manage my workspaces.

e) Nice, though we probably want to add a way to override the default config file. Maybe an argument?

f) xdotool is used for the default backwards/forwards gestures, and is the primary way that I use libinput-gestures.

I guess d and f mean that it makes more sense to let the user add more tools to the path themselves (another argument?), than to hardcode a few blessed utilities into the package.

peterhoeg · 2018-02-20T14:07:38Z

a) Fair enough. I didn't know buildPython{Package,Application} was compatible with packages that didn't use distutils/setuptools.

That's what ```format = "other";``` does.

b,c) Fair enough. Personally I prefer wrapping over patching since it doesn't need to be maintained when the underlying package changes.

Patching allows us to use absolute paths to the dependencies instead of accidentally picking up something from the user's path. There is a bit of a maintenance burden, but it makes things more predictable.

d) Good catch. Didn't think about that since I don't use libinput-gestures to manage my workspaces.

I just looked through the code for references to ```run```.

e) Nice, though we probably want to add a way to override the default config file. Maybe an argument?

The application already takes that as an argument.

f) xdotool is used for the default backwards/forwards gestures

I don't see any mentioning of it in the code.

I guess d and f mean that it makes more sense to let the user add more tools to the path themselves (another argument?), than to hardcode a few blessed utilities into the package.

We should include the ones that are necessary for the operation. Which ones are optional?

nightkr · 2018-02-20T19:29:33Z

We should include the ones that are necessary for the operation. Which ones are optional?

The config file allows you to map gestures to arbitrary commands. The default configuration (https://github.com/bulletmark/libinput-gestures/blob/master/libinput-gestures.conf) uses xdotool for three-finger swipes to the left and right, as well as for pinches.

peterhoeg · 2018-02-21T08:37:43Z

The way I read the config file, it uses wmctrl using the _internal command (which is what we substitute for a full path name) for most things as it works on both wayland and X whereas xdotool is X only.

If somebody wants to do more, they can always wrap it accordingly.

peterhoeg · 2018-02-22T02:04:31Z

I just realized that meta.maintainers is missing - you don't want to add yourself as maintainer?

nightkr · 2018-02-22T09:05:58Z

I wasn't sure since I'm not in maintainers.nix, but I suppose I can add myself to there.

peterhoeg · 2018-02-22T09:58:08Z

Sure, just add a separate commit for that.

peterhoeg · 2018-02-26T08:32:53Z

@teozkr, any chance you can get the last bits in within the next couple of days so this can make it into 18.03 ?

nightkr · 2018-02-26T09:50:18Z

Yeah sure, sorry.

…

On 26 February 2018 at 09:32, Peter Hoeg ***@***.***> wrote: @teozkr <https://github.com/teozkr>, any chance you can get the last bits in within the next couple of days so this can make it into 18.03 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34687 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnqaIAWRmXUnAdPRU4TjRWr1-uiFa2Jks5tYmw5gaJpZM4R76tv> .

@peterhoeg

xf86-input-synaptics is unmaintained, and touchegg doesn't work with the latest synaptics driver or libinput. Ideally DEs would implement this themselves, but at least KDE/Plasma doesn't (yet) 0002-paths.patch was contributed by @peterhoeg.

nightkr · 2018-02-26T23:02:42Z

Set myself as maintainer and merged most of your suggestions.

I'm still leaving in xdotool by default since a) it's used by the default configuration and b) most users are presumably still using X11. That said, I made it optional, and added an argument for adding more utilities to $PATH.

Seems to build fine for me, going to try it out on my laptop in a moment and report back with the results.

nightkr · 2018-02-26T23:10:19Z

Can confirm that it works fine on my laptop.

peterhoeg · 2018-02-27T02:03:33Z

Thanks for your patience @teozkr! Looking forward to the next PR. ;-)

nightkr · 2018-02-27T09:31:49Z

Thanks!

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Feb 7, 2018

peterhoeg requested changes Feb 7, 2018

View reviewed changes

nightkr force-pushed the libinput-gestures-2.32 branch from c55bed5 to 66cffda Compare February 7, 2018 11:59

nightkr force-pushed the libinput-gestures-2.32 branch from 66cffda to 8b54d1c Compare February 19, 2018 22:59

GrahamcOfBorg added the 8.has: package (new) label Feb 19, 2018

peterhoeg reviewed Feb 20, 2018

View reviewed changes

nightkr force-pushed the libinput-gestures-2.32 branch from 8b54d1c to d545773 Compare February 20, 2018 01:06

nightkr force-pushed the libinput-gestures-2.32 branch from d545773 to 946f395 Compare February 26, 2018 22:46

nightkr added 2 commits February 26, 2018 23:52

maintainers: add teozkr

c4bd96e

libinput-gestures: init at 2.32

cf5a17e

xf86-input-synaptics is unmaintained, and touchegg doesn't work with the latest synaptics driver or libinput. Ideally DEs would implement this themselves, but at least KDE/Plasma doesn't (yet) 0002-paths.patch was contributed by @peterhoeg.

nightkr force-pushed the libinput-gestures-2.32 branch from 946f395 to cf5a17e Compare February 26, 2018 22:53

peterhoeg merged commit 953b394 into NixOS:master Feb 27, 2018

		@@ -0,0 +1,35 @@
		{lib, stdenv, fetchFromGitHub, bash, fakeroot, libinput, xdotool, makeWrapper}:

libinput-gestures: init at 2.32 #34687

libinput-gestures: init at 2.32 #34687

Conversation

nightkr commented Feb 6, 2018

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightkr commented Feb 19, 2018

peterhoeg commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightkr commented Feb 20, 2018

peterhoeg commented Feb 20, 2018

nightkr commented Feb 20, 2018 • edited

peterhoeg commented Feb 20, 2018 via email

nightkr commented Feb 20, 2018

peterhoeg commented Feb 21, 2018

peterhoeg commented Feb 22, 2018

nightkr commented Feb 22, 2018

peterhoeg commented Feb 22, 2018

peterhoeg commented Feb 26, 2018

nightkr commented Feb 26, 2018 via email

nightkr commented Feb 26, 2018

nightkr commented Feb 26, 2018

peterhoeg commented Feb 27, 2018

nightkr commented Feb 27, 2018

nightkr commented Feb 20, 2018 •

edited