SuperSandro2000
commented
6 months ago @djacu @samueldr can you take a look?
Closed OPNA2608 closed 6 months ago
SuperSandro2000
commented
6 months ago @djacu @samueldr can you take a look?
djacu
commented
6 months ago @djacu @samueldr can you take a look?
I was not around when the deep magics of this makefile were written. Nothing looks off to me and I agree with the reasoning behind this PR, however I would defer to @samueldr or anyone else who has contributed to the makefile for final approval.
Also @OPNA2608, my entire being is stunned!
samueldr
commented
6 months ago One problem with this solution is that it also would involve adding xmlstarlet as part of the closure of when nixos-icons is used.
See https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/data/misc/nixos-artwork/icons.nix
I think the actual solution for this would be simpler: use the already in-use imagemagick to relayout the icon.
Or since we probably want it for the SVG anyway... change nothing in the build, but edit the source to live in a square viewbox/document?
OPNA2608
commented
6 months ago One problem with this solution is that it also would involve adding xmlstarlet as part of the closure of when nixos-icons is used.
xmlstarlet has these inputs:
nativeBuildInputs = [ autoreconfHook pkg-config ];
buildInputs = [ libxml2 libxslt ];The build closure of nixos-icons already seems to have libxml2 and libxslt, so the increase appears to be negligible?
nix-repl> nixos-icons
«derivation /nix/store/l7vikid1xapk34m6g6423fdvdpsvjwaq-nixos-icons-2021-02-24.drv»
nix-repl> nixos-icons.overrideAttrs (oa: { nativeBuildInputs = oa.nativeBuildInputs ++ [ xmlstarlet ]; })
«derivation /nix/store/jr1v1l5c7igs6fliygww5wa1gnrwzry1-nixos-icons-2021-02-24.drv»↪ nix --extra-experimental-features nix-command path-info -S /nix/store/l7vikid1xapk34m6g6423fdvdpsvjwaq-nixos-icons-2021-02-24.drv
warning: The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '/nix/store/l7vikid1xapk34m6g6423fdvdpsvjwaq-nixos-icons-2021-02-24.drv^*'
/nix/store/l7vikid1xapk34m6g6423fdvdpsvjwaq-nixos-icons-2021-02-24.drv 4074792
↪ nix --extra-experimental-features nix-command path-info -S /nix/store/jr1v1l5c7igs6fliygww5wa1gnrwzry1-nixos-icons-2021-02-24.drv
warning: The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '/nix/store/jr1v1l5c7igs6fliygww5wa1gnrwzry1-nixos-icons-2021-02-24.drv^*'
/nix/store/jr1v1l5c7igs6fliygww5wa1gnrwzry1-nixos-icons-2021-02-24.drv 4085232I think the actual solution for this would be simpler: use the already in-use imagemagick to relayout the icon.
From my experimenting, imagemagick does not do SVG -> SVG manipulations. So this would only work for the raster variants.
Or since we probably want it for the SVG anyway... change nothing in the build, but edit the source to live in a square viewbox/document?
I'm not graphically inclined enough to know if doing this properly without breaking some design stuff involves messing with any of these hidden layers to match the increased height.
And if we change the committed sources to fit our output needs, then I feel like there should be some "these must be square to not break output promises" kinda enforcement going forward. Because they haven't been square since… the very beginning?, so if we really want squares out the other end of the process, then something/someone needs to pay attention to that. This build-time code would at least enforce this desired squaredness.
samueldr
commented
6 months ago I've opened #120, I think it's more respectful of your time than trying to lead you to a result.
Though, sorry if your changes are not in use.
First of all, interesting that xmlstarlet probably wouldn't have been a closure issue. Good to know if it ever becomes needed.
(skipping over imagemagick svg-to-svg)
I'm not graphically inclined enough to know if doing this properly without breaking some design stuff involves messing with any of these hidden layers to match the increased height.
[...]
And if we change the committed sources to fit our output needs, then I feel like there should be some "these must be square to not break output promises" kinda enforcement going forward. Because they haven't been square since… the very beginning?, so if we really want squares out the other end of the process, then something/someone needs to pay attention to that. This build-time code would at least enforce this desired squaredness.
I totally understand the concerns. I am taking charge of that risk. I am the person in charge for this.
First of all... those two files did not serve as an official "design" source element. They were, in an utilitarian fashion, split off from the original design file(s), and used for icons. Usage outside of this repo in automated fashion without declaring a specific input should uh... not have been done, so I think changing this is fine.
Keeping them squared off I think makes sense given the use-case. If a non-square version is needed, self-service from those source artifacts is expected. Since anyways the default size might not even match what is needed.
Does that make sense? Anyway, if you can spare the cycles, #120 is what I had in mind.
OPNA2608
commented
6 months ago Though, sorry if your changes are not in use.
That's fine! As long as this gets addressed in some way, I'm happy. :grinning:
Closes #42
An example, with the background filled in to show its stunning squareness. :smile: