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

aerc: improve module

Open lukasngl opened this issue 2 years ago • 1 comments

Follow-up to #3070.

  • Added empty lines after definitons
  • Improved warning realted to extraConfig.general.unsafe-accounts-conf = true;
  • Only generate files if they would have content, i.e. the respective config has non-empty values
  • Let values from <account>.aerc.extraAccounts take precedence over all derived values

lukasngl avatar Aug 16 '22 11:08 lukasngl

is it ready for merge? then you have to change from draft

luxus avatar Aug 20 '22 16:08 luxus

I have the problem, that home-manager creates an empty binds.conf, effectively disabling all keybindings. Does this PR fix that issue?

Edit: A similar problem exists for aerc.conf when setting programs.aerc.extraConfig.general.unsafe-accounts-conf = true. aerc.conf is created with only the line unsafe-accounts-conf = true, disabling default filters for e.g. text/plain.

vbrandl avatar Nov 11 '22 16:11 vbrandl

I have the problem, that home-manager creates an empty binds.conf, effectively disabling all keybindings. Does this PR fix that issue? Yes, e36acb9 should adress this issue.

Edit: A similar problem exists for aerc.conf when setting programs.aerc.extraConfig.general.unsafe-accounts-conf = true. aerc.conf is created with only the line unsafe-accounts-conf = true, disabling default filters for e.g. text/plain. If your config was empty, the module (with this PR merged) would not try to create the aerc.conf.

lukasngl avatar Nov 11 '22 17:11 lukasngl

I think even with an non-empty config, the default values should also be written to aerc.conf and binds.conf.

Also the accounts in accounts.conf are currently sorted alphabetically. The first account in accounts.conf is opened by default when starting aerc. Placing the primary account on top would make it the default mailbox when opening aerc.

I also just sent a patch (https://lists.sr.ht/~rjarry/aerc-devel/patches/36790) to make the supplied filters work nice with NixOS.

vbrandl avatar Nov 12 '22 11:11 vbrandl

I think even with an non-empty config, the default values should also be written to aerc.conf and binds.conf.

Well, that would be nice. How would you do it?

Also the accounts in accounts.conf are currently sorted alphabetically. The first account in accounts.conf is opened by default when starting aerc. Placing the primary account on top would make it the default mailbox when opening aerc.

Good point, fixed in a096894e.

I also just sent a patch (https://lists.sr.ht/~rjarry/aerc-devel/patches/36790) to make the supplied filters work nice with NixOS.

The patch probably won't be accepted, see for example nix-shell(1) man page:

The lines starting with #! nix-shell specify nix-shell options (see above). Note that you cannot write #! /usr/bin/env nix-shell -i ... because many operating systems only allow one argument in #! lines.

You could run the filters as ${pkgs.awk}/bin/awk -f ${pkgs.aerc}/share/aerc/filters/colorize.

lukasngl avatar Nov 14 '22 20:11 lukasngl

I think even with an non-empty config, the default values should also be written to aerc.conf and binds.conf.

Well, that would be nice. How would you do it?

While it isn't pretty, defining a set for the default binds.conf and aerc.conf and merging it with the user supplied settings might work...

vbrandl avatar Nov 16 '22 16:11 vbrandl

I think even with an non-empty config, the default values should also be written to aerc.conf and binds.conf. I guess default values for aerc.conf would not be needed, since they actually use defaults if no config was supplied.

While it isn't pretty, defining a set for the default binds.conf and aerc.conf and merging it with the user supplied settings might work... I don't think this is feasible, since a statet requirement for modules is forwards- and backwards-compatibility. So if the names of commands change, the default set would only be compatible with either version.

Copying the default config as a text file and merging it, is imho infeasible since the section for the global binding has to be the first section and without a section header. So merging those would require minor hack.

Secondly, Why not use nix/home-manager to configure your binds? If you do not want to translate all the default bindings into nix, copy the default bindings and edit them in your hm-config repo, e.g.:

{
	programs.aerc = { ... };
	home.file.".config/aerc/binds.conf".source = ./binds.conf;
}

One could probably make that a feature of the module, but this PR should imho be merged regardless of this. So maybe open a new Isssue/PR?

lukasngl avatar Nov 18 '22 08:11 lukasngl

I agree that anything related to the default configuration is out of scope for this PR.

Just my thoughts:

Not generating binds.conf if the extraBinds configuration is empty solves the problem for the happy path but if one were to configure some extraBinds, a file only containing the defined binds will be created, removing all default bindings. I think at least the key should be renamed to just binds since those binds wouldn't be 'extra' as in 'additional'.

Also since unsafe-accounts-conf must be set, the extraConfig set is never empty, so I end up with aerc.conf with only unsafe-accounts-conf set to true. This means there is no [filters] section and messages cannot be viewed. Having to duplicate the complete configuration for aerc.conf in home-manager only to get the default behaviour seems wierd to me...

I'm not sure how this could be fixed nicely. Making aerc itself fallback to default values for undefined keys in the configuration seems to be the cleanest solution to me.

vbrandl avatar Nov 19 '22 02:11 vbrandl

This may need a rebase on the latest master to get the tests working

sumnerevans avatar Jan 25 '23 22:01 sumnerevans

Hello!

Is there any status on this PR? Unfortunately aerc isn't particularly usable, IMO, without some of this PR's fixes. if it's not just awaiting review I'm happy to continue work on this.

Thanks :slightly_smiling_face:

jleightcap avatar Jun 13 '23 00:06 jleightcap