rofi: add theme option
#36785
rofi: add theme option
#36785
Conversation
the
Just to get your point right: you'd like to see several more options for |
|
I want an opinion from maintainers about desirability of having the wrapper code inside the main package definition, if I were sure I would speak with more confidence. The goal is to write instead of right? |
|
that's true. I think that the first option is the simpler and more understandable approach. Furthermore I think that having such configuration options available in a package, so a simple |
|
Theme is probably the most used rofi tweak, so I think having a special provision for it is OK. This change allow downstreams or even Nixpkgs to ship a default theme with Rofi, and is more "natural" for end users than calling So LGTM. |
There was a problem hiding this comment.
If we stuck everything in main derivation then every change will require rofi to be recompiled. It would make more sense to split it.
in pkgs/top-level/all-packages.nix we need :
{
wrapRofi = callPackage ./../applications/misc/rofi/wrapper.nix {};
rofi-unwrapped = callPackage ./../applications/misc/rofi {};
rofi = wrapRofi rofi-unwrapped {};
}I would look at neovim as an example how to write a wrapper derivation (pkgs/applications/editors/neovim/wrapper.nix)
|
@garbas ACK. I hope that I have sufficient time to tomorrow to implement this :) |
Ma27
force-pushed
the
configurable-rofi-theme
branch
2 times, most recently
from
e924015
to
f48d13d
Compare
|
@garbas do you like the change now? %) |
| wrapProgram "$out/bin/rofi" --add-flags "-theme ${theme}" | ||
| ''; | ||
|
|
||
| inherit (rofi_unwrapped) meta; |
There was a problem hiding this comment.
maybe do
meta = rofi-unwrapped.meta // {
# prefer wrapper over the package
priority = (rofi-unwrapped.meta.priority or 0) -1;
}There was a problem hiding this comment.
ahh I didn't know about priority, thanks!
pkgs/top-level/all-packages.nix
Outdated
| @@ -17335,7 +17335,8 @@ with pkgs; | |||
|
|
|||
| rkt = callPackage ../applications/virtualization/rkt { }; | |||
|
|
|||
| rofi = callPackage ../applications/misc/rofi { }; | |||
| rofi_unwrapped = callPackage ../applications/misc/rofi { }; | |||
There was a problem hiding this comment.
should be named rofi-unwrapped to be consistent with how others are naming
| @@ -0,0 +1,14 @@ | |||
| { symlinkJoin, rofi_unwrapped, makeWrapper, theme ? null, lib }: | |||
|
|
|||
| symlinkJoin { | |||
There was a problem hiding this comment.
i would use simple mkDerivation since we only want to link $out/bin/rofi in our wrapper and also make it overridable.
{ stdenv, lib, rofi-unwrapped, makeWrapper, theme ? null }:
let
wrapper = stdenv.mkDerivation {
name = "rofi-${version}";
buildInputs = [ makeWrapper ];
preferLocalBuild = true;
passthru = { unwrapped = rofi-unwrapped; };
buildCommand = ''
ln -s ${rofi-unwrapped}/bin/rofi
wrapProgram $out/bin/rofi --add-flags "-theme ${theme}"
'';
meta = rofi-unwrapped.meta // {
# prefer wrapper over the package
priority = (rofi-unwrapped.meta.priority or 0) -1;
};
};
in lib.makeOverridable wrapper
In fact I don't, I'm always glad to learn new things (see my comment) and it keeps the codebase clean :D |
Ma27
force-pushed
the
configurable-rofi-theme
branch
from
f48d13d
to
b84035d
Compare
|
@garbas done, I have just one (possibly stupid) question: what exactly do you want to achieve with { lib }:
let
wrapper = { stdenv, ... }: stdenv.mkDerivation {};
in lib.makeOverridable wrapperThe solution you proposed wouldn't work either as |
|
ping @garbas |
|
@garbas is there anything to add? I'd love to see this merged %) |
|
@Ma27 OK, I can compare the requests to the current state: the idea of making |
Ma27
force-pushed
the
configurable-rofi-theme
branch
2 times, most recently
from
5d35eb8
to
7dceaf1
Compare
|
@7c6f434c ahh you're right, thank you! %) |
|
Failure on x86_64-linux (full log) Attempted: rofi Partial log (click to expand)
|
|
@Ma27 I don't think the URL should use the name with |
This is helpful when defining a `*.rasi` theme for rofi using `pkgs.writeText` rather than messing up the impure `~/.config` directory.
Ma27
force-pushed
the
configurable-rofi-theme
branch
from
7dceaf1
to
26a4e02
Compare
|
Failure on aarch64-linux (full log) Attempted: rofi Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: rofi Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: rofi Partial log (click to expand)
|
Motivation for this change
This is helpful when defining a
*.rasitheme for rofi usingpkgs.writeTextrather than messing up the impure~/.configdirectory.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)