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

services.xserver.imwheel: add module #71052

Merged
merged 1 commit into from Dec 13, 2019
Merged

Conversation

@turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Oct 12, 2019

Add IMWheel module.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-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.
Notify maintainers

cc @

@turboMaCk turboMaCk requested a review from Infinisil as a code owner Oct 12, 2019
@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch 2 times, most recently from a0de9e5 to 31272dd Oct 13, 2019
@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch from ac08ec6 to 8472a25 Oct 20, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Oct 20, 2019

rebased. Any idea who to ask for a review @worldofpeace @Infinisil @jhillyerd ?

@jhillyerd
Copy link
Contributor

@jhillyerd jhillyerd commented Oct 20, 2019

I haven't done services yet with NixOS, so I'm probably not the best to review. One comment though: could we make -d mandatory instead of default, so that imwheel never daemonizes itself, and drop the ExecStop=pkill line? (Edit: this would be systemd Type=simple vs Type=forking)

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Oct 20, 2019

I guess that would be an option though I'm not sure if there is not any valid usecases for overriding that flug though I guess there isn't any.

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Oct 20, 2019

@jhillyerd I think I found a problem with this. If -d would be present in options this would not work. You can try to run imwheel -d -d which will start imwheel as a deamon. Same way imwheel -d -d -d works same way as -d so there is probably some negation going on. I personally feel that theit would be better to keep this as is but it's open to discussion.

@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch from 8472a25 to 283454b Oct 22, 2019
@ofborg ofborg bot requested a review from jhillyerd Oct 22, 2019
Copy link
Contributor

@jhillyerd jhillyerd left a comment

approving w/ a couple nit picks.

nixos/modules/services/x11/imwheel.nix Outdated Show resolved Hide resolved
pkgs/tools/X11/imwheel/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/imwheel.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/imwheel.nix Outdated Show resolved Hide resolved
@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch 2 times, most recently from 3a80908 to a8e00d9 Oct 27, 2019
''';
'';
description = '''
Window's translation rules.

This comment has been minimized.

@turboMaCk

turboMaCk Oct 27, 2019
Author Member

I don't honestly even know what is the right terminology to use there. Any suggestion for improvements are welcome.

This comment has been minimized.

@jhillyerd

jhillyerd Dec 11, 2019
Contributor

Maybe Window class translation rules. since that's what is being matched?

This comment has been minimized.

@turboMaCk

turboMaCk Dec 13, 2019
Author Member

Thanks, I like it

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Oct 27, 2019

changes based on feedback are implemented but I would still want to try it out on my desktop to be sure it works.

@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch from a8e00d9 to 8416ea4 Nov 2, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 2, 2019

Ready for next review. Sorry it took so long. I needed to find some time while I'm on a machine that actually has mouse attached. Example configuration I've tested this with:

  services.xserver.imwheel = {
    enable = true;
    rules = {
      "chromium-browser" = ''
        None,      Up,   Button4, 8
        None,      Down, Button5, 8
        Shift_L,   Up,   Shift_L|Button4, 4
        Shift_L,   Down, Shift_L|Button5, 4
        Control_L, Up,   Control_L|Button4
        Control_L, Down, Control_L|Button5
      '';
    };
  };

I've also tested 2 mouses with multiple buttons to make sure default configuration doesn't break things like back and forward buttons.

@ofborg ofborg bot requested a review from jhillyerd Nov 2, 2019
@turboMaCk turboMaCk requested review from Infinisil and worldofpeace Nov 5, 2019

options = mkOption {
type = types.listOf types.str;
default = ["--detach" "--buttons 45" "--kill"];

This comment has been minimized.

@Infinisil

Infinisil Nov 14, 2019
Member

I'd do some changes regarding this:

  • If users set this to e.g. ["--buttons 45"] they would override the --detach option (which actually should be called --no-detach or so anyways), so the service would fork off, which is not as nicely handled anymore then. So I suggest to add --detach unconditionally in the ExecStart to prevent this. I don't see any reason anybody would want to change that anyways.
  • The name options is already an overloaded term from the module system, and with above change, extraOptions is more fitting, which is also the standard naming for such options.
  • Unless I'm missing something, I don't see a reason --buttons 45 should be passed by default. Same with --kill which doesn't make any sense here, or does it? So I'd just make default = [] but put some examples in example = [ "--debug" "--buttons 45" ]

This comment has been minimized.

@turboMaCk

turboMaCk Nov 14, 2019
Author Member

These defaults are based mostly on my experimentation because sadly running IMWheel without flags causes some issues

  • detach is by default true so passing --detach toggles it so it doesn't detach. Which I believe makes more sense when it's ran as a service.
  • kill meas that any already running IMWheel process will be killed when this one starts. I think this also make sense because otherwise result can be unpredictable
  • buttons 45 is passed because without this IMWheel breaks functionality of additional mouse buttons like back and forward (tested with Logitech and MAD Rat)

Using defaults make it still possible for user to easily overwrite those things. This is why there is a pkill https://github.com/NixOS/nixpkgs/pull/71052/files#diff-ea25b843a6681ed1b7711805ec769667R64 so in case --detach is not passed (and so process is detached) IMWheel is still killed when service stops. This is also related to this comment #71052 (comment) and my reaction #71052 (comment).

so TL;DR:

  • --detach is to prevent detaching which imo makes more sense for a service
  • --buttons 45 is to avoid breaking addition mouse buttons with default settings (just enable = true)
  • --kill is to avoid multiple IMWheel processes (whenever started manually or by systemd)
  • pkill is used in cases someone overwrides defaults but doesn't pass --detach or -d

Which this in mind do you think want to change these defaults @Infinisil ?

EDIT: There is also no --no-detach flag available
EDIT: Passing some aruguments twice disables it so:

imwheel -d -d has same effect as imwhell
imwheel -d -d -d has same effect as imwheel -d

This makes it a bit more problematic to make some flags mandatory.

This comment has been minimized.

@turboMaCk

turboMaCk Nov 27, 2019
Author Member

sorry for ping @Infinisil I don't want to be too pushy about this. I'm happy to implement any changes, just want to make sure you still feel we want to do them considering the comment above.

This comment has been minimized.

@jhillyerd

jhillyerd Dec 11, 2019
Contributor

It seems regardless of any changes to the defaults, the rename from options to extraOptions should happen.

This comment has been minimized.

@Infinisil

Infinisil Dec 12, 2019
Member

Hm okay I can see the buttons default being reasonable, but --detach and maybe --kill too can imo be added like

{
  ExecStart = "${pkgs.imwheel}/bin/imwheel" + escapeShellArgs ([
    "--detach"
    "--kill"
  ] ++ cfg.extraOptions);
}

This means even when the user doesn't specify them in extraOptions, they still get added, and the expectation is that users won't add --detach in extraOptions. It should feel like "Extra options in addition to the ones NixOS adds on its own because they always make sense to use"

This comment has been minimized.

@turboMaCk

turboMaCk Dec 13, 2019
Author Member

Thanks @Infinisil for the feedback. I've incorporate this change. just few remarks:

  • I've add space behind bin/imwheel because I don't think escapeShellArgs do that. Let me know if that's not right.
  • There is still the pkill on ExecStop. With these additional changes it might make sense to incorporate even change to Simple type - depends of if we want to try to protect user from shooting him/herself in foot (multiple --detach arguments case) or we want to make assumption default is good and if someone overrides it by combination of extraOptions then it's his/her thing to deal with.
  • I've retested changes with nixos-rebuild -I ....
@turboMaCk turboMaCk requested a review from Infinisil Nov 14, 2019
@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch from 8416ea4 to cbfac1b Dec 5, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Dec 5, 2019

rebased onto master

@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch 5 times, most recently from 0134cec to f6832e3 Dec 11, 2019
@turboMaCk turboMaCk force-pushed the turboMaCk:imwheel-service branch from f6832e3 to 7406c0a Dec 13, 2019
Copy link
Member

@Infinisil Infinisil left a comment

Looking good!

@Infinisil Infinisil merged commit 89eccbf into NixOS:master Dec 13, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@turboMaCk turboMaCk mentioned this pull request Dec 19, 2019
1 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.