home-manager icon indicating copy to clipboard operation
home-manager copied to clipboard

programs.wlr-which-key: init

Open minijackson opened this issue 1 year ago • 10 comments

Description

wlr-which-key is a keymap manager for wlroots-based compositors, inspired by which-key.nvim.

closes #4396

Checklist

  • [x] Change is backwards compatible.

  • [x] Code formatted with ./format.

  • [x] Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • [x] Test cases updated/added. See example.

  • [x] 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

    • [x] Added myself as module maintainer. See example.

Maintainer CC

@mightyiam, @hervyqa any of you interested in becoming co-maintainer of this module?

minijackson avatar Jul 27 '24 20:07 minijackson

Sure, I can be a co-maintainer.

mightyiam avatar Jul 28 '24 01:07 mightyiam

How about doing something like this? (taken from my hm config)

{
  config,
  lib,
  pkgs,
  ...
}:
let
  cfg = config.programs.wlr-which-key;
  yamlFormat = pkgs.formats.yaml { };
in
{
  options.settings.wlr-which-key = {
    enable = lib.mkEnableOption "wlr-which-key";
    settings = lib.mkOption {
      type = yamlFormat.type;
      default = { };
      example =
        lib.literalExpression # nix
          ''
            {
              font = "JetBrainsMono Nerd Font 12";
              background = "#282828cc";
              color = "#ebdbb2";
              border = "#3c3836";
              border_width = 2;
              corner_r = 12;
              padding = 16;
              separator = " -> ";

              anchor = "center"; # One of center, left, right, top, bottom, bottom-left, top-left, etc.
              margin_right = 0; # Only relevant when not center
              margin_bottom = 0;
              margin_left = 0;
              margin_top = 0;
            }
          '';
    };
    menus = lib.mkOption {
      type = lib.types.attrsOf yamlFormat.type;
      default = { };
      example =
        lib.literalExpression # nix
          ''
            {
              default = {
                w = {
                  desc = "WiFi";
                  submenu = {
                    t = {
                      desc = "Toggle";
                      cmd = "rfkill toggle wifi";
                    };
                    c = {
                      desc = "Connect";
                      cmd = "kitty -e nmtui-connect";
                    };
                  };
                };
                l = {
                  desc = "Lock";
                  cmd = "pidof swaylock || swaylock";
                };
              };
            }
          '';
    };
  };

  config = lib.mkIf cfg.enable {
    home.packages = [ pkgs.wlr-which-key ];
    xdg.configFile = lib.attrsets.mapAttrs' (
      name: menu:
      let
        filename = if name == "default" then "config" else name;
      in
      lib.attrsets.nameValuePair "wlr-which-key/${filename}.yaml" {
        source = (yamlFormat.generate "${filename}.yaml" (cfg.settings // { inherit menu; }));
      }
    ) cfg.menus;
  };
}

From wlr-which-key --help, it says that different menus can be opened from different config files. This does that, and keeps the settings the same between them while allowing for defining different menus, e.g. wlr-which-key power, wlr-which-key firefox etc.

LilleAila avatar Jul 28 '24 12:07 LilleAila

@LilleAila aha! Hadn't thought about that particular usecase, thanks! I've opted for a slightly different implementation. Let me know what you think.

minijackson avatar Jul 29 '24 16:07 minijackson

@LilleAila aha! Hadn't thought about that particular usecase, thanks! I've opted for a slightly different implementation. Let me know what you think.

LGTM! But maybe commonSettings should be renamed to just settings, to comply with the naming scheme of most other modules. I guess i could also be a co-maintainer.

LilleAila avatar Jul 29 '24 16:07 LilleAila

But maybe commonSettings should be renamed to just settings, to comply with the naming scheme of most other modules.

I'm not completely sure, this settings option doesn't map to a config file. If some user just does programs.wlr-which-key.settings = { ... }, it wouldn't do anything.

I guess i could also be a co-maintainer.

Awesome! I'll add you

minijackson avatar Jul 29 '24 17:07 minijackson

Hmmm, I've tried adding you @LilleAila but home-manager doesn't seem to know you:

       error: undefined variable 'LilleAila'

       at /home/minijackson/Documents/Dev/dev/github.com/nix-community/home-manager/modules/programs/wlr-which-key.nix:7:46:

            6| in {
            7|   meta.maintainers = with lib.maintainers; [ LilleAila mightyiam minijackson ];
             |                                              ^
            8|

minijackson avatar Jul 29 '24 17:07 minijackson

Hmmm, I've tried adding you @LilleAila but home-manager doesn't seem to know you:

       error: undefined variable 'LilleAila'

       at /home/minijackson/Documents/Dev/dev/github.com/nix-community/home-manager/modules/programs/wlr-which-key.nix:7:46:

            6| in {
            7|   meta.maintainers = with lib.maintainers; [ LilleAila mightyiam minijackson ];
             |                                              ^
            8|

Right, i forgot the PR adding me isn’t merget yet. I think the one in nixpkgs maintainers is though, otherwise it’s okay not to be added.

LilleAila avatar Jul 29 '24 17:07 LilleAila

I think home-manager can use the maintainer list from nixpkgs. If you've added yourself in nixpkgs recently, might be that home-manager's nixpkgs is not recent enough.

minijackson avatar Jul 29 '24 17:07 minijackson

@LilleAila after investigating a bit more, I think this is a "me" issue, since the way I tested it was using import <nixpkgs>. I've added you, and I guess we'll see if this goes well!

minijackson avatar Aug 01 '24 16:08 minijackson

@IldenH I'm not sold on making settings both the configuration for the default file, and also apply them to the other menus. I think it's best to keep one option for common settings, and one option that map to files.

I've reworded the sentence explaining the default file, please tell me if it's still confusing.

I also prefer keeping configs since I feel that with menus it can be confused with the menu setting inside the config.

I agree that programs.wlr-which-key.configs.config feels clunky to write, but the name config.yaml is upstream's choice and I don't think we should hide it.

minijackson avatar Oct 04 '24 13:10 minijackson

This PR seems stale, but I have some feedback that might move things forward.

Packing the menus in a single variable and keeping the settings separate, unless you choose to override the settings for a specific menu is a complicated and confusing layout to configure this package. It seems that this was chosen so that settings can be shared between different menus. The user can do this in their own config by creating a local variable for the config let myStyle = { ... }; in and then using unions to override changed values with style = myStyle // { ... }.

Putting the config that gets called by default into the config variable is intuitive. Also, any extra configs being in extraConfigs would make sense that you need to pass an argument to call those configs explicitly, without the help of a description.

{
  programs.wlr-which-key = 
  let
   myStyle = {
    anchor = "center";
    background = "#282828d0";
    border = "#8ec07c";
    border_width = 2;
    color = "#fbf1c7";
    corner_r = 10;
    font = "JetBrainsMono Nerd Font 12";
    margin_bottom = 0;
    margin_left = 0;
    margin_right = 0;
    margin_top = 0;
    padding = 15;
    separator = " ➜ ";
   };
  in {
  enable = true;
  
  # Default config
  config = {
    style = myStyle
    menu = {
      ...
    };
  };

  # Extra Configs if the user desires
  extraConfigs = {
    firefox = {
      style = myStyle // { border = "#ff0000"; };
      menu = {
         ...
      };
    };
  };
};

Catmaniscatlord avatar Dec 27 '24 23:12 Catmaniscatlord

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.
If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

stale[bot] avatar Mar 28 '25 05:03 stale[bot]

no stale

hervyqa avatar Mar 28 '25 08:03 hervyqa

This PR seems stale, but I have some feedback that might move things forward.

Packing the menus in a single variable and keeping the settings separate, unless you choose to override the settings for a specific menu is a complicated and confusing layout to configure this package. It seems that this was chosen so that settings can be shared between different menus. The user can do this in their own config by creating a local variable for the config let myStyle = { ... }; in and then using unions to override changed values with style = myStyle // { ... }.

Putting the config that gets called by default into the config variable is intuitive. Also, any extra configs being in extraConfigs would make sense that you need to pass an argument to call those configs explicitly, without the help of a description.

{
  programs.wlr-which-key = 
  let
   myStyle = {
    anchor = "center";
    background = "#282828d0";
    border = "#8ec07c";
    border_width = 2;
    color = "#fbf1c7";
    corner_r = 10;
    font = "JetBrainsMono Nerd Font 12";
    margin_bottom = 0;
    margin_left = 0;
    margin_right = 0;
    margin_top = 0;
    padding = 15;
    separator = " ➜ ";
   };
  in {
  enable = true;
  
  # Default config
  config = {
    style = myStyle
    menu = {
      ...
    };
  };

  # Extra Configs if the user desires
  extraConfigs = {
    firefox = {
      style = myStyle // { border = "#ff0000"; };
      menu = {
         ...
      };
    };
  };
};

Yeah, I was reviewing module a bit and this did stand out as a weird UX. We should strive for as much of a nix -> config format transparency as possible. If the desire is just to deduplicate code in a user's config, they can leverage variables for that.

khaneliman avatar Apr 21 '25 17:04 khaneliman

Sorry all, I've lost interest in becoming maintainer of this module. If anyone wants to pick this PR backup, please feel free!

minijackson avatar Sep 03 '25 08:09 minijackson

Sorry, a bit unrelated; is there an alternative to this that is already in home-manager?

mightyiam avatar Sep 13 '25 10:09 mightyiam