feat!: add basic support for icons#12369
feat!: add basic support for icons#12369RoloEdits wants to merge 2 commits intohelix-editor:masterfrom
Conversation
|
@darshanCommits Taking the discussion here. Yeah, I agree with the groupings, just need to figure out how that will go. As we dont really have snippets in full form, like no manager, and that I dont really use them, I think just one icon would also suffice? like a Currently I am using |
|
Also let it be known that I don't really use icons, or I guess feel the need to use them, but am trying to get in front of an issue with a solution. This is basically an RFC. I think at most I would only mess with the vcs icon, which I do now my using the separator option. I dont ever use the bufferline. And maybe the statusline filetype. I dont feel the need to have them in the file picker or the path completion, I think the recent theming is enough for me, so if this goes anywhere, I might leave that to someone else to do after the fact. |
At first I wasn't fully sold on this but now this doesn't seem bad. Laying the infrastructure to do all that seems good idea. Specially with the language icons. And a follow up PR for non-nerdfont vcs and completion icons(not LSP, just if it's coming from path, buffer, snippet or ls) were default that would be it for me already, which could also serve as fallback if nothing is defined in the editor.icons. And diagnostics, color works for me tbh, but since color is not the most accessible indicator, might as well have that in default. Reason as to why would be accessibility, every other form of icon, to me personally is more of an aesthetic decision. Rest of the work is better off delegate to user, or more precisely imo, to plugins in the far future. |
|
Cool stuff, wish to see this evolve into something |
|
@darshanCommits Yeah, the diagnostic icons are also different, taken from #12060
|
|
I think the part I might like the most here is that there would be no need for another config option to enable or disable the symbols. The out of box experience would be the same, and then adding to the config "adds them". Its entirely opt-in user side. opt-in in perhaps the most complete sense as they will need to go as far as they want to get what they desire. But it puts the burden on them to make sure the fonts they pick render well in their terminal emulator. This is actually why I used |
|
An example config that covers pretty much all my use cases (the lsp ones would be the last thing, in the completion menu or a breadcrumb, for example):
I think it still retains the helix minimal config spirit pretty well. |
|
Gonna open this for review as the main stuff is done, its just a matter of coming up with names and seeing if the concept itself it good. Main questions, besides if this is even an approach that is deemed acceptable, should I move all the editor config stuff over to Im going to leave the work for adding icons to the finder/completion to someone else for later, as its not something I am really sold on. |
|
There should probably be a default for the icons. I can see people copy pasting 400 line icon configurations into their configs, which isn't pretty. Meanwhile we could just have a default for them and people who want to have icons just enable an option |
|
Or alternatively we can have a dedicated file for icon configs It will be treated like themes You can specify in the config like this icon-pack = "nerd"This will load the In this case we won't need an enable/disable option for icons. People can just remove the |
These things are exactly the reason the other icons pr never got merged. And when I see this
My thoughts are to let them do that and maintain it. Like I mentioned before, fonts and terminal emulators are the wild west. Often I need to add an extra space to the right of the font to make them render the right size. Adding 400 icons and everyone running 100 different fonts on 20 different terminal emulators(new one just dropped the other day) sounds like a nightmare to me. |
Ok, honestly just getting this PR merge would already be a win! We can think about any sort of defaults or stuff like that later down the line :) |
|
My thoughts exactly. Its main purpose it to hopefully get out in front of some other PRs that have their own bespoke configs and solve a potential issue of merging those in, rather than them getting stuck on "we don't want any more config bloat". |
|
#2869 (comment): config-per-icon is the granularity we're trying to avoid. "Starter pack" configs or config distributions like this would inspire are antithetical to how we try to expose configuration, even if it means making some things non-configurable. Basic icon support, as far as I read into @archseer's comment, is an icons module (probably toplevel in |
This was that attempt to do these things at once. What I read from it was that the config in that PR allowed things like color changing and a bunch of other things, which is what I took from reading:
The part about hard coded values, to me, was like a stopgap that is simple, as he says its something he would end up needing to own:
As I said above, hard coded values present their own issues with rendering. Its not that someone wants to change the set to another one, as far as configurability, it could be that it doesn't even render correctly for them (too big, small, shifted, etc.).
This is already on the pipeline with #12151, #6646, and #12060. Each PR is implementing their own way to expose icons/symbols to the user through a config. What is the plan for those? If those PRs arent dead in the water, then this only(attempts to) unifies the interface. |
|
The scale of icons I also feel is perceived to be much larger than it really is. Most people arent interacting with all 216 currently supported languages. Which is the largest portion of the potential icons. The screenshot I have of my config, substituting here an there, probably covers 99% of people. |
|
I'm not sure how that "override mechanism" should work exactly and I'm not sure we'll need it at all so I would target the hardcoded values idea instead. We can always introduce more config later if different widths are a problem in practice.
These PRs seem to be consistently created for the purpose of setting an icon - see the description or comment images. Were it up to me alone I would not ever expose this granularity of configuration especially when it comes to cosmetics, and this applies to other cosmetics configuration like #12311 or the numerous PRs about the bufferline. I don't consider cosmetics a priority and I hate adding config options in general though so I may have an extreme opinion here - other maintainers might dissent.
Needing to set 20 config options unrelated to actual functionality seems like way too much to me. |
Its a known problem for me already. If this was to go through, despite me making the PR, it would be unusable for me.
This I think is the strong suit of this implimentation. You can add it if you want. If not, it changes nothing of how helix looks, and it doesn't need to be designed around in the future, which is another issue of hardcoding. And if you happen to only want to change a small subset of things, its clear how you would piecemeal it (with hopefully good naming and a logical abstraction in the config).
I see this a lot as something asked for and a few of the PRs doing them. I mentioned this issue here:
Im not a big icons guy myself, but either those PRs should be closed, or at least told that they wont merge, as this is the same issue here as there. Or if they are merged with no coordination, we have an ad-hoc config issue, which I think is something that no one wants. I agree having good defaults, and that should always be a priority, so I will see if I can think of ways to improve this, look around more and see what other TUIs are doing, but as far as maintenance is concerned, which im most worried about, this is currently only I set my 10 icons and by serendipity find them used later elsewhere without having to change more icons. So in the mode of set and forget. Not a daily back and forth with the config. |
|
Looking around, it looks like |
|
Id also argue that were are already near the maximum upper bound for complexity here as far as "domains" are concerned. The only missing symbols, as far as I can see, are for snippets and the dictionary. Both of which I believe are just one symbol each. Something like a And something like an Im not sure what wouldn't be covered by these, but like I said, I am not really a symbols person. The current literal text works for me just fine. |
|
Just saw icons = {
misc = {
dots = "",
},
ft = {
octo = "",
},
dap = {
Stopped = { " ", "DiagnosticWarn", "DapStoppedLine" },
Breakpoint = " ",
BreakpointCondition = " ",
BreakpointRejected = { " ", "DiagnosticError" },
LogPoint = ".>",
},
diagnostics = {
Error = " ",
Warn = " ",
Hint = " ",
Info = " ",
},
git = {
added = " ",
modified = " ",
removed = " ",
},
kinds = {
Array = " ",
Boolean = " ",
Class = " ",
Codeium = " ",
Color = " ",
Control = " ",
Collapsed = " ",
Constant = " ",
Constructor = " ",
Copilot = " ",
Enum = " ",
EnumMember = " ",
Event = " ",
Field = " ",
File = " ",
Folder = " ",
Function = " ",
Interface = " ",
Key = " ",
Keyword = " ",
Method = " ",
Module = " ",
Namespace = " ",
Null = " ",
Number = " ",
Object = " ",
Operator = " ",
Package = " ",
Property = " ",
Reference = " ",
Snippet = " ",
String = " ",
Struct = " ",
Supermaven = " ",
TabNine = " ",
Text = " ",
TypeParameter = " ",
Unit = " ",
Value = " ",
Variable = " ",
},
},Not sure what |
|
Is there a good example / minimal reproduction of something that wouldn't work correctly as a default? What I've read of that I was suspecting to be speculation but if there are already actual problems with width then an override system like @archseer mentioned could be a part of the "basic support". (On the technical side we would definitely want to use a small string optimization for this type.) But I would disagree with the comments above about not providing a default and requiring configuration for every glyph. |
For me its as simple as removing a space to the right: Ive also had spurious issues with alignment or skewing? Im not really sure how to describe it. Different terminals displayed it differently as well. Which is another issue. I'm on my third Ioskiva Nerd Font(😱) trying to find the best balance of issues.
Yeah that would be the start of my work once I can find a direction to go in.
If we keep the current configure everything way, as the override, then it should be easy enough to just add them to the default impl. And then just add a global override like before. Due to the design where everything is gotten through getters, the propagating should only need to be scoped to those getters. |
This comment was marked as outdated.
This comment was marked as outdated.
|
There is no toggle for the diagnostics as we already always use them. They can just be overridden. Same for the breakpoints. |
|
Now only this is needed to get what I have shown before: [editor.icons.vcs]
enable = true
[editor.icons.mime]
enable = trueI added the changes from #12060 making the icons you(@the-mikedavis) were last known using, the defaults. |
RoloEdits
force-pushed
the
icons-v2
branch
5 times, most recently
from
dc96799 to
cad25fe
Compare
RoloEdits
force-pushed
the
icons-v2
branch
14 times, most recently
from
479e6fd to
7183c22
Compare
|
Ok, I think we are ready for some light testing. I have (hopefully) structured things in a way that solve the reported problems. I think I should wait to fix the inadvertent overriding for another follow-up PR, as this one is already on the limits of what I would want for a feature like this. I have added a new padding field, which takes a I mentioned it way above, but terminals handle rendering wide width characters in various ways, and so it was chosen to default to a visual language where the icon is spaced away from whatever it is next to. This will hopefully let all terminals render them the best they can, and also required the above padding addition to achieve correctly. The other feedback was about the clashing colors, and for that I have shifted a lot of the burden to the themes. I think in the long term this is the best approach though, as the context of if a color is good or not is theme dependent. And example with the kind: "icons.kind.file" = { fg = "blue" }
"icons.kind.link" = { fg = "blue" }
"icons.kind.folder" = { fg = "blue" }
"icons.kind.module" = { fg = "mauve" }
"icons.kind.namespace" = { fg = "mauve" }
"icons.kind.package" = { fg = "mauve" }
"icons.kind.class" = { fg = "yellow" }
"icons.kind.method" = { fg = "blue" }
"icons.kind.property" = { fg = "lavender" }
"icons.kind.field" = { fg = "lavender" }
"icons.kind.construct" = { fg = "sapphire" }
"icons.kind.enum" = { fg = "teal" }An example with the filetypes: 'icons.file.LICENSE' = { fg = "green" }
'icons.file.README.md' = { fg = "green" }
'icons.file.rs' = { fg = "green" }The filetypes can also have an active style, which takes affect only on the bufferline(at least for now?): 'icons.file.rs.active' = { fg = "blue" }(I still haven't decided on the exact API here, so expect changes. Directories also haven't been added as there is an existing The main idea here is that most icons' brand colors should be fine, with only the most extreme color icons needing changing. For now this fully grained, meaning there are no groups, like now For the config, you can still have: [icons.vcs]
enabled = true
branch = "B"But almost always will you need to provide a padding: [icons.vcs]
enabled = true
branch = { glyph = "B", padding = { left = 1, right = 1 } }Beyond these fields, it also accepts the same inputs as the themes do ( The main philosophy for the colors and icons is that if there is a known brand, then pick those above all else. The model here is mostly https://github.com/nvim-tree/nvim-web-devicons. Beyond that, as long as a color is close, I think it should be desired to pick colors that fit the most themes, but only within reason of the brand. If there is not a brand color, picking something that fits the most themes should be prioritized. Anything else will be left to the theme makers. I can still go further in the themeing capabilities, but I want ease in as much as possible. I also added a bunch of icons from For now I am looking for more feedback into correctness + theme maintenance issues. I would start with testing the issues anyone has faced first, before merging into daily drivers, just to give a chance for quick feedback, and as a lot was changed, i'm sure there will be some issues. As part of this PR, I also plan to add a directory nested under the Thanks for the feedback! |
|
Ok, starting to refine the configuration stuff now. Will be updating regularly, but currently this should work in the themes config file: "icons.directory..git" = { fg = "orange" }This should also work with the user config: [icons.fs.directory]
".git" = { fg = "#FF0000" }Main thing left (as far as this current implementation goes) is to make sure the areas with the icons can be overridden. Currently only did so to this degree with the file picker/explorer and the bufferline. More will come quick (hopefully). I have also tried to use more balanced colors for a hopefully better default experience. Darkened a lot of the almost pure white colors. Changing default colors so that they work better on the most themes should be a goal striven for. As far as the future, I think we are close to an actual review + time in the hands of some testers, but I think we are pretty close to the goal of this PR. Anything more advanced can be worked on in follow-ups. Im sure there will need to be refinement with the addition of the steel engine, but cant really work on that until its merged into master. Also forgot to mention but the config syntax for extensions is: [icons.fs.file.extension]
rs = { fg = "#FF0000", bg = "#00FF00", underline = { color = "#000000", style = "line" } }There also a Refining this syntax is also something that will probably need to happen. |
The steel thing should not be more advanced than plumbing some |
|
Something that needs to be figured out is how styling should be scoped. For example, I have added an icon spot on the vcs statusline element. Following the line of thinking that the user should be able to override the icon itself, as well as the style, I could add an After trying to figure out how we should reconcile the different scopes and how we would cull them down, I think it makes sense to have a base icon themeing, including the user config override, while also still having the ui element specific styling capability. But I dont think it will be consistent between the different kinds of icons we have, as it doesnt fit the same way everywhere. Example priority: This layering should continue to fallback on the weaker layers, with patching all the way down. This would mean that theme authors can have enough control over the specific ui elements, while leaving an override for the end user. If a user wants to change the icon in a specific element, rather than everywhere, then they would have to make a custom theme; they can choose to underline the branch icon in the in the statusline, but not in the in-line git blame. Its not a "perfect" solution(whatever that means in this context), but in the helix philosophy it doesnt have to be. It should work out of the box, and for most cases for most people. As most people just use the builtin themes, giving enough control for the theme authors should be enough. The most subjective part for the icons is usually which glyph to use, which the current setup handles fairly well. And more than anything, I think we just need to see this out in the wild to actually figure out how it will actually be used, and just try to set ourselves up to be flexible, which I am also trying to do with how things are structured. |
| // FIX: Ruler doesn't draw to the statusline when bufferline and | ||
| // breadcrumbs are shown. | ||
| // | ||
| // `=area.height` worked when those things are enabled, but when | ||
| // disabled or not shown, the ruler draws over the statusline. | ||
| for y in area.y..area.height { | ||
| surface.set_string( | ||
| area.x, | ||
| y, | ||
| icons.ui().r#virtual().ruler().to_string(), | ||
| style, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I've shared the fix for this here #14453 (comment)
16 participants
Adds a centralized container for icons/symbols that is easy to propagate around to areas that use them. It tries to categorize around both a domain and UI element. UI elements are scoped to parts of the UI, like the statusline and bufferline, to avoid naming collisions and provide clear language to access an icon. Domains encompass something that can be used in multiple places and aren't attached to any specific UI element, like symbols for a struct or variable that can be used in the completion list, breadcrumb, and symbol pickers.
The main goal of this PR is to centralize and provide an API for a way to expose icon customization, rather than having a bunch of ad-hoc ways. It differs from #2869 in that it doesn't use any loader interface. It just uses plain values in a config, with defaults assigned in the source. Also breaking from standard config usage is that it is not attached to the
Editorbut instead a static container ofIcons. This makes propagating the icons very, very easy, and doesn't need any viral changes needed when passing anEditorplaces. This will hopefully make maintaining this low priority item, which the core maintainers might not themselves use or need, much nicer.Despite the unique way of handling the
icons, it still supports refresh on reload. Any patched font usage needs to be manually enabled in the config. Out of box should be similar (though perhaps new choices in icons/symbols) to now.helixitself uses symbols already, and they are moved to the new interface. This has the benefit of now having a uniform way to change things, rather than having aseparatorhere and anindicatorthere.[editor]can now define behavior while[icons]will define look.Icons are added to all pickers, the statusline, bufferline, and the completion list.
Here is an example config:
The goal, though, is to provide a satisfactory default experience inline with a neovim distro, continuing helix's excellent out of box experience.
Examples
Review Points
While the type for some of the symbols might say
String, its an alias added at the top of the file so thatSmartString<LazyCompact>doesnt need to be written everywhere.The icons for filetypes and their colors might take some extra effort to go over. There are many added currently, though unknown how many will want to be added initially. There could be color conflicts with some themes. I think this can only wait till its used in the wild to refine over time. This also adds functionality from other PRs, some done by myself, and others copied over. Im unsure if any will be stripped out so havent added any co-authors, so not sure how we can coordinate credit where its due. We can talk about that more when we get closer to the end.
One minor pain point, where im not sure if there is a better way to handle, is that when a file is listed in a picker, if there is no extension, like say
Makefile, it will show the fallback text, as there is no match found with the info available, in this case just the name. If you open the file it would work as expected in areas where there is more information about aDocument, like the bufferline and statusline:There are some tests in the rustdocs in
icons.rsbut for now areno_runas not sure how to decide what configuration.defaults to load for unit tests, as by default the icons are disabled. Not sure how if there is an existing way to accomplish this or will have to hack something together.Markdown documentation will be added once the final form has settled, or looks like its rounding into form.
Future
Icon Plugins
Another use could come in the form of icon plugins. By providing a single interface to alter icons, which are propagated in the most common areas, a plugin would just need to reassign to new values after the defaults are loaded. If a new icon is needed somewhere, it can be propagated easily.
Ruler Changes
This PR changes so that one can choose a new ruler symbol, but given that the practice right now is to set the BG color, rather than FG, a separate PR can change to a new default and also alter the themes necessary . One major issue around this is if people still want to have the current "space" character and have themes support that. I would image that there would be a check for a space in the code and then special case to clear the FG and set the BG with the themes FG, but that can be left to figure out.
Breaking changes
The following elements have been moved from there current location to the new one:
[editor.statusline.separator]->[icons.ui.statusline.separator][editor.whitespace.characters]->[icons.ui.virtual.*][editor.indent-guides.character]->[icons.ui.virtual.indent][editor.soft-wrap.wrap-indicator]->[icons.ui.virtual.wrap]Supersedes:
Consumes:
vcs:prefix toversion-controlstatusline element #6646Closes:
Related: