feat: add basic support for icons #12369
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. |
|
Ok, did a few here as I wanted to see what scale would look like for a best effort default. 45 file types done. Further toggling can be discussed as we go on. For now it defaults to off, as expected.
|
|
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. |
|
Also forgot to say, if it wasn't obvious already, the names match what the |
|
The lsp one will need to be integrated into something so they can actually be seen, which #12151 does. We could also add the color swatch there if needed, but I think the best symbol is already chosen for it. |
|
Tried to get the spacing right for the bufferline (almost forgot about it), but given my history of font icon oddness, it really does need to be built and checked in editor by other people. |
Adds support for a basic and simple way to support icons/symbols in helix. This approach differs from #2869 in that there is only options to assign new icons, reducing a lot of the complexity; its just strings. There is no theming infrastructure or inheritance. It simply propagates the symbols around where they are used.
Here is an example usage:
Here is an example using
nerdfonts:This combines #6646, #12060 and would add a source for the lsp icons for #12151, but organizes how the icons are stored instead of each one having their own ad-hoc config option.