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

inori: init module #6289

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

inori: init module #6289

wants to merge 1 commit into from

Conversation

miku4k
Copy link

@miku4k miku4k commented Jan 8, 2025

Description

This adds a module to configure Inori, a MPD client written in Rust.

This depends on NixOS/nixpkgs#371762 getting merged first. Tests are expected to fail since the package doesnt exist yet.

This also depends on features that are expected to come out in the upcoming 0.2.3 release (should be within the next few days).

CC: @eshrh @stephen-huan

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

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

  • 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.

Maintainer CC

modules/programs/inori.nix Outdated Show resolved Hide resolved
@eshrh
Copy link

eshrh commented Jan 9, 2025

thanks!! this will be very useful

@eshrh
Copy link

eshrh commented Jan 12, 2025

Hi! the nixpkgs pr was merged. @stephen-huan has a pending review and some improvements but can't get to his computer or log in until monday, so please keep this drafted until then.

Copy link

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay; didn't have access to my laptop over the weekend.

(thanks for the PR, by the way!)

modules/lib/maintainers.nix Show resolved Hide resolved
modules/programs/inori.nix Outdated Show resolved Hide resolved
modules/programs/inori.nix Outdated Show resolved Hide resolved
modules/programs/inori.nix Show resolved Hide resolved
modules/programs/inori.nix Show resolved Hide resolved
tests/modules/programs/inori/full-config.nix Outdated Show resolved Hide resolved
tests/modules/programs/inori/default-config.nix Outdated Show resolved Hide resolved
tests/modules/programs/inori/default-config.nix Outdated Show resolved Hide resolved
tests/modules/programs/inori/full-config.toml Outdated Show resolved Hide resolved
tests/modules/programs/inori/full-config.toml Outdated Show resolved Hide resolved
right = "l";
top = [ "g g" "<home>" ];
bottom = [ "G" "<end>" ];
quit = [ "<esc>" "q" ];

Choose a reason for hiding this comment

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

Suggested change
quit = [ "<esc>" "q" ];
quit = [ "<escape>" "q" ];

From eshrh/inori@0e371e3, as <esc> is inconsistent with the rest of the special keys (which are full words).

Copy link
Author

Choose a reason for hiding this comment

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

i would wait till thats in a release

Choose a reason for hiding this comment

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

the keybind arrays (eshrh/inori@b6f7b5d) are also blocked on a new release, right?

should be pretty fast as @eshrh just needs to tag a new release on github/cargo and on the nixpkgs side we can merge without approval using the merge bot.

Copy link
Author

Choose a reason for hiding this comment

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

the keybind arrays (eshrh/inori@b6f7b5d) are also blocked on a new release, right?

right completely forgot about that

@stephen-huan
Copy link

stephen-huan commented Jan 13, 2025

Ideally home-manager modules would be completely typesafe, that is, an invalid configuration would lead to a failed build. Of course practically, it's often not worth it because of option churn & effort to make the nix module. But here we can work closely with upstream, so I think it's worth being a bit more semantic than just wrapping toml.

Below is a prototype that defines the new options keybindingSet, keybindings, and theme and is fully typechecked against the upstream specification. This leads to full-config.nix looking like the following instead. Note that this is fully backwards compatible with the current module, as settings is an escape hatch which overrides the other options.

home-manager config
programs.inori = {
  enable = true;
  keybindingSet = "dvorak";
  keybindings = {
    toggle_playpause = [
      { character = "p"; }
      { character = "<space>"; }
    ];
    next_song = [
      { character = ">"; }
      {
        modifier = "C";
        character = "n";
      }
    ];
    previous_song = [
      { character = "<"; }
      {
        modifier = "C";
        character = "p";
      }
    ];
    seek = [ { character = "<right>"; } ];
    seek_backwards = [ { character = "<left>"; } ];

    up = [ { character = "k"; } ];
    down = [ { character = "j"; } ];
    left = [ { character = "h"; } ];
    right = [ { character = "l"; } ];
    top = [
      [
        { character = "g"; }
        { character = "g"; }
      ]
      { character = "<home>"; }
    ];
    bottom = [
      { character = "G"; }
      { character = "<end>"; }
    ];
    quit = [
      { character = "<escape>"; }
      { character = "q"; }
    ];
  };
  theme = {
    status_artist.fg = "#fab387";
    status_album.fg = "#89b4fa";
    status_title = {
      fg = "#cba6f7";
      add_modifier = [
        "BOLD"
        "ITALIC"
      ];
    };
    album.fg = "#89b4fa";
    playing.fg = "#a6e3a1";
  };
  settings.seek_seconds = 10;
};
home-manager module
{ lib, config, pkgs, ... }:

let
  inherit (lib)
    mkOption mkEnableOption mkPackageOption literalExpression mkIf hm types
    maintainers;

  cfg = config.programs.inori;

  tomlFormat = pkgs.formats.toml { };

  keybindingSetType = types.enum [ "dvorak" "qwerty" ];

  keyModifierType = types.enum [ "C" "M" "S" "C-M" "" ];
  keySingleCharacterType = types.strMatching ".";
  keySpecialType = types.enum [
    "<space>"
    "<tab>"
    "<escape>"
    "<backspace>"
    "<delete>"
    "<up>"
    "<down>"
    "<left>"
    "<right>"
    "<enter>"
    "<home>"
    "<end>"
  ];
  keyCharacterType = types.either keySingleCharacterType keySpecialType;

  keybindType = types.submodule {
    options = {
      modifier = mkOption {
        type = types.nullOr keyModifierType;
        default = "";
        description = ''
          Modifier keys.
        '';
      };
      character = mkOption {
        type = types.nullOr keyCharacterType;
        default = null;
        description = ''
          Character to bind.
        '';
      };
    };
  };
  keystrType = types.either (types.listOf keybindType) keybindType;
  # can't include keystrType because of parsing ambuigity
  keybindingType = types.listOf keystrType;

  # https://docs.rs/ratatui/latest/ratatui/style/enum.Color.html#variants
  # this is an underapproximation, but let's be a bit normative
  colorANSIType = types.enum [
    "Reset"
    "Black"
    "Red"
    "Green"
    "Yellow"
    "Blue"
    "Magenta"
    "Cyan"
    "Gray"
    "DarkGray"
    "LightRed"
    "LightGreen"
    "LightYellow"
    "LightBlue"
    "LightMagenta"
    "LightCyan"
    "White"
  ];
  colorHexType = types.strMatching "#[[:xdigit:]]{6}";
  colorType = types.oneOf [ colorANSIType colorHexType types.ints.u8 ];

  colorModifierType = types.listOf (types.enum [
    "BOLD"
    "DIM"
    "ITALIC"
    "UNDERLINED"
    "SLOW_BLINK"
    "RAPID_BLINK"
    "REVERSED"
    "HIDDEN"
    "CROSSED_OUT"
  ]);

  themeType = types.submodule {
    options = {
      fg = mkOption {
        type = types.nullOr colorType;
        default = null;
        description = ''
          Foreground color.
        '';
      };
      bg = mkOption {
        type = types.nullOr colorType;
        default = null;
        description = ''
          Background color.
        '';
      };
      add_modifier = mkOption {
        type = types.nullOr colorModifierType;
        default = null;
        description = ''
          Modifiers to add.
        '';
      };
      sub_modifier = mkOption {
        type = types.nullOr colorModifierType;
        default = null;
        description = ''
          Modifiers to remove.
        '';
      };
    };
  };
in {
  meta.maintainers = [ hm.maintainers.lunahd maintainers.stephen-huan ];

  options.programs.inori = {
    enable = mkEnableOption "inori";

    package = mkPackageOption pkgs "inori" { };

    keybindingSet = mkOption {
      type = types.nullOr keybindingSetType;
      default = null;
      example = literalExpression ''
        "qwerty"
      '';
      description = ''
        Keybinding set.
      '';
    };

    keybindings = mkOption {
      type = types.attrsOf keybindingType;
      default = { };
      example = literalExpression ''
        {
          toggle_playpause = [{ character = "p"; } { character = "<space>"; }];
          next_song = [
            { character = ">"; }
            { modifier = "C"; character = "n"; }
          ];
          previous_song = [
            { character = "<"; }
            { modifier = "C"; character = "p"; }
          ];
          seek = [{ character = "<right>"; }];
          seek_backwards = [{ character = "<left>"; }];
        };
      '';
      description = ''
        Keybindings in Emacs format.

        See <https://github.com/eshrh/inori/blob/master/CONFIGURATION.md#keybindings> for available options.
      '';
    };

    theme = mkOption {
      type = types.attrsOf themeType;
      default = { };
      example = literalExpression ''
        {
          status_artist.fg = "#fab387";
          status_album.bg = 255;
          status_title = {
            fg = "LightCyan";
            add_modifier = [ "BOLD" "ITALIC" ];
          };
        };
      '';
      description = ''
        Theme in Ratatui format.

        See <https://github.com/eshrh/inori/blob/master/CONFIGURATION.md#theme> for available options.
      '';
    };

    settings = mkOption {
      type = tomlFormat.type;
      default = { };
      example = literalExpression ''
        {
          seek_seconds = 10;
          dvorak_keybindings = true;
        }
      '';
      description = ''
        Extra configuration written to {file}`$XDG_CONFIG_HOME/inori/config.toml`.

        See <https://github.com/eshrh/inori/blob/master/CONFIGURATION.md> for available options.
      '';
    };
  };

  config = let
    keybindToString = keybind:
      "${if (keybind.modifier != "") then
        keybind.modifier + "-" + keybind.character
      else
        keybind.modifier + keybind.character}";
    headOrId = x: if builtins.length x == 1 then builtins.head x else x;
    keybindings = builtins.mapAttrs (_: command:
      headOrId (map (keystr:
        (if (builtins.isList keystr) then
          (lib.concatMapStringsSep " " keybindToString keystr)
        else
          keybindToString keystr)) command)) cfg.keybindings;
    theme = builtins.mapAttrs
      (_: attrs: lib.filterAttrs (_: value: value != null) attrs) cfg.theme;
    finalConfig = (lib.optionalAttrs (cfg.keybindingSet != null) {
      "${cfg.keybindingSet}_keybindings" = true;
    }) // (lib.optionalAttrs (keybindings != { }) { inherit keybindings; })
      // (lib.optionalAttrs (theme != { }) { inherit theme; }) // cfg.settings;
  in mkIf cfg.enable {
    home.packages = [ cfg.package ];

    xdg.configFile."inori/config.toml" = mkIf (finalConfig != { }) {
      source = tomlFormat.generate "config.toml" finalConfig;
    };
  };
}

At the minimum, theme and keybindingSet are "zero-cost" abstractions (i.e. you can just lift settings.theme to theme) that I think would be nice to have. Admittedly, the keybindings interface is more verbose than I'd like, but again there's the settings.keybindings escape hatch for a concise string representation (at the cost of typechecking, of course).

There are some limitations i.e. the use of enum precludes case-insensitivity in key modifiers, color ansi names, and color modifiers, for example. I'm not sure what the best interface for this is. In general the interface is an underapproximation, i.e. it rules out valid configurations in the interest of normalization. Perhaps this is too aggressive.

@miku4k
Copy link
Author

miku4k commented Jan 13, 2025

having an option for everything is discouraged https://nix-community.github.io/home-manager/index.xhtml#sec-guidelines-valuable-options
i considered lifting theme and keybindings out of settings but home-manager is not my project so i dont decide how things should be done

@miku4k miku4k force-pushed the master branch 2 times, most recently from 4b1e102 to 3c1d25c Compare January 13, 2025 21:43
@stephen-huan
Copy link

having an option for everything is discouraged

I didn't add mpd_address and seek_seconds for precisely this reason---the difference between ad-hoc configuration options and keybinding / themes is that they generalize many individual options so there's extra leverage (i.e. all possible commands and styles, respectively). This isn't so much of a hard-and-fast rule but more of a suggestion, see e.g. termite, i3, picom, etc. It's more a trade-off of whether you believe the maintenance burden is worth the utility. Again, since we have a close connection to upstream I think we can be a bit more coupled than would be otherwise best practices.

Keybinding set is something I think almost every user will set, unless they want to use the arrow keys and <home> / <end> for navigation. Theming I think will also be relatively common, and there's almost no overhead.

I don't feel too strongly about this but I think it'd be nice to have---otherwise I don't see much point in a home-manager module if it's just exposing toml straight-through (with no additional options).

@miku4k
Copy link
Author

miku4k commented Jan 15, 2025

hey sorry for not responding earlier
i thought about it some more and im fine with lifting keybindings and theme (and maybe keybindSet) to their own options, ill get on it tomorrow

@stephen-huan
Copy link

stephen-huan commented Jan 15, 2025

Oh no worries at all, there's no rush. We're blocked on a new inori release (then for the nixpkgs bot to pick it up, then merging, then for it to hit nixos-unstable, then for home-manager to update nixos-unstable) anyways.

keybindingSet is much easier to implement in the module, easier to use from a config, and probably more useful than keybindings in my opinion. (The type is just types.nullOr (types.enum [ "dvorak" "qwerty" ])).

If you didn't see, there is a working prototype in #6289 (comment) under the detail "home-manager module".

Again, thanks for your work and apologies for the back-and-forth nitpicking!

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.

4 participants