feat: icon support #2869
feat: icon support #2869
Conversation
|
I use https://github.com/microsoft/vscode-codicons in Neovim and they don't need any patching, they seem to just work out of the box. Just double click https://github.com/microsoft/vscode-codicons/blob/main/dist/codicon.ttf in Linux/Gnome and they install. You can see them in action here: https://github.com/hrsh7th/nvim-cmp/wiki/Menu-Appearance#how-to-add-visual-studio-code-dark-theme-colors-to-the-menu |
The "patched" here does not refer to any manual patching operation of some sort (nerd-fonts work the same, you install it, it works), but rather to the font having glyphs mapped to non-standard unicode values. This essentially means that one unicode value will not represent the same type of glyph for two different "patched" fonts. This is the reason why icon support for icons that are only available in patched fonts (such as filetype icons) should remain opt-in, otherwise people trying helix might not understand why they have unrecognized characters. This is also why I propose that we make defaults for some popular patched fonts for when the "extended" icon support is enabled by the user. |
kirawi
added
A-helix-term
|
Just a heads-up, this PR is not really inactive, just waiting on #2377 to get merged so that work can start on stable foundations. |
the-mikedavis
removed
the
S-inactive
kirawi
added
the
S-waiting-on-pr
|
@lazytanuki the PR you are waiting on has been merged! Just letting you know. |
|
I've mainly been waiting for #2377 to be merged since that will be where most of the icons will appear, but I think it is a good time for me to rebase this PR and start working on it again indeed. |
|
Hi ! Quick update: I have added an
|
|
Wow this looks great! I look forward to seeing this feature come to Helix! |
lazytanuki
force-pushed
the
icons
branch
2 times, most recently
from
6dcd963 to
470d4ff
Compare
|
The diagnostics icons now get their default style from the theme diagnostics settings, if no color has been given in the
|
lazytanuki
force-pushed
the
icons
branch
2 times, most recently
from
116513a to
88dce0c
Compare
|
Rebased on master, gonna try to factorize some code between the theme and icon loaders, and add support for diff gutter icons next. |
|
Progress on this so far? Estimated time to final. |
If you have a look at the first post, there is a todolist. |
|
Ah, didn't see this. Thanks! |
|
When I input some word in file piker or symbol picker, the color of word has offset 2.
|
|
Has there been any conversation off-GitHub regarding the future of this PR? It's been over half a year since maintainers last interacted officially with this pull-request... I very much appreciate the work and effort put in by them, but I think it's a little much to leave @lazytanuki hanging in PR-limbo after his hard work on such a popular feature, seeing as he's still actively maintaining it and had questions left unanswered. Are there any specific items this PR needs to implement to get accepted? Thank you! |
|
Here's a little summary of interactions between this PR and the maintainers : At the start (June 2022), I was working with @sudormrfbin (a maintainer) on this PR to propose something that would address @archseer's concerns about icon support. With one of the maintainers on board, I thought okay it's worth spending time working on this. I contacted @archseer on Matrix to ask for a review on March 15, 2023. He told me that "it makes sense to have some type of file-type icon configuration" and that "the consensus so far has been to see if we can simplify [the configuration] a bit". This tells me that I can still spend time working on this, so I proposed something for a simpler configuration, but never got any answer. Then, two maintainers came here with comments that I perceived as a bit disdainful, to which I replied with per-point answers, hoping for a conversation. However, there was no conversation. Overall, I think I have been respectful of the maintainers time (I came on Matrix to ask for a review like twice in a year and a half, it's not too much). I also tried to provide detailed answers to everyone's concerns and be respectful. Nevertheless, the opposite is just not true. I have no issues with maintainers refusing a feature, that I can understand. I had to rebase this PR many times, with some very heavy ones. Lately there have been too many breaking changes and I just don't see myself maintaining this anymore given all of the above. I apologize to people who were using it in their own forks and who were supporting this work. This is not too far from what happened with #5768. This feature had 3 PRs, with heavy modifications asked from the maintainers, only to be thrown away because they want it as a plugin. Again, no issues with maintainers refusing a PR, but to me, asking for heavy modifications and then totally ignoring the PR's author is not okay. Contributors time is to be respected as well. I love this project and had many ideas for possible contributions. I especially wanted to invest time in the snippet engine, but this whole experience really threw me off from contributing to this project, at least for a little while. |
|
I agree that this could have been handled better and I regret not being more active on this PR. This PR and a couple of others have been sitting in my backlog and I feel particularly guilty when I see it's still not resolved. But we do have hundreds of PRs and issues open and it's been difficult to keep track of them all. I've approached this PR a couple times this year but didn't have a good solution for it. It's a feature I don't use so it's hard to come up with something adequate if I'm not the end user. Our reasoning is that we can accept something simple, but if the simple solution isn't adequate and we can't reach consensus with the PR author we'd rather see it implemented via a plugin down the road. I know it's a bit hand-wavy to offload it to plugins but we've at least been making progress on that front. Both this and the file picker are features I didn't particularly want, but was open to including based on popular demand -- since I'll have to end up owning these features that's where the push for simplification comes from. The consensus among maintainers has been that the design is too complex and we asked to remove the additional TOML config. The linked explanation meant to address that still seems to use an almost identical setup. |
|
Moreover though, from my perspective, @lazytanuki you haven't really taken our feedback. You say you want to have a conversation, but your interactions with us haven't really reflected that. You were given pretty direct feedback that this feature is more complicated than we want to support in core, but openness was expressed if the complexity could come down. You pointed previously to #2869 (comment) as your response to this feedback, but all you did was say "if we make it simpler, then we can't have all the features I want". That's not a discussion or a compromise, that's just an objection. Getting rid of an "Discussions" involve recognizing and acknowledging differences in values, and typically involve compromise or concession -- they are not simply responding point-for-point why you think the other person is wrong until you convince them. It's really hard to have a conversation with someone who does this, so I usually choose not to donate my time and brain space to such interactions, since they will more often than not be fruitless. Also, typically in the open source world (or really any software development in general), if you want to contribute a feature, especially a very complex one, you start with discussion to get buy-in from the outset, you don't start with a big investment of your own time and then fight for your change if it receives feedback you don't like. When you approach PRs like this, you set yourself up for conflict and bitterness over your own wasted time, but this is not the fault of the maintainers, who never asked for your time investment. |
|
Having looked at this again I actually don't see the need to make this configurable (at least not in this scale, with a separate config file and presets). Most font support patched-in nerdfonts (or implement them using the same offsets) so we should be able to hard-code the values. This would satisfy the vast majority of users and be basic enough to get merged (just a bunch of constants in an icons module). Some users would want to use their custom fonts or decide they want to use a different glyph than the one we've chosen, we can either not support that for now or add an override mechanism in a follow-up (that should just fit in |
| | Key | Description | Default | | ||
| | --- | --- | --- | | ||
| | `picker` | Whether icons in pickers are enabled. | `true` | | ||
| | `bufferline` | Whether icons in the buffer line are enabled. | `true` | | ||
| | `statusline` | Whether icons in the status line are enabled. | `true` | |
There was a problem hiding this comment.
I'd still keep this config though, it makes sense to enable support per UI area
| [diagnostic] | ||
| error = {icon = "●"} | ||
| warning = {icon = "●"} | ||
| info = {icon = "●"} | ||
| hint = {icon = "●"} | ||
|
|
||
| [breakpoint] | ||
| verified = {icon = "●"} | ||
| unverified = {icon = "◯"} | ||
| pause-indicator = {icon = "▶"} | ||
|
|
||
| [diff] | ||
| added = {icon = "▍"} | ||
| deleted = {icon = "▔"} | ||
| modified = {icon = "▍"} |
There was a problem hiding this comment.
To work around this config (where we want ASCII by default, icons if supported), I'd add a toplevel config setting icons = true that would switch over from ascii symbols to nerdfonts. There's precedence in true-color support working this way for example.
|
(I understand you're probably burnt out on this PR by now but figured I'd propose a path forward for anyone picking this up in the future) |
|
@archseer @dead10ck
Why don't we explicitly mention something about discussing a PR before the contributor goes ahead with the hope of getting it merged so as to forestall similar scenario from reoccurring in the future?
…On Nov 17, 2023, 21:34, at 21:34, Skyler Hawthorne ***@***.***> wrote:
Moreover though, from my perspective, @lazytanuki you haven't really
taken our feedback. You say you want to have a conversation, but your
interactions with us haven't really reflected that. You were given
pretty direct feedback that this feature is more complicated than we
want to support in core, but openness was expressed if the complexity
could come down. You pointed previously to
#2869 (comment)
as your response to this feedback, but all you did was say "if we make
it simpler, then we can't have all the features I want". That's not a
discussion or a compromise, that's just an objection. Getting rid of an
`icons.toml` and putting all that same exact config in the
`config.toml` completely misses the point -- it's not about the extra
file, it's about the extra configuration and all of the maintenance
burden that would come along with that.
"Discussions" involve recognizing and acknowledging differences in
values, and typically involve compromise or concession -- they are not
simply responding point-for-point why you think the other person is
wrong until you convince them. It's really hard to have a conversation
with someone who does this, so I usually choose not to donate my time
and brain space to such interactions, since they will more often than
not be fruitless.
Also, typically in the open source world, if you want to contribute a
feature, especially a very complex one, you *start* with discussion to
get buy-in from the outset, you don't start with a big investment of
your own time and then fight for your change if it receives feedback
you don't like.
--
Reply to this email directly or view it on GitHub:
#2869 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
Thanks for jumping in all of you.
@archseer: This is a great idea! If this direction suits you, I'll see if I can spare some time in the near future to work on this. I'll probably make a new PR, this one is becoming a bit cluttered. @dead10ck: If you don't like per-point answers that's alright, I'll just say that I find your depiction of events to be inaccurate. I don't think I have a problem with having discussions and making concessions. I am perfectly fine with archseer's proposed solution even if it removes a couple features. If somebody disagrees with you in a couple messages, it doesn't mean they suddenly cannot have proper discussions in general. They might just disagree with you, that happens. Please do not make such assumptions this quickly. @gyreas: Some projects such as Iced have an RFC system for bigger features, maybe that could be a nice solution here. However, I would like to point out that I started working on this with the help and approval of one of the maintainers of the time. I also felt like implementing a good part of it was nice for showing the idea behind it. My main concern was about mixed messages, not with rejection after having put in the work. |
|
I really hope this PR in one form or another gets merged! I just went to the bother of manually compiling @lazytanuki Thanks for sticking with this and not taking anything personally, very professional. |
|
Hi,
You have the wrong Deadlock!
I'm not part of helix
Regards,
Martin.
…On Sat, 18 Nov 2023, 8:42 pm lazytanuki, ***@***.***> wrote:
Thanks for jumping in all of you.
Having looked at this again I actually don't see the need to make this
configurable (at least not in this scale, with a separate config file and
presets). Most font support patched-in nerdfonts (or implement them using
the same offsets <https://github.com/slavfox/Cozette>) so we should be
able to hard-code the values. This would satisfy the vast majority of users
and be basic enough to get merged (just a bunch of constants in an icons
module).
Some users would want to use their custom fonts or decide they want to use
a different glyph than the one we've chosen, we can either not support that
for now or add an override mechanism in a follow-up (that should just fit
in config.toml).
@archseer <https://github.com/archseer>: This is a great idea! If this
direction suits you, I'll see if I can spare some time in the near future
to work on this. I'll probably make a new PR, this one is becoming a bit
cluttered.
@dead10ck <https://github.com/dead10ck>: If you don't like per-point
answers that's alright, I'll just say that I find your depiction of events
to be inaccurate. I don't think I have a problem with having discussions
and making concessions. I am perfectly fine with archseer's proposed
solution even if it removes a couple features. If somebody disagrees with
you in a couple messages, it doesn't mean they suddenly cannot have proper
discussions in general. They might just disagree with you, that happens.
Please do not make such assumptions this quickly.
@gyreas <https://github.com/gyreas>: Some projects such as Iced
<https://github.com/iced-rs/iced> have an RFC system for bigger features,
maybe that could be a nice solution here. However, I would like to point
out that I started working on this with the help and approval of one of the
maintainers of the time. I also felt like implementing a good part of it
was nice for showing the idea behind it. My main concern was about mixed
messages, not with rejection after having put in the work.
—
Reply to this email directly, view it on GitHub
<#2869 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABEHMPDKUIZRKT5VUHYKQUDYFEMSPAVCNFSM5ZVSWQZ2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBRG43DGMBRG44Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
likewise! |
Woops, really sorry, my brain can't leet-speak correctly it seems 😄 |
sorry, my bad 😅. it's a rather a curious coincidence tho |
|
Lol.
No worries.
It is a coincidence though
On Sat, 18 Nov 2023, 10:10 pm Saheed Adeleye, ***@***.***>
wrote:
… Hi,
You have the wrong Deadlock!
I'm not part of helix
Regards,
Martin.
On Sat, 18 Nov 2023, 8:42 pm lazytanuki, *@*.***> wrote:
Thanks for jumping in all of you.
Having looked at this again I actually don't see the need to make this
configurable (at least not in this scale, with a separate config file and
presets). Most font support patched-in nerdfonts (or implement them using
the same offsets https://github.com/slavfox/Cozette) so we should be
able to hard-code the values. This would satisfy the vast majority of users
and be basic enough to get merged (just a bunch of constants in an icons
module).
Some users would want to use their custom fonts or decide they want to use
a different glyph than the one we've chosen, we can either not support that
for now or add an override mechanism in a follow-up (that should just fit
in config.toml).
@archseer <https://github.com/archseer> https://github.com/archseer: This
is a great idea! If this
direction suits you, I'll see if I can spare some time in the near future
to work on this. I'll probably make a new PR, this one is becoming a bit
cluttered.
@dead10ck <https://github.com/dead10ck> https://github.com/dead10ck: If
you don't like per-point
answers that's alright, I'll just say that I find your depiction of events
to be inaccurate. I don't think I have a problem with having discussions
and making concessions. I am perfectly fine with archseer's proposed
solution even if it removes a couple features. If somebody disagrees with
you in a couple messages, it doesn't mean they suddenly cannot have proper
discussions in general. They might just disagree with you, that happens.
Please do not make such assumptions this quickly.
@gyreas <https://github.com/gyreas> https://github.com/gyreas: Some
projects such as Iced
https://github.com/iced-rs/iced have an RFC system for bigger features,
maybe that could be a nice solution here. However, I would like to point
out that I started working on this with the help and approval of one of the
maintainers of the time. I also felt like implementing a good part of it
was nice for showing the idea behind it. My main concern was about mixed
messages, not with rejection after having put in the work.
—
Reply to this email directly, view it on GitHub
#2869 (comment)
<#2869 (comment)>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABEHMPDKUIZRKT5VUHYKQUDYFEMSPAVCNFSM5ZVSWQZ2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBRG43DGMBRG44Q
.
You are receiving this because you were mentioned.Message ID:
*@*.***>
sorry, my bad 😅. it's a rather a curious coincidence tho
—
Reply to this email directly, view it on GitHub
<#2869 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABEHMPBPURULQ3RDKQFA4PLYFEW4ZAVCNFSM5ZVSWQZ2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBRG43DMMJXGE3Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fair point, but this will add more complexity to the base editor code base and additional dependencies, for such a niche feature, not everyone will use it, and it doesn't add any (real) feature. But it may end up to make the editor less usable in some scenarios, like tty users. |
The most complicated part from the current implementation was the configuration, which is to go away. The rest was fairly simple (it's just a unicode value with a style at some places). |
|
@lazytanuki There were breaking changes in Nerd Fonts v3, but the authors have stated that there won't be any more in the future, there is app to automatically check for obsolete font references: https://github.com/loichyan/nerdfix Also, I think it would be good to note that people really should not have to change their current fonts to some patched version! You can use the I am hoping that your new version will simply default to using Nerd Fonts v3 and won't bother with any other options to keep things simple :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@lazytanuki The branch is very far behind master. Have you got plans to update the branch and make changes from @David-Else comment at all or is this branch dead? |
|
@peteringram0 Afraid I have no idea if this branch is dead or not, I hope not as it was a very cool feature that I do miss. Sadly I don't think it has a future until plugins are available, but that was a decision that I strongly disagreed with. |
Add new theme highlight keys, for setting the colour of the breakpoint character and the current line at which execution has been paused at. The two new keys are `ui.highlight.frameline` and `ui.debug.breakpoint`. Highlight according to those keys, both the line at which debugging is paused at and the breakpoint indicator. Add an indicator for the current line at which execution is paused at, themed by the `ui.debug.active` theme scope. Update various themes to showcase how the new functionality works. Better icons are dependent on helix-editor#2869, and as such will be handled in the future, once it lands. Closes: helix-editor#5952 Signed-off-by: Filip Dutescu <[email protected]>
|
This looks like a great feature @lazytanuki - thanks for all the work! |
|
Any status updates? I would love to use and build this feature. |
Hello there !
I created this issue to gather some feedback about my proposition and have a discussion about icons support in Helix.
The goal here is to find a solution that suits everyone.
Current result
Kooha-2023-01-21-16-05-20.mp4
So here are a cons and pros lists, followed by the proposition to address these points.
The cons
Here are the opinions I could gather from other issues (which act as the "cons" list):
The pros
icons are aesthetically pleasing to many users and bring some variety.
icons can convey information faster. For instance, having icons on a tree browser brings a quick overview about the structure and the content of the elements that are presented to the user:
icons can convey information where words would be too inconvenient:
git branch
file-type
diagnostics and spinners (although they already have icons)
a statusline component to show current symbol location is on the way (which is a really nice and helpful feature !).
In this case, icons can bring information about the kind of symbol that are part of the scope-tree way better than words.
icons are modern, and Helix is the post-modern editor !
there already are icons in Helix. A single interface would be a more consistent way to configure and customize them (diagnostics, breakpoints, etc.)
we can have nice things !
Why I don't think icons support should be done as a plugin
Icons are to be used at multiple places throughout the code, not just on some command or callback, thus I think making a plugin for it would be quite the trouble for such a simple feature. Also, I think it would be nice to have a centralized way to source icons from within Helix, so everything remains nice and consistent, and without too much overhead.
Implementation
Icons support as a fine-grained, opt-in feature with a unified interface.
runtime/iconsor~/.config/helix/icons/. Defaults for other popular fonts should be user-submitted in the future.Objective
The goal of #2869 is to have a single, centralized interface to manage icons, or symbols, in helix. It also implements the display of symbols where none were shown before. Here are places that can now show symbols :
The PR also handles gutter symbols.
Icon flavors are treated like themes, and they even share some loader functions. They even do support inheritance as a consequence.
The default flavor doesn't change anything to the way helix currently looks. That way, it will not introduce odd behavior for new users if they don't have a certain font installed (which was to me the main concern for @archseer)
A nerd font flavor was introduced. To use it, users have to set
icons = "nerfonts"in their config.toml file, and the documentation warns them about the need to have said font installed to use it. Here, I talk mainly about nerd fonts because it is the one I added, but after this PR gets merged, users could contribute to other fonts defaults, much like themes.The benefit of having that kind of support is that it enables the representation of file type icons, and symbol kinds, which are not currently in helix. I tried to do so in a manner that does not introduce odd behavior for unaware users.
Kooha-2023-03-07-10-29-35.mp4
Also, it brings the possibility to have independent colors per icon. File type icons are essentially logos with predefined colors, which are here supported.
Configuration complexity
With this PR, all symbols and icons are set in a toml files just like themes. This makes for a tidier configuration than having them in the main configuration file. Moreover, if a user was to put all file type icons into the main configuration file, it would become enormous!
In addition: a lot of users want sane defaults, and I believe it is part of Helix's philosophy. With this PR, users who want to use file type icons and symbol kinds do not have to source them themselves, they can just use the preconfigured ones. If they want to adjust some icons, they can use inheritance, like themes.
In any case, at least to me it simplifies configuration more than the opposite.
Motivation
The motivation comes from a long time using neovim.
One of the things that made using neovim frustrating was that a lot of the time, several users would share the same part of configuration code that could have been implemented as a default. Also, with its plugins, we would sometime have plugins that were using symbols from different fonts, which would rapidly make it a giant mess.
Given that a plugin system might come one day, I think it is a good idea to have a main, user controlled API for icons, instead of reproducing the same problems as with neovim plugins.
It will also come in handy for future features, such as the file explorer. In order to display icons, it could just use the API, without worrying about whether to display icons or not, or about per-icon colors, or about using a bon standard font at all: all of this is handled here. Same would go for a symbol tree, and so on.
User cases
Now let's have some examples.
Without this PR, using their configuration on a machine that does not have said special font would make this look broken. Editing the configuration in every place where they set special icons would take time and overall be ugly.
With this PR, they could just set the default icons flavor back on said machines, or use a different font flavor.
PR changelog
runtime/iconsicon()function in theItemtrait for pickers items (file types, diagnostics, symbol kinds)ThemeandIconloader to enable TOML inheritancewait for File explorer and tree helper #2377 to be merged to add icons to trees (or not, it's been inactive for quite a while and is not a requirement for this PR)