Skip to content

hyprland: init#3837

Merged
rycee merged 1 commit intonix-community:masterfrom
fufexan:hyprland
Jul 20, 2023
Merged

hyprland: init#3837
rycee merged 1 commit intonix-community:masterfrom
fufexan:hyprland

Conversation

@fufexan
Copy link
Copy Markdown
Contributor

@fufexan fufexan commented Apr 3, 2023

Description

Add the Hyprland module, ported from
https://github.com/hyprwm/Hyprland/blob/main/nix/hm-module.nix,
which was adapted from the sway module.

CC: @Mic92

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@fufexan fufexan requested a review from rycee as a code owner April 3, 2023 17:44
@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Apr 3, 2023

I'm not sure we want to maintain the module in home-manager if upstream has its own?

@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Apr 3, 2023

Alright, fair. I was just following up on this.

I can close the PR if having it in two places is not suitable.

@FlorianFranzen
Copy link
Copy Markdown
Contributor

@ncfavier So am I expected to track all the upstream repositories of software with nix support to get home-manager integration? That does not really sound feasible to me either.

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented May 9, 2023

I guess? Not sure why that wouldn't be feasible.

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented May 9, 2023

To be clear, I'm not opposed to adding a hyprland module at all, if that's what upstream wants. As long as they maintain their own out-of-tree module, I don't see the point in duplicating code and work. Closing for now.

@ncfavier ncfavier closed this May 9, 2023
@Mic92
Copy link
Copy Markdown
Member

Mic92 commented May 9, 2023

One advantage of the home-manager module would be that it than uses the nixpkgs package while upstream will use its own (which means user have to compile the window manager themself).

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented May 9, 2023

(Just realised @fufexan is upstream... 🤦 guess I'll reopen.)

@ncfavier ncfavier reopened this May 9, 2023
@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented May 9, 2023

I'm unsure why the tests fail on a module that I haven't touched.

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented May 9, 2023

Should be fixed by rebasing.

Copy link
Copy Markdown
Contributor

@FlorianFranzen FlorianFranzen left a comment

Choose a reason for hiding this comment

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

Found a few mostly minor issues. Any reason why plugin support was removed?

Comment thread modules/services/window-managers/hyprland.nix Outdated
Comment thread modules/services/window-managers/hyprland.nix Outdated
Comment thread modules/services/window-managers/hyprland.nix Outdated
Comment thread modules/services/window-managers/hyprland.nix Outdated
@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented May 11, 2023

Any reason why plugin support was removed?

This PR predates the addition of plugin support. I will add it here as well.

@fufexan fufexan force-pushed the hyprland branch 2 times, most recently from 1a1aded to 3449677 Compare May 11, 2023 11:26
'';
};

disableAutoreload = lib.mkOption {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Best to avoid confusion with double negation and such. I would suggest

Suggested change
disableAutoreload = lib.mkOption {
enableAutoreload = lib.mkOption {

with the default being true. That said, is there any occasion where you would not want automatic reload?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added it because of this issue, however I think it's safe to remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Comment on lines +19 to +20
Hyprland package to use. Will override the 'xwayland' and 'nvidiaPatches'
options.
Copy link
Copy Markdown
Member

@rycee rycee May 25, 2023

Choose a reason for hiding this comment

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

Seems a bit confusing, why not have the default be pkgs.hyprland and then add a read-only finalPackage option? Like in the emacs module, for example.

In the config section set

wayland.windowManager.hyprland.finalPackage = cfg.package.override { … }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't even think of this. Thanks, will fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

'';
};

systemdIntegration = lib.mkOption {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When would you not want to use systemd integration? Btw, when enabling the systemd integration I don't see any attempt at shutting down the graphical session when Hyprland exits. Will that happen automatically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it makes sense for users that are on a distro without systemd. But they'd have a hard time using Nix by itself, let alone HM.

No, there's no attempt to shut down the graphical session. I don't know how to do this, but I'm willing to include it if you have clues. Also, it does not automatically shut down.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Without something like exec-exit, which would run a command on shutdown it would be tricky to solve the shutdown entirely within Hyprland. I think it would require wrapping the Hyprland executable and in the wrapper do something like

systemctl --user stop graphical-session.target
systemctl --user stop graphical-session-pre.target
# Wait until the units actually stop.
while [ -n "$(systemctl --user --no-legend --state=deactivating list-units)" ]; do
sleep 0.5
done
${optionalString (cfg.importedVariables != [ ])
("systemctl --user unset-environment "
+ escapeShellArgs cfg.importedVariables)}
on shutdown.

Comment thread modules/services/window-managers/hyprland.nix Outdated
Comment thread modules/misc/news.nix
Comment thread modules/services/window-managers/hyprland.nix
(lib.mdDoc "patching wlroots for better Nvidia support.");

extraConfig = lib.mkOption {
type = lib.types.nullOr lib.types.lines;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you could omit the null case.

Suggested change
type = lib.types.nullOr lib.types.lines;
type = lib.types.lines;

That said, to me it seems like it would be possible to relatively easily create an RFC-42 style settings option instead of extraConfig?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it could work. Currently there's hyprwm/Hyprland#870 which is very big and intricate.

Perhaps adapting lib.generators.toINI could work, but I'm unsure how to actually do it. I've tried

lib: let
  libStr = lib.strings;
  libAttr = lib.attrsets;
in {
  toHyprconf = {
    # format a setting line from key and value
    mkKeyValue ? lib.generators.mkKeyValueDefault {} "=",
    # allow lists as values for duplicate keys
    listsAsDuplicateKeys ? false,
  }: attrsOfAttrs: let
    # map function to string for each key val
    mapAttrsToStringsSep = sep: mapFn: attrs:
      libStr.concatStringsSep sep
      (libAttr.mapAttrsToList mapFn attrs);
    mkSection = sectName: sectValues:
      ''
        ${sectName} {
      ''
      + (lib.generators.toKeyValue {inherit mkKeyValue listsAsDuplicateKeys;} sectValues)
      + ''
        }
      '';
  in
    # map input to hyprconf sections
    mapAttrsToStringsSep "\n" mkSection attrsOfAttrs;
}

but couldn't test it in a repl.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here is a rough and mostly untested attempt:

toHyprconf = { }: attrs: let
  mkSection = n: attrs: ''
    ${n} {
    ${toKeyValue { listsAsDuplicateKeys = true; } attrs}
    }
  '';

  mkField = mkKeyValueDefault {} "=";

  sections = filterAttrs (n: v: isAttrs v) attrs;
  fields = filterAttrs (n: v: !(isAttrs v)) attrs;
in
  concatStringsSep "\n" (
    mapAttrsToList mkSection sections ++ mapAttrsToList mkField fields
  );

This does assume that Hyprland does not care about field order, but I think that is a correct assumption?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That gets us closer, but there are instances of nested sections, for example

input {
  sensitivity = 0.5
  touchpad {
    natural_scroll = true
  }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe something like

toHyprconf = { }: attrs: let
  mkSection = n: attrs: ''
    ${n} {
    ${toHyprconf { } attrs}}
  '';

  mkFields = generators.toKeyValue { listsAsDuplicateKeys = true; };

  sections = filterAttrs (n: v: isAttrs v) attrs;
  fields = filterAttrs (n: v: !(isAttrs v)) attrs;
in
  concatStringsSep "\n" (mapAttrsToList mkSection sections)
  + mkFields fields;

Won't do indentation or anything fancy but I guess good enough to start with.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The hyprland config allows a key to have multiple values. For example

exec-once=eww open bar
exec-once=firefox

bind=SUPER, Return, exec, kitty
bind=SUPER, Return, exec, notify-send "Kitty was started!"

How could we make this work?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's what listsAsDuplicateKeys does

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's really handy then!

Copy link
Copy Markdown
Contributor Author

@fufexan fufexan Jun 17, 2023

Choose a reason for hiding this comment

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

Sorry for the long delay, I've been having exams for the past month.

I just realized we have to handle submaps (nested keybinds) as a special case.

    # window resize
    bind = $mod, S, submap, resize

    submap = resize
    binde = , right, resizeactive, 10 0
    binde = , left, resizeactive, -10 0
    binde = , up, resizeactive, 0 -10
    binde = , down, resizeactive, 0 10
    bind = , escape, submap, reset
    submap = reset

Here, the order of binds matters. Can we simply pass this as key=value (I'm thinking multiline string) or is there a better way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems the multiline string way worked just fine, although it looks a bit ugly.

@fufexan fufexan force-pushed the hyprland branch 4 times, most recently from 638b3c1 to b773241 Compare May 25, 2023 23:00
Comment thread modules/services/window-managers/hyprland.nix Outdated
Comment thread modules/services/window-managers/hyprland.nix Outdated
@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jun 17, 2023

I've tried testing this with my configuration but it does not seem to work in any way. I have the testing branch available here. If anyone wants to test building it (nixos-rebuild build --flake github:fufexan/dotfiles/hyprland-module#io).

Alternatively, here's the rough config file I've used for testing the toHyprconf function (which worked fine). hyprlang2.txt
I load it in a repl and write it in a file with

> :lf nixpkgs
> config = import ./hyprconf2.nix lib
> :b legacyPackages.x86_64-linux.writeText "hyprland.conf" config

@fufexan fufexan force-pushed the hyprland branch 2 times, most recently from 7911d82 to 2f1e776 Compare July 4, 2023 12:22
@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jul 4, 2023

We may be able to remove the xwayland.hidpi option (and the patching), as Hyprland has a new option that does the same thing, called xwayland:force_zero_scaling. Paired with toolkit-specific scaling options, it should achieve even better support for HiDPI. Currently not everything works perfectly, so I will wait for fixes before removing the hidpi option from this module.

@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jul 7, 2023

Actually, it seems force_zero_scaling won't fix all HiDPI issues so we'll keep the patches. Anything else needs to be done for this module?

enableNvidiaPatches =
lib.mkEnableOption "patching wlroots for better Nvidia support";

settings = lib.mkOption {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note, name changed to settings in line with RFC 42.

@rycee
Copy link
Copy Markdown
Member

rycee commented Jul 19, 2023

Looks pretty good! I rebased and added a fixup commit with various cleanups (and added tests). If you can, then please give it a try.

I'll try to finish it all up tomorrow.

@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jul 20, 2023

Thank you! I'll test this right now.

@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jul 20, 2023

I got this error

       error: A definition for option `home-manager.users.mihai.wayland.windowManager.hyprland.settings.input.touchpad' is not of type `atom (null, bool, int, float or string) or a list of them for duplicate keys'. Definition values:
       - In `/nix/store/ml8sy5pirc2yi9d2kzkpw6hw9b87qn6i-source/home/wayland/hyprland/settings.nix':
           {
             scroll_factor = 0.3;
           }

This is the nix code that triggered it

    input = {
      kb_layout = "ro";

      # focus change on cursor move
      follow_mouse = 1;
      accel_profile = "flat";
      touchpad = {
        scroll_factor = 0.3;
      };
    };

Edit: here's my dotfiles branch where I switched to this module. For more options, you can check home/wayland/hyprland/settings.nix.

@rycee
Copy link
Copy Markdown
Member

rycee commented Jul 20, 2023

@fufexan Aha, I wasn't aware that the nesting could be more than one level deep. I've force-pushed an update now that allows arbitrary nesting. Please give it a try.

@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jul 20, 2023

Thank you, works just fine now!

Ported from

  https://github.com/hyprwm/Hyprland/blob/main/nix/hm-module.nix

which was adapted from the sway module.

Co-authored-by: Robert Helgesson <robert@rycee.net>
@rycee rycee merged commit ee56732 into nix-community:master Jul 20, 2023
@rycee
Copy link
Copy Markdown
Member

rycee commented Jul 20, 2023

Great! I've merged it now 🙂

@fufexan
Copy link
Copy Markdown
Contributor Author

fufexan commented Jul 20, 2023

Thank you for all the help, Robert!

@RaySlash
Copy link
Copy Markdown

RaySlash commented Oct 8, 2023

Can this module be backported to 23.05 so that we can use hyprland on nixos stable release with hm.? Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants