Check for Nerd Font and switch #53

heathhenley · 2025-02-12T04:56:47Z

This is on top of #51, part of #14 - it uses fc-list to check if Nerd Font is there, if it is it uses the normal icons, if not, it just uses simple characters (probably needs some work - but it's a start).

Here's what it is looks like with no font installed:

There is still something weird going on with WSL - all my WSL terminals report that I have nerd font installed, but if I use the Windows "Terminal" app, make a new Ubuntu WSL terminal, and try it, fc-list says it's installed (it's the same file system) but I unfortunately can't see the fonts. However, in WSL in the vscode terminal (I usually run connected to WSL) it works, so still might be little more to figure out. Worried it might be based on host fonts (?), but in that case both should be working for me so tbd...

Let me know what you think so far of the approach

thezbm · 2025-02-13T18:49:21Z

There is still something weird going on with WSL - all my WSL terminals report that I have nerd font installed, but if I use the Windows "Terminal" app, make a new Ubuntu WSL terminal, and try it, fc-list says it's installed (it's the same file system) but I unfortunately can't see the fonts. However, in WSL in the vscode terminal (I usually run connected to WSL) it works, so still might be little more to figure out. Worried it might be based on host fonts (?), but in that case both should be working for me so tbd...

I haven't tried on my Windows but is it because you didn't set the font of your Windows Terminal to a nerd font?

chshersh · 2025-02-13T20:30:55Z

There is still something weird going on with WSL - all my WSL terminals report that I have nerd font installed, but if I use the Windows "Terminal" app, make a new Ubuntu WSL terminal, and try it, fc-list says it's installed (it's the same file system) but I unfortunately can't see the fonts. However, in WSL in the vscode terminal (I usually run connected to WSL) it works, so still might be little more to figure out. Worried it might be based on host fonts (?), but in that case both should be working for me so tbd...

I haven't tried on my Windows but is it because you didn't set the font of your Windows Terminal to a nerd font?

Another idea to try: the problem could be in this code:

let nerd_font_installed () =
  let cmd = "fc-list | grep 'Hack Nerd Font Mono' > /dev/null 2>&1" in
  if Sys.command cmd = 0 then true else false

I think by default OCaml doesn't pass the shell environment to executable commands and it could matter here.

You may try using an already existing function from the shell local library that passes the environment correctly:

github-tui/lib/shell/shell.ml

Lines 14 to 21 in e85d1a8

    
           let proc_stdout cmd = 
        
             let env = Unix.environment () in 
        
             let ((proc_stdout, _proc_stdin, _proc_stderr) as process) = 
        
               Unix.open_process_full cmd env 
        
             in 
        
             let stdout_result = collect_chan proc_stdout in 
        
             let _ = Unix.close_process_full process in 
        
             String.trim stdout_result

heathhenley · 2025-02-13T23:05:40Z

There is still something weird going on with WSL - all my WSL terminals report that I have nerd font installed, but if I use the Windows "Terminal" app, make a new Ubuntu WSL terminal, and try it, fc-list says it's installed (it's the same file system) but I unfortunately can't see the fonts. However, in WSL in the vscode terminal (I usually run connected to WSL) it works, so still might be little more to figure out. Worried it might be based on host fonts (?), but in that case both should be working for me so tbd...

I haven't tried on my Windows but is it because you didn't set the font of your Windows Terminal to a nerd font?

Another idea to try: the problem could be in this code:
let nerd_font_installed () =
  let cmd = "fc-list | grep 'Hack Nerd Font Mono' > /dev/null 2>&1" in
  if Sys.command cmd = 0 then true else false
I think by default OCaml doesn't pass the shell environment to executable commands and it could matter here.

You may try using an already existing function from the shell local library that passes the environment correctly:

github-tui/lib/shell/shell.ml

Lines 14 to 21 in e85d1a8

let proc_stdout cmd =

let env = Unix.environment () in

let ((proc_stdout, _proc_stdin, _proc_stderr) as process) =

Unix.open_process_full cmd env

in

let stdout_result = collect_chan proc_stdout in

let _ = Unix.close_process_full process in

String.trim stdout_result

@thezbm good call, that did it! I had to go into Windows Terminal settings and select Nerd Font Mono for the WLS terminal "profile" - so I guess there are sometime two layers - I don't think we can do anything about that though.

@chshersh That makes sense, I thought it was a little simpler to check the grep return code than check the std out string - but also might be nice to keep using the same utils everywhere.

Going to rebase soon and push up

thezbm · 2025-02-14T08:14:55Z

Yeah I don't think we can check if the user actually uses a Nerd Font in the terminal, so maybe a flag like --no-nerd-font would be helpful for those without Nerd Fonts enabled.

As a side note, macOS doesn't seem to have fc-list. An alternative way would be to check well-known directories for fonts.

heathhenley · 2025-02-16T20:42:17Z

If we do add a cl arg for 'no nerd font' - what is the right way to get that state into where we need it? It doesn't seem right to just pass it down through the view to everything that needs it, but making something in Icons mutable and setting at start up also feels bad.

chshersh

Great stuff! But this indeed turns out to be a challenging task

chshersh · 2025-02-17T07:50:47Z

lib/pretty/icon.ml

+(** Use fc-list to check if Hack Nerd Font is installed - this might be a little
+    brittle, but it's simple - update if it becomes an issue. **)
+let nerd_font_installed () =
+  fc_list_cmd |> Shell.proc_stdout |> String.trim |> String.length > 0


Thanks for the docs! Indeed, we can always update this later 👍🏻

chshersh · 2025-02-17T07:58:17Z

lib/pretty/icon.ml

+  issue_char : string;
+}
+
+let fc_list_cmd = {| fc-list | grep 'Hack Nerd Font Mono' |}


The fact that it doesn't work on macOS is mildly sad.

I don't have a goal to support Windows but supporting both Linux and macOS should be good.

Could you create an issue to add macOS support and mention this issue in the comment here?

As @thezbm mentioned, we'll probably have to check special directories on macOS but then we also need to understand whether we're running from macOS or Linux.

Obviously, if you want, you can do this directly in this PR to fully finish this 😅

But it would be nice to at least avoid breaking macOS users. I guess, they'll get exception on the app start immediately.

A proper way to do this is to check if fc-list executable exists. Unfortunately, I haven't found a standard function to do this. One way would be to query env variable $PATH, split it and then check for the executable in every directory.

But maybe file_exists from Sys will work too:

https://ocaml.org/manual/5.1/api/Sys.html

Yeah I agree, I'll update to include it here so that we don't add regression for macOS.

What do you think if we use uname sys command to match on:

Linux --> try fc-list if it's there - in this PR or out of scope we could check Linux common dirs too if it's not

masOS --> check for the font in common dirs

Then maybe we're robust enough not to need another cl arg? Would just need to confirm that it's actually different (google said it is) for macOS, eg I hope it doesn't just say "Unix" like Sys.os_type but I don't have a mac

uname prints "Darwin" on my Mac 👍

chshersh · 2025-02-17T07:58:34Z

lib/pretty/icon.mli

+type t = {
+  closed_char : string;
+  open_char : string;
+  merged_char : string;
+  arrow_left_char : string;
+  pwd_char : string;
+  dir_char : string;
+  empty_dir_char : string;
+  file_char : string;
+  bin_char : string;
+  warning : string;
+  issue_char : string;
+}


Idea to use a record is really good!

chshersh · 2025-02-17T07:59:54Z

lib/pretty/pretty.mli

+(** A record representing the icon set **)
+val icons : Icon.t


Below, you use icons from the Pretty.Icon module directly:

module Icon = Pretty.Icon ... | Open -> Layout.(fmt Style.issue_open Icon.icons.issue_char)

So I guess we don't need to reexport it from Pretty?

🤦‍♂️

heathhenley · 2025-02-21T23:44:23Z

Here's some progress on this - @thezbm would you be interested in helping to debug / test if you have time? I don't have a mac so I have no idea if it's close to working or not 😬

lib/pretty/icon.ml

thezbm · 2025-02-23T07:56:27Z

It just occurred to me though some popular terminal emulators like kitty and Ghostty have built-in Nerd Fonts so these icons will work automatically, even if no Nerd Font is installed. AFAIK the best way to identify the terminal emulator in use would be to check the parent process recursively, which I think just goes too far.

Co-authored-by: Beiming Zhang <[email protected]>

heathhenley · 2025-02-23T16:45:23Z

It just occurred to me though some popular terminal emulators like kitty and Ghostty have built-in Nerd Fonts so these icons will work automatically, even if no Nerd Font is installed. AFAIK the best way to identify the terminal emulator in use would be to check the parent process recursively, which I think just goes too far.

That makes sense, that's probably the same issue as if you're running WSL in a Windows Terminal for example.

For those cases where Nerd is available but it doesn't seem like, this would break them (cause them to just use icons).

Maybe it's not worth it to do all this checking then?

We could remove all the os checking, add the no-nerd-font flag were talking about before - and just switch on that instead.

Only thing in that case is I'm not sure sure of the nice way to get it in there - really a mutable variable that gets set on start up from the cl args would work but doesn't feel like the right way in ocaml. It could be a env var instead, it's doing the same thing though. I guess I'm not sure if there's a better way to do it without mutability without passing down through everything that needs or making it be there on start up (env var).

chshersh · 2025-02-24T10:57:35Z

Apologies I wasn't that active on the discussion.

I don't want for this effort to get wasted, so I'm really sorry about what I'm about to say!

But what if we "just add a CLI flag" to disable icons? This way, inside GitHub TUI we don't have to rely on unprecise heuristics to figure out what terminal, what Nerd Fonts are install, what is the system of the user, and so on. This sounds like a lot of accidental complexity.

In the future, we may even want to support env variables and some global user config, so that users don't have to pass CLI flags manually.

We can continue discussion about the automatic detection under a separate issue until we figure out the best way.

My concern is that this is a lot of accidental complexity, 60% of the users won't even need that because they can just install the font (especially because it's mentioned in prerequisites), 20% will benefit but the remaining 20% will find this behaviour annoyingly frustrating because it won't work for them.

I'm happy to change my mind if you see that there's indeed huge value in automatic detection of fonts 🙂

heathhenley · 2025-02-24T13:34:47Z

No worries that makes sense to me - bummer that there doesn't seem to be a good way to figure it out programmatically.

heathhenley · 2025-02-24T13:36:23Z

Going to close and make a separate pr for the cl arg change when I get a chance (even though it uses some of this one)

heathhenley requested a review from chshersh February 12, 2025 04:56

heathhenley force-pushed the dev_hh_check_for_nerd_font branch from a15b4fe to 6dfef45 Compare February 13, 2025 23:20

heathhenley mentioned this pull request Feb 15, 2025

Refactor init #54

Merged

chshersh reviewed Feb 17, 2025

View reviewed changes

heathhenley added 4 commits February 21, 2025 18:29

Use fc-list to check for Nerd Font

8a912c9

Doc format fix

d42efd5

Use existing shell util

42a5d1a

WIP: checking for fonts

8124e04

heathhenley force-pushed the dev_hh_check_for_nerd_font branch from cec0899 to 8124e04 Compare February 21, 2025 23:29

heathhenley added 2 commits February 21, 2025 18:37

Export just icons in pretty

d45419f

Filename has no space (fc-list prints font name too)

1c98f79

thezbm reviewed Feb 23, 2025

View reviewed changes

lib/pretty/icon.ml Outdated Show resolved Hide resolved

thezbm reviewed Feb 23, 2025

View reviewed changes

lib/pretty/icon.ml Show resolved Hide resolved

Fix: home dir on mac

5176f03

Co-authored-by: Beiming Zhang <[email protected]>

heathhenley closed this Feb 24, 2025

		( A record representing the icon set )
		val icons : Icon.t

Uh oh!

Check for Nerd Font and switch #53

Check for Nerd Font and switch #53

Uh oh!

Conversation

heathhenley commented Feb 12, 2025

Uh oh!

thezbm commented Feb 13, 2025

Uh oh!

chshersh commented Feb 13, 2025

Uh oh!

heathhenley commented Feb 13, 2025

Uh oh!

thezbm commented Feb 14, 2025

Uh oh!

heathhenley commented Feb 16, 2025

Uh oh!

chshersh left a comment

Choose a reason for hiding this comment

Uh oh!

chshersh Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

chshersh Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

heathhenley Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

thezbm Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

chshersh Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

chshersh Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

heathhenley Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

heathhenley commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thezbm commented Feb 23, 2025

Uh oh!

heathhenley commented Feb 23, 2025

Uh oh!

chshersh commented Feb 24, 2025

Uh oh!

heathhenley commented Feb 24, 2025

Uh oh!

heathhenley commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

heathhenley commented Feb 21, 2025 •

edited

Loading

heathhenley commented Feb 24, 2025 •

edited

Loading