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

fonts: support fallback fonts #201

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jalil-salame
Copy link
Contributor

Alternative to #198 . It is a big breaking change to the config. If you set the fonts you now need to add []. For example:

serif = {
  package = pkgs.dejavu_fonts;
  name = "DejaVu Serif";
};

Becomes:

#       v--- IMPORTANT!
serif = [{
  package = pkgs.dejavu_fonts;
  name = "DejaVu Serif";
}];
#^-- IMPORTANT!

@jalil-salame
Copy link
Contributor Author

I'm getting build issues when using this branch 🥲

I'll look over the diff to try and debug it.

This is the build log: https://github.com/jalil-salame/NixOsConfig/actions/runs/7276308829

modules/i3/hm.nix Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jalil-salame jalil-salame left a comment

Choose a reason for hiding this comment

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

I think I found the issue

stylix/darwin/fonts.nix Outdated Show resolved Hide resolved
stylix/fonts.nix Show resolved Hide resolved
stylix/hm/fonts.nix Outdated Show resolved Hide resolved
stylix/nixos/fonts.nix Outdated Show resolved Hide resolved
@jalil-salame
Copy link
Contributor Author

jalil-salame commented Dec 20, 2023

I think I found the issue

I didn't :c

It still fails: https://github.com/jalil-salame/NixOsConfig/actions/runs/7278474766

If you manage to catch something I missed then please tell me.

@jalil-salame jalil-salame marked this pull request as draft December 20, 2023 17:14
Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

Not sure why this is failing - from the error message my best guess would be that we are using package somewhere where we should be using { name, package }.

stylix/helper.nix Outdated Show resolved Hide resolved
stylix/fonts.nix Show resolved Hide resolved
@jalil-salame
Copy link
Contributor Author

Not sure why this is failing - from the error message my best guess would be that we are using package somewhere where we should be using { name, package }.

I will grep for this case again as soon as I have time.

@jalil-salame
Copy link
Contributor Author

Not sure why this is failing - from the error message my best guess would be that we are using package somewhere where we should be using { name, package }.

I will grep for this case again as soon as I have time.

I'm confused; I couldn't find this case anywhere but changing the functions to catAttrs fixed the build issues...

@jalil-salame jalil-salame marked this pull request as ready for review December 23, 2023 20:14
@jalil-salame jalil-salame requested a review from danth December 30, 2023 22:38
@danth
Copy link
Owner

danth commented Jan 3, 2024

Strange, I wonder if the issue is actually fixed or if catAttrs just filtered it out.

Collect each attribute named attr from a list of attribute sets. Sets that don't contain the named attribute are ignored.

stylix/fonts.nix Outdated Show resolved Hide resolved
@trueNAHO
Copy link
Collaborator

@jalil-salame jalil-salame requested a review from danth January 14, 2024 13:54
@jalil-salame
Copy link
Contributor Author

Job failure is unrelated to my changes, it probably needs to be rerun

@danth
Copy link
Owner

danth commented Jan 14, 2024

Should be re-running now.

@jalil-salame
Copy link
Contributor Author

It passed 🎉 this PR is now ready to be merged c:

@jalil-salame
Copy link
Contributor Author

Rebased on top of master and addressed conflicts so it can be merged. Is anything blocking this? I don't want to keep going back and fixing small conflicts on this PR.

@jalil-salame jalil-salame force-pushed the fallback-fonts-v2 branch 2 times, most recently from 025e1da to 2ec5b63 Compare January 27, 2024 14:37
@jalil-salame jalil-salame force-pushed the fallback-fonts-v2 branch 2 times, most recently from a492a0a to 8059347 Compare February 8, 2024 07:03
@trueNAHO trueNAHO changed the title feat(fonts): Add support for fallback fonts (v2) fonts: support fallback fonts (v2) Feb 9, 2024
@trueNAHO
Copy link
Collaborator

trueNAHO commented Feb 9, 2024

The following patch applies this PR on my Home Manager configuration:

From 5f88a66d30bf014109ce8a41350447621d100515 Mon Sep 17 00:00:00 2001
From: NAHO <[email protected]>
Date: Fri, 9 Feb 2024 23:04:41 +0100
Subject: [PATCH] feat(modules/stylix): support fallback fonts

---
 flake.lock                 | 11 ++++-----
 flake.nix                  |  2 +-
 modules/stylix/default.nix | 46 ++++++++++++++++++++++----------------
 3 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/flake.lock b/flake.lock
index 8b35089..f58efc3 100644
--- a/flake.lock
+++ b/flake.lock
@@ -555,15 +555,16 @@
         ]
       },
       "locked": {
-        "lastModified": 1707414210,
-        "narHash": "sha256-MJ4deL9tTzowkGpW9Iq+k3cSKo2gnvyIkIuFctNz/dQ=",
-        "owner": "danth",
+        "lastModified": 1707499046,
+        "narHash": "sha256-KTAvqoFbvUmzfPgN3vxlr8FABlbJ9gbwRTYtSQ3HfWw=",
+        "owner": "jalil-salame",
         "repo": "stylix",
-        "rev": "f3b302dd9bb66fcdd1ed3f185068a5f1000eb863",
+        "rev": "20e410269dc0432e6884a1a31403951abf6c155f",
         "type": "github"
       },
       "original": {
-        "owner": "danth",
+        "owner": "jalil-salame",
+        "ref": "fallback-fonts-v2",
         "repo": "stylix",
         "type": "github"
       }
diff --git a/flake.nix b/flake.nix
index 97da286..a1c6fa2 100644
--- a/flake.nix
+++ b/flake.nix
@@ -73,7 +73,7 @@
         nixpkgs.follows = "nixpkgs";
       };

-      url = "github:danth/stylix";
+      url = "github:jalil-salame/stylix/fallback-fonts-v2";
     };

     # Add the 'systems' input for consistent versioning across inputs.
diff --git a/modules/stylix/default.nix b/modules/stylix/default.nix
index a758aa5..1365865 100644
--- a/modules/stylix/default.nix
+++ b/modules/stylix/default.nix
@@ -17,25 +17,33 @@
       };

       fonts = {
-        emoji = {
-          package = pkgs.noto-fonts-emoji;
-          name = "Noto Color Emoji";
-        };
-
-        monospace = {
-          package = pkgs.nerdfonts.override {fonts = ["FiraCode"];};
-          name = "FiraCodeNerdFont";
-        };
-
-        sansSerif = {
-          package = pkgs.ibm-plex;
-          name = "IBMPlexSans";
-        };
-
-        serif = {
-          package = pkgs.crimson;
-          name = "Crimson";
-        };
+        emoji = [
+          {
+            package = pkgs.noto-fonts-emoji;
+            name = "Noto Color Emoji";
+          }
+        ];
+
+        monospace = [
+          {
+            package = pkgs.nerdfonts.override {fonts = ["FiraCode"];};
+            name = "FiraCodeNerdFont";
+          }
+        ];
+
+        sansSerif = [
+          {
+            package = pkgs.ibm-plex;
+            name = "IBMPlexSans";
+          }
+        ];
+
+        serif = [
+          {
+            package = pkgs.crimson;
+            name = "Crimson";
+          }
+        ];

         sizes = {
           terminal = 7;
--
2.43.0

Unfortunately, I encounter the following build error:

$ home-manager switch --flake <FLAKE>
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'home-manager-generation'
         whose name attribute is located at /nix/store/5gc4ncnkzcgma8fl1qabm8v9kj7w68gl-source/pkgs/stdenv/generic/make-derivation.nix:352:7

       … while evaluating attribute 'buildCommand' of derivation 'home-manager-generation'

         at /nix/store/5gc4ncnkzcgma8fl1qabm8v9kj7w68gl-source/pkgs/build-support/trivial-builders/default.nix:98:16:

           97|         enableParallelBuilding = true;
           98|         inherit buildCommand name;
             |                ^
           99|         passAsFile = [ "buildCommand" ]

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: infinite recursion encountered

       at «none»:0: (source not available)

For reference, the following follow-up patch still causes the infinite recursion encountered error:

From a82f82470a83a7ea08c514d3f02ec8f0db10d15d Mon Sep 17 00:00:00 2001
From: NAHO <[email protected]>
Date: Fri, 9 Feb 2024 23:18:34 +0100
Subject: [PATCH] feat(modules/stylix): set 'monospace' font to default

---
 modules/stylix/default.nix | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/modules/stylix/default.nix b/modules/stylix/default.nix
index 1365865..7325a01 100644
--- a/modules/stylix/default.nix
+++ b/modules/stylix/default.nix
@@ -24,13 +24,6 @@
           }
         ];

-        monospace = [
-          {
-            package = pkgs.nerdfonts.override {fonts = ["FiraCode"];};
-            name = "FiraCodeNerdFont";
-          }
-        ];
-
         sansSerif = [
           {
             package = pkgs.ibm-plex;
--
2.43.0

@trueNAHO trueNAHO marked this pull request as draft March 18, 2024 09:23
@trueNAHO
Copy link
Collaborator

Am I improperly applying your patch, or is there a bug?

There probably is a bug. Thanks for noticing.

So far what I can tell is that dunst(?) is accessing the fonts before they are fully evaluated, but I need to look more into it before I come to a conclusion and can push a fix.

Mark the PR as ready once it's intended for review.

@jalil-salame
Copy link
Contributor Author

Am I improperly applying your patch, or is there a bug?

There probably is a bug. Thanks for noticing.

So far what I can tell is that dunst(?) is accessing the fonts before they are fully evaluated, but I need to look more into it before I come to a conclusion and can push a fix.

Mark the PR as ready once it's intended for review.

Sorry about that, I haven't had time to fix this. It broke locally so I had to rebase it, but I don't have time to look into the issue...

@jalil-salame jalil-salame force-pushed the fallback-fonts-v2 branch 2 times, most recently from 2f2be91 to 00b32e1 Compare April 22, 2024 14:35
@jalil-salame jalil-salame force-pushed the fallback-fonts-v2 branch 2 times, most recently from da4c299 to 26fd862 Compare May 5, 2024 11:35
@jalil-salame
Copy link
Contributor Author

I can't figure out what is causing the infinite recursion in @trueNAHO 's case. I'll try removing the coercing code and checking again.

jalil-salame added a commit to jalil-salame/stylix that referenced this pull request May 5, 2024
This should hopefilly fix the infinite recursion errors in
danth#201 (comment)
@jalil-salame
Copy link
Contributor Author

Removing the coercion to a list did not work T-T.

@jalil-salame
Copy link
Contributor Author

I don't know what is going on with the infinite recursion, it was not the coercing code, so it probably has to do with builtins.head being eager while we need some construct like lib.mkIf instead. I don't know how to build such a construct though, so I give up. Feel free to take over or close this.

@jalil-salame jalil-salame marked this pull request as ready for review May 5, 2024 12:17
@trueNAHO
Copy link
Collaborator

trueNAHO commented May 5, 2024

I can't figure out what is causing the infinite recursion in @trueNAHO 's case. I'll try removing the coercing code and checking again.

Do you not encounter a infinite recursion encountered error?

@jalil-salame
Copy link
Contributor Author

I can't figure out what is causing the infinite recursion in @trueNAHO 's case. I'll try removing the coercing code and checking again.

Do you not encounter a infinite recursion encountered error?

I do encounter it, I just could not find the cause. I tried turning off different parts of the config, but nothing worked.

Comment on lines -29 to +48
programs.bemenu.settings = with config.stylix.targets.bemenu; {
tb = "${base01}${bemenuOpacity}"; # Title bg
nb = "${base01}${bemenuOpacity}"; # Normal bg
fb = "${base01}${bemenuOpacity}"; # Filter bg
hb = "${base03}${bemenuOpacity}"; # Highlighted bg
sb = "${base03}${bemenuOpacity}"; # Selected bg
scb = "${base01}"; # Scrollbar bg

hf = "${base0A}"; # Highlighted fg
sf = "${base0B}"; # Selected fg
tf = "${base05}"; # Title fg
ff = "${base05}"; # Filter fg
nf = "${base05}"; # Normal fg
scf = "${base03}"; # Scrollbar fg

ab = "${if alternate then base00 else base01}"; # Alternate bg
af = "${if alternate then base04 else base05}"; # Alternate fg

# Font name
fn = "${sansSerif.name} ${lib.optionalString (fontSize != null) (builtins.toString fontSize)}";
};
home.sessionVariables.BEMENU_OPTS = with config.stylix.targets.bemenu; builtins.concatStringsSep " " [
# Inspired from https://git.sr.ht/~h4n1/base16-bemenu_opts
"--tb '${base01}${bemenuOpacity}'"
"--nb '${base01}${bemenuOpacity}'"
"--fb '${base01}${bemenuOpacity}'"
"--hb '${base03}${bemenuOpacity}'"
"--sb '${base03}${bemenuOpacity}'"
"--hf '${base0A}'"
"--sf '${base0B}'"
"--tf '${base05}'"
"--ff '${base05}'"
"--nf '${base05}'"
"--scb '${base01}'"
"--scf '${base03}'"
"--ab '${if alternate then base00 else base01}'"
"--af '${if alternate then base04 else base05}'"
"--fn '${sansSerif.name} ${lib.optionalString (fontSize != null) (builtins.toString fontSize)}'"
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain why the previous programs.bemenu.settings implementation was replaced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably a mistake I made while fixing merge conflicts

Comment on lines 78 to 129
stylix.fonts = {
serif = {
serif = [{
package = pkgs.dejavu_fonts;
name = "DejaVu Serif";
};
}];

sansSerif = {
sansSerif = [{
package = pkgs.dejavu_fonts;
name = "DejaVu Sans";
};
}];

monospace = {
monospace = [{
package = pkgs.dejavu_fonts;
name = "DejaVu Sans Mono";
};
}];

emoji = {
emoji = [{
package = pkgs.noto-fonts-emoji;
name = "Noto Color Emoji";
};
}];
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

The following format would better highlight the list:

  sytlix.fonts = {
    serif = [
      {
        package = pkgs.dejavu_fonts;
        name = "DejaVu Serif";
      }
    ];

    sansSerif = [
      {
        package = pkgs.dejavu_fonts;
        name = "DejaVu Sans";
      }
    ];

    monospace = [
      {
        package = pkgs.dejavu_fonts;
        name = "DejaVu Sans Mono";
      }
    ];

    emoji = [
      {
        package = pkgs.noto-fonts-emoji;
        name = "Noto Color Emoji";
      }
    ];
  };

nestedTypes.elemType = elemType;
});

fontList = coercedToNonEmptyListOf fontType;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still receiving an infinite recursion encountered error with this patch:

Suggested change
fontList = coercedToNonEmptyListOf fontType;
fontList = types.listOf fontType;

Any ideas what could the cause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is builtins.head being eager to evaluate

@trueNAHO
Copy link
Collaborator

trueNAHO commented May 5, 2024

Do you not encounter a infinite recursion encountered error?

I do encounter it, I just could not find the cause. I tried turning off different parts of the config, but nothing worked.

Simplifying the PR with #201 (comment) does not resolve the issue. This seems to be a rather nasty infinite recursion encountered error...

However, the PR itself seems fine. I assume the problem stems from something seemingly unrelated within Stylix.

@trueNAHO
Copy link
Collaborator

trueNAHO commented May 5, 2024

Would it not be better to supersede this PR with the one mentioned in #216 (comment)? Although that PR would need to be superseded as well to resolve the pending suggestions (and attract new attention).

@jalil-salame
Copy link
Contributor Author

jalil-salame commented May 5, 2024

Would it not be better to supersede this PR with the one mentioned in #216 (comment)? Although that PR would need to be superseded as well to resolve the pending suggestions (and attract new attention).

Ideally yes, but it has been open for ~2 years now

@trueNAHO
Copy link
Collaborator

Would it not be better to supersede this PR with the one mentioned in #216 (comment)? Although that PR would need to be superseded as well to resolve the pending suggestions (and attract new attention).

Ideally yes, but it has been open for ~2 years now

nix-community/home-manager#2732 has been merged.

@jalil-salame
Copy link
Contributor Author

jalil-salame commented May 12, 2024

Would it not be better to supersede this PR with the one mentioned in #216 (comment)? Although that PR would need to be superseded as well to resolve the pending suggestions (and attract new attention).

Ideally yes, but it has been open for ~2 years now

🎉 great work! Thanks! Feel free to close this PR then!

@trueNAHO
Copy link
Collaborator

🎉 great work! Thanks! Feel free to close this PR then!

Does nix-community/home-manager#2732 cover what you wanted to do with this PR?

@jalil-salame
Copy link
Contributor Author

Not really, what I'd want is to pull the defaultFonts.* and get my apps to follow it (specifically Wezterm). But this can be done using a different PR

@trueNAHO
Copy link
Collaborator

Not really, what I'd want is to pull the defaultFonts.* and get my apps to follow it (specifically Wezterm). But this can be done using a different PR

Depending on the discussion in #366, we might still need this PR.

Some programs use them (notably wezterm which is why I made this PR) and
they are a nice way to add nerdfont symbols without patching fonts.
This modifies the modules to add support for fallback fonts. Some of
them just use the default fonts (the first font in the list), some also
respect the fallback.
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.

3 participants