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

Remove display-graphic-p from icon checks #7

Closed
mohkale opened this issue Oct 21, 2021 · 28 comments
Closed

Remove display-graphic-p from icon checks #7

mohkale opened this issue Oct 21, 2021 · 28 comments

Comments

@mohkale
Copy link

mohkale commented Oct 21, 2021

I've got icons setup on my terminal but this plugin doesn't render them because it actively disables them on non GUI frames (here and here). It should be sufficient to have end-users disable all-the-icons-ibuffer-icon on non graphical frames if they don't want them so there's no need for the plugin to do so as well.

@seagle0128
Copy link
Owner

It's as designed since all-the-icons doesn't support terminal well.

@mohkale
Copy link
Author

mohkale commented Oct 21, 2021

@seagle0128

It's as designed since all-the-icons doesn't support terminal well.

I agree it's a little more of a hassle but it's certainly not unsupported or unusable.

Screenshot_20211021_160926

Either way the way this plugin has implemented it there's no way to use all-the-icons-ibuffer-mode on terminal frames. That's probably a good move for most users, but I think taking the abilitiy away to use them altogether for other users is a bad choice. If users don't want icons on terminal frames they should (setq all-the-icons-ibuffer-icon (display-graphic-p)) (or perhaps we should make the defcustom default to that).

Edit: Technically I'm not using that mode so this isn't an issue for me, instead I'm just setting ibuffer-formats manually and using the icon column defined by this package. I didn't realise I had setup a custom variable for showing icons when I made this issue so if you don't want to change the behaviour that's fine, I just thought there's little value in having the check.

@mohkale mohkale closed this as completed Oct 21, 2021
@seagle0128
Copy link
Owner

Thanks for the update! I am curious how do you use all-the-icons in terminal mode? What OS and version?

@mohkale
Copy link
Author

mohkale commented Oct 23, 2021

@seagle0128

how do you use all-the-icons in terminal mode? What OS and version?

I'm on linux, using st (here's my fork) a lightweight terminal built for the X window system. If you look in my fonts config for st you'll find I've added all the all-the-icons fonts as sparefonts meaning when my terminal tries to render a glyph not in my main font, it tries each font in sparefonts until it finds one that has the glyph. Thats' the terminal side, and it works reasonably well, however because all-the-icons uses multiple fonts there's a high chance a glyph will conflict between them. For example my octicon icon for repository is a clock due to how my font lookup precedence works. To get around that I'm using nerd-fonts (these have the highest priority in my terminal lookup) and then replacing any references to an all-the-icons font (eg: octicon) with the equivalent nerd-font (eg: nerd-oct). This is configured here. Lastly since most icons are double width (they take up the size of two characters when rendered) I've had to advise several functions to add another space when on terminal frames. For example this is how I do it with this package, or on some functions I just advise all the all-the-icons-%s family functions to be suffixed such as on all-the-icons-dired. This last point isn't ideal since some packages don't expose the functionality for advising how they insert a icon. For example I've managed to advise dashboard to properly pad icons in the headers (recent files, projects, etc.) but can't figure out a way to do so for the contents (the org icon for example should have an extra space before the ~/ but it doesn't.

Screenshot_20211023_153128

Here's my emacs config in case you want some more screenshots of icons on the terminal.

@seagle0128
Copy link
Owner

@mohkale Thanks for your sharing! I've understood your solution completely. It would be better if you make a separate package.

@mohkale
Copy link
Author

mohkale commented Oct 30, 2021

You mean fork ibuffer-all-the-icons? Or create a package that bridges the gap between nerd fonts and all-the-icons?

The latter I've avoided since there's a pending issue in the nerd font package that I'm waiting on the maintainer to resolve.

@seagle0128
Copy link
Owner

seagle0128 commented Oct 30, 2021

I mean the latter. And I found some icons are missing in nerd font, e.g. emacs. Is it still an issue?

And I recalled, should consider this case: the user is using both terminal and GUI, but the icons don't work in terminal. The current solution is able to work.

@mohkale
Copy link
Author

mohkale commented Oct 30, 2021

e.g. emacs. Is it still an issue?

Yep, their really taking their time on it. 😞.

the user is using both terminal and GUI, but the icons don't work in terminal. The current solution is able to work.

I mean I can do that?

Screenshot_20211030_162646

In the end my opinion on this is if you have a more complex workflow, you should configure it to handle that. If you can only support icons on graphical frames then don't use the terminal, or don't enable icons on either. You could even setup a hook that detects when a new frame is made and disables icons on all frames based on whether its on the terminal or the GUI. Hard-coding a dependence on a specific front-end is a bad idea. Consistency across GUIs and TTYs is one of emacs strong points.

EDIT: Also correct me if I'm wrong but display-graphic-p doesn't stop icons being inserted in tty frames? It's not as if ibuffer is refreshed when you visit an existing buffer from a terminal frame. For example I can make the ibuffer on a graphical frame, make a new tty frame and then switch-buffer to it and the icons would still be there.

@mohkale
Copy link
Author

mohkale commented Apr 22, 2022

Hi. A recent change to this project has invalidated the workaround I had in my config... so I'm asking again, please don't disable icons on terminal frames. The new implementation is baked directly into the minor mode so now I can't workaround it without advising away display-graphic-p within all-the-icons-ibuffer-mode or redefining the minor mode verbatim just without display-graphic-p.

@seagle0128
Copy link
Owner

Right! I made a break change recently, to support daemon mode. Please see #9 (comment).

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

@seagle0128

I can see why you made it a minor mode and I agree it makes sense. I don't see why you're still preventing the mode from being used in terminal frames when I've demonstrated it works fine. Could you at the very least move it to an option I can disable. Something like (defcustom ibuffer-all-the-icons-display-predicate #'display-graphic-p).

@seagle0128
Copy link
Owner

@mohkale I know it's possible to display the icons in the terminal. Are you using icons-in-terminal or something else?

all-the-icons doesn't support terminal officially, so all-the-icons-ibuffer doesn't, either. all-the-icons is used in multiple platform including Linux, macOS and Windows. If removing display-grahpic-p, users will see lots of wired characters, especially on Windows/Cygwin. I don't want to handle such issues.

(defcustom ibuffer-all-the-icons-display-predicate #'display-graphic-p)

But this may be a good idea!

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

I know it's possible to display the icons in the terminal. Are you using icons-in-terminal or something else?

I'm using all-the-icons. Perhaps scroll through the history of this issue ticket (it's been a while so I'd understand if you've forgotten). Since then I've also created a new package all-the-icons-nerd-fonts which contains the bulk of my configuration.

users will see lots of wired characters.

Which is an issue with their terminal configuration, not this package. Most terminal users would understand this (and barring that you can include a section in the README mentioning it). I'm fine with you disabling terminal support out of the box, that's likely a reasonable default, but atm there's no convenient way to enable it for terminal users which is my issue.

But this may be a good idea!

Great. PR incoming.

@seagle0128
Copy link
Owner

The pactch 0c72213 has been commit.

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

Great. PR incoming.

Ah, you're too fast for me. Thanks a bunch :-).

@seagle0128
Copy link
Owner

seagle0128 commented Apr 24, 2022

@mohkale Welcome. When all-the-icons-nerd-fonts will be released to MELPA? And I remember some icons are missing in nerd fonts, e.g. Emacs 😢

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

When all-the-icons-nerd-fonts will be released to MELPA

Ideally when nerd-fonts#2 is fixed. That's been open for over a year so I'm not sure whether the nerd-fonts package is even still actively maintained. I could likely add a workaround in my package but it'd require some testing. If nerd-fonts itself isn't updated soon that's likely what I'd do, it's in my backlog.

And I remember some icons are missing in nerd fonts, e.g. Emacs

Yes. Still an issue. It's not one I've faced yet since nothing in my nerd-fonts font conflicts with the emacs icon and I've got all of all-the-icons fonts in my lookup as well. To demonstrate you can take a look at my tmux status-line which has the emacs icon right there :-).

Screenshot_20220424_113728

@seagle0128
Copy link
Owner

Oh, thanks for the info. I will try nerd fonts, again.

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

Great. If you're gonna try out all-the-icons-nerd-fonts any feedback would be appreciated :-).

For reference here's the list of fonts I have and the order in which their configured. In way of terminals I'd recommend kitty. It has really nice font support letting you specify which individual glyphs you want from a font. It also has this really cool feature that shrinks a multi-character glyph to fit. So if you have a 2-character width icon it gets followed by a space it's shown as two characters, if you don't have the space it's resized and shown as a single character. In my terminal it just cuts off the icon at the halfway point 😢. I'll create a patch for st for this at some point as well :-).

@seagle0128
Copy link
Owner

Unfortunately I got this screenshot 😢

The folder icon is correct, but still no emacs icon for me

image

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

Could you share which terminal you're using and what configuration (if any) you have for it?

@seagle0128
Copy link
Owner

I am using iterm2 with Hack Nerd Fonts on macOS. No special configurations.

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

Hmmm... I can't really reproduce since I don't have a mac-os machine :/. I'd suggest making sure the right fonts are in the lookup. The question mark glyph shown there is often used when your terminal doesn't have any fonts with the glyph in its lookup.

Edit: Looks like iterm doesn't support font-fallbacks. I'd suggest trying a terminal that can like mine (st) or one you can configure to like kitty.

@seagle0128
Copy link
Owner

seagle0128 commented Apr 24, 2022

What fonts are you using for the fallbacks? I think there is no emacs icon in the latest nerd fonts.

See ryanoasis/nerd-fonts#342.

@mohkale
Copy link
Author

mohkale commented Apr 24, 2022

I linked it above. font is the main font, then it goes through each font in sparefonts until it finds one with an entry for the current glyph.

@seagle0128
Copy link
Owner

seagle0128 commented Apr 25, 2022

Oh, I see. You are using st which is built by yourself. The solution seems not working for other terminals, unless building all fonts into one, IMO.

Edit: I added this to all-the-icons-nerd-fonts-convert-icons, it looks good. The only problem is the nerd-dev icons are much smaller. This seems the best common solution for missing icons, at least to me.

    ((all-the-icons-fileicon   . "elisp")            .    (all-the-icons-nerd-dev    . "gnu"))
    ((all-the-icons-fileicon   . "emacs")            .    (all-the-icons-nerd-dev    . "gnu"))

image

@mohkale
Copy link
Author

mohkale commented Apr 25, 2022

it looks good. The only problem is the nerd-dev icons are much smaller.

You should be able to configure the scaling for individual fonts. In my case they were too big so I scaled them down a bit.

I added this to all-the-icons-nerd-fonts-convert-icons

Why would you replace the emacs and elisp icons with GNU ones? Personally I prefer them staying as they are.

@seagle0128
Copy link
Owner

Why would you replace the emacs and elisp icons with GNU ones? Personally I prefer them staying as they are.

Because there is no emacs or elisp icon in nerd-fonts. I have to use gnu icon instead, for testing purpose.

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

No branches or pull requests

2 participants