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

autorandr: configModule.extraConfig

Open zackattackz opened this issue 1 year ago • 5 comments

Description

Add an option to programs.autorandr's configModule to allow arbitrary extra config lines.

No option exists for adding arbitrary key/values to generated autorandr profile config, as is common in other nix modules. This commit adds one.

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

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

Maintainer CC

@uvNikita

zackattackz avatar May 05 '24 19:05 zackattackz

@uvNikita Hello no worries on this but just wanted to ping you if you have the time for a review. I need this option for some device specific config that is not included in the regular config options. I'm sure having extraConfig can be useful to others who need to add something to their config quick and dirty.

Thank you!

zackattackz avatar Jun 19 '24 17:06 zackattackz

Makes sense to have possibility to add extra options.

If the options are always in the format of key value, maybe it makes sense to add settings option instead, in line with rfcs42?

uvNikita avatar Jun 19 '24 17:06 uvNikita

You can see implementation example in other modules, e.g. https://github.com/nix-community/home-manager/blob/d7830d05421d0ced83a0f007900898bdcaf2a2ca/modules/programs/i3status.nix#L30

uvNikita avatar Jun 19 '24 17:06 uvNikita

@uvNikita Good point. I am not sure of the config format exactly for autorandr, but I can investigate! If it is strictly keys/values I can try to make settings. But if it turns out more complicated, or if it also allows flag type options where it's just a key and no value, then would extraConfig be fine then?

zackattackz avatar Jun 19 '24 18:06 zackattackz

@zackattackz I think so. Thank you for looking into this and sorry for a late review :)

uvNikita avatar Jun 19 '24 18:06 uvNikita

Hi @uvNikita

Sorry for the lack of response here. I appreciate your review.

I looked into the config file format and it just seems to be a bespoke parser: https://github.com/phillipberndt/autorandr/blob/770dc86bec9d20bbe22b596aad9aa1520c966720/autorandr.py#L505

It can be either keys/values pairs or also just independent keys ("off" for example).

I think this can be represented as a settings type of attrsOf (either str bool)

so for example:

config.autorandr.profiles.${profile}.config.output1.settings = {
  "someBoolConfig" = true;
};
config.autorandr.profiles.${profile}.config.output2.settings = {
  "somekey" = "somevalue";
};

Which would generate config like:

output output1
someBoolConfig

output output2
somekey someval

I think this wouldnt be too hard to implement?

But honestly, because I couldn't find a solid definition of the config format, and because the autorandr code is kind of a lot to take in, I could be missing some considerations here.

I feel like extraConfig would be easier to keep and maintain in the long run, though I do understand settings is preferred generally.

If you'd like, I can implement settings as described above (or some other way you see fit).

Or I can amend with the suggestion you made for extraConfig to merge that instead?

zackattackz avatar Sep 07 '24 12:09 zackattackz

Since it's a custom format, I don't mind if we keep your original suggestion of adding extraConfig option. Another one can also be added later if need arises.

uvNikita avatar Sep 09 '24 09:09 uvNikita

OK! Just lmk if anything else is needed, no rush of course.

zackattackz avatar Sep 10 '24 13:09 zackattackz

Sorry forgot about those suggestions. Added them in, just needed a slight difference from your last suggestion.

zackattackz avatar Sep 10 '24 14:09 zackattackz

Amended with the requested changes 🫡

zackattackz avatar Sep 11 '24 00:09 zackattackz

@rycee Do you mind merging this one? I can't merge until test succeed, but doesn't look like this is related?

uvNikita avatar Sep 11 '24 18:09 uvNikita

Merged now!

rycee avatar Sep 13 '24 07:09 rycee

Thanks for the review :)

zackattackz avatar Sep 14 '24 00:09 zackattackz