search

jdtsmith / kind-icon

Completion kind text/icon prefix labelling for emacs in-region completion
GNU Affero General Public License v3.0
174 stars 4 forks source link

kind-icon-mapping has an odd format #4

Closed minad closed 2 years ago

minad commented 2 years ago

Why not (id :text "text" :icon "icon" :face face)? Again only a minor stylistic issue - your package looks good. These are all the things I've found during my brief review. I am looking forward to seeing your package on ELPA/MELPA! I mentioned this before, but GNU ELPA seems to make sense here since all involved components live there (svg-lib, corfu, company) and maybe soon kind-icon! Not sure if you like the assignment though.

jdtsmith commented 2 years ago

Not sure whey I didn't make a proper plist, but it actually does make the customize interface look a bit nicer (not sure if you've tried that, I even have a preview for the icon button). I'd like to get the preview in-buffer in customize (like the custom face widget) but don't know the widget system at all.

In terms of repo, I already have FSF assignment so no problem there. Makes sense to put it where corfu lives. I am waiting for you to discover a few more problems before submitting :).

jdtsmith commented 2 years ago

Another question is whether to require svg-lib. I think so, but it should work with pure text labels without any svg-lib. I'm also sensitive to the network-attached requirement. I'll open an issue on that.

minad commented 2 years ago

Not sure whey I didn't make a proper plist, but it actually does make the customize interface look a bit nicer (not sure if you've tried that, I even have a preview for the icon button). I'd like to get the preview in-buffer in customize (like the custom face widget) but don't know the widget system at all.

Shouldn't it be possible to achieve the same with a custom widget? Chosing the data format in order to get a nice customize interface is backwards imho.

Another question is whether to require svg-lib. I think so, but it should work with pure text labels without any svg-lib. I'm also sensitive to the network-attached requirement. I'll open an issue on that.

Honestly - I am not happy with the dependency. There is the network issue, which I strongly dislike. Furthermore you don't use much code from svg-lib. It boils down to maybe 50 lines. svg-lib overall seems like a pretty much unnecessary small library, you don't control and you cannot change according to your liking. For comparison, company also avoids unnecessary dependency and has its own svg scaling code.

jdtsmith commented 2 years ago

RE data structure, now I remember why — I really wanted to make sure the user includes a short-text entry, so I elevated that to a top-level element of the alist (even if setting by hand in their init).

No matter the data layout, a customize widget would be great, and ideally it would auto-download, style and show the icon right in the customize buffer (vs. my "Preview button"), just like the 'face customize style does. Sadly I know nothing about the widget library, but would be happy to take any help on this.

svg-lib worked quite hard to get the icons to fill 2x1, including flexible styles like :scale for scaling them (which the user can control), without leaving "gaps". In practice this is hard, in particular because emacs' svg support doesn't have graceful transparency fall-through. That's I think why company uses fixed size icons? The other advantage it brings is users can trivially pick whichever icon set they want without downloading anything. I just need to add some network resilience (see #5).

jdtsmith commented 2 years ago

In terms of svg-lib, I meant simply whether to require it in the package download. kind-icon should work fine (with text) without it.

minad commented 2 years ago

Okay, then you are probably right that svg-lib is a justified dependency. In particular if upstream improvements are to be expected. I am just wary in general of dependencies (left-pad problem) and many dependencies in the elisp ecosystem are also unnecessary and unidiomatic (dash, s, f, ht, etc).

Regarding the network - I dislike if package download things behind my back. But there are a few packages which do that. I am just generally wary of packages which access the network.

jdtsmith commented 2 years ago

I 100% agree with the over-proliferation of dependencies. I do think svg-lib adds some real value and to do svg rescaling and auto-download right I'd just end up recreating it (sans the progress bar etc. stuff). Of course in addition to "don't download things", there is the eventual issue that the particular download method gets broken, but I think the tradeoffs for real user convenience is worth this. I mean people download packages from MELPA willy-nilly... I bet they don't closely examine that code.

I've added a warning label in the icon section of the README mentioning the location to which svg-lib downloads its icons. Luckily the files downloaded are trivially short so easy to check:

<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"><svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" id="mdi-code-brackets" width="24" height="24" viewBox="0 0 24 24"><path d="M15,4V6H18V18H15V20H20V4M4,4V20H9V18H6V6H9V4H4Z" /></svg>

I could also consider adding another value to kind-icon-use-icons of 'cached, so only cached icons are used, and short-text otherwise. I'd need support from svg-lib on this. I.e. setup your favorite icons, preview them all, then disable further downloads (re-enabling if you want to change any or re-install). So far the maintainer has been responsive.

jdtsmith commented 2 years ago

I'll keep this format; I've mentioned that kind and short-text are both required in the README.