Helm-Icons: Add central file to manage icon support

[Thaodan]

Helm-icons provides provider independent icon support for Helm.
Each provider can be added to the abstraction logic with small
changes.
The only requirement is that the API is mostly the same as
all-the-icons or nerd-icons.

Where possible use the provider specific handler functions such
icon-for-mode or icon-for-file are used.

For actions there are no specific functions so there's a separate
wrapper for that between the supported providers. The wrapper
can also where possible use a better matching icon where available
in a provider.

The idea is to centralize the icon enabler and then individually
have the option to disable the icon support for each source.
Currently this doesn't work exactly.

The branch also contains other related changes such as:

• Allow for icons in basic file sources such as recentf
• Add icon to set bookmark action if enabled
• Use more detailed transformer function for bookmarks in all cases

My goal is to completely replace the external helm-icons package which
provided these features but missed some since it did not directly integrate with Helm.

The PR is WIP, especially the first commit could require some improvements.
The code is already much cleaner then before since much of the icon handling
is now done by the icon provider but some such as helm-hightlight-bookmark
could require some improvement as the bookmark to be mapped to it's major mode
could be done using a list with the symbol of the corresponding major-mode.

[thierryvolpiatto]

Björn Bidar ***@***.***> writes:

Helm-icons provides provider independent icon support for Helm. Each provider can be added to the abstraction logic with small changes. The only requirement is that the API is mostly the same as all-the-icons or nerd-icons.

Thanks for working on this.

Where possible use the provider specific handler functions such icon-for-mode or icon-for-file are used.

I tried previously to avoid this in places where performance is important, these functions make unnecessary checking and BTW are slow. Thus some of these functions like all-the-icons-icon-for-mode are wrong, returning a symbol (the mode in this case) instead of a string which lead to errors.

For actions there are no specific functions so there's a separate wrapper for that between the supported providers. The wrapper can also where possible use a better matching icon where available in a provider. The idea is to centralize the icon enabler and then individually have the option to disable the icon support for each source. Currently this doesn't work exactly. The branch also contains other related changes such as: • Allow for icons in basic file sources such as recentf • Add icon to set bookmark action if enabled • Use more detailed transformer function for bookmarks in all cases My goal is to completely replace the external helm-icons package which provided these features but missed some since it did not directly integrate with Helm. The PR is WIP, especially the first commit could require some improvements. The code is already much cleaner then before since much of the icon handling is now done by the icon provider but some such as helm-hightlight-bookmark could require some improvement as the bookmark to be mapped to it's major mode could be done using a list with the symbol of the corresponding major-mode.

Some comments: - No need to handle treemacs, which is not really an icon provider. - No need to handle Spacemacs, it is up to them to setup such things. - Avoid in helm code when-let/if-let, they are not compatible with old emacs. - Always compile with make to see warnings. Thanks.

…

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ You can view, comment on, or merge this pull request online at: #2701 Commit Summary • 28d34ce Helm-Icons: Add central file to manage icon support • 7e2fdeb Allow for icons in basic file sources such as recentf • bac0e2b fixup! Helm-Icons: Add central file to manage icon support • ae66b53 Add icon to set bookmark action if enabled • 7b48da2 Use more detailed transformer function for bookmarks in all cases File Changes (7 files) • M helm-bookmark.el (79) • M helm-buffers.el (29) • M helm-files.el (51) • M helm-for-files.el (36) • M helm-help.el (2) • A helm-icons.el (191) • M helm-imenu.el (174) Patch Links: • https://github.com/emacs-helm/helm/pull/2701.patch • https://github.com/emacs-helm/helm/pull/2701.diff — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.*Message ID: ***@***.***>

-- Thierry

[Thaodan]

Thierry Volpiatto ***@***.***> writes:

> Where possible use the provider specific handler functions such > icon-for-mode or icon-for-file are used. I tried previously to avoid this in places where performance is important, these functions make unnecessary checking and BTW are slow.

The checks that you ended up doing o look very much the same like the ones ended up in Helm.

Thus some of these functions like all-the-icons-icon-for-mode are wrong, returning a symbol (the mode in this case) instead of a string which lead to errors.

They return the icon not a string. Just checked.

> For actions there are no specific functions so there's a separate > wrapper for that between the supported providers. The wrapper > can also where possible use a better matching icon where available > in a provider. > > The idea is to centralize the icon enabler and then individually > have the option to disable the icon support for each source. > Currently this doesn't work exactly. > > The branch also contains other related changes such as: > > • Allow for icons in basic file sources such as recentf > • Add icon to set bookmark action if enabled > • Use more detailed transformer function for bookmarks in all cases > > My goal is to completely replace the external helm-icons package which > provided these features but missed some since it did not directly integrate with Helm. > > The PR is WIP, especially the first commit could require some improvements. > The code is already much cleaner then before since much of the icon handling > is now done by the icon provider but some such as helm-hightlight-bookmark > could require some improvement as the bookmark to be mapped to it's major mode > could be done using a list with the symbol of the corresponding major-mode. Some comments: - No need to handle treemacs, which is not really an icon provider. - No need to handle Spacemacs, it is up to them to setup such things.

I didn't handle Spacemacs or did I?

- Avoid in helm code when-let/if-let, they are not compatible with old emacs.

What is "old"? These are available since Emacs 25.1 which already quite old. 25.1 is also the minimum version for helm so it should be fine. Supporting more than the last two major versions isn't a good idea IMHO.

[thierryvolpiatto]

Björn Bidar ***@***.***> writes:

1. ( ) text/plain (*) text/html Thierry Volpiatto ***@***.***> writes: >> Where possible use the provider specific handler functions such >> icon-for-mode or icon-for-file are used. > > I tried previously to avoid this in places where performance is > important, these functions make unnecessary checking and BTW are slow. The checks that you ended up doing o look very much the same like the ones ended up in Helm.

Sorry I don't understand what you mean.

> Thus some of these functions like all-the-icons-icon-for-mode are wrong, > returning a symbol (the mode in this case) instead of a string which > lead to errors. They return the icon not a string. Just checked.

Yes sorry, the icon, but when it fails to find an icon it returns a symbol which is plain wrong (your bookmark code fails immediately with error). Please try your code with both nerd-icons and all-the-icons.

I didn't handle Spacemacs or did I?

(defcustom helm-icons-mode-icons-alist '((dired-mode . "dir-closed") (emacs-lisp-mode . "el") (spacemacs-buffer-mode . "el")) "Alist describing which mode should use which icon. If MODE isn't contained in list fallback to icon provider." :type '(alist :key-type symbol :value-type sexp))

> - Avoid in helm code when-let/if-let, they are not compatible with > old emacs. What is "old"? These are available since Emacs 25.1 which already quite old. 25.1 is also the minimum version for helm so it should be fine.

No it is not, it needs a require (subr-x), anyway I don't want these in Helm code.

Supporting more than the last two major versions isn't a good idea IMHO.

For now we try to support 25.1, we have still some users using this version (and others 26 etc..). I may limit support to 26 once emacs-30 is out.

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: ***@***.***>

-- Thierry

[thierryvolpiatto]

Also, note that helm-icons.el is not usable as it will clash with the existing helm-icons package.

[Thaodan]

Thierry Volpiatto ***@***.***> writes:

Björn Bidar ***@***.***> writes: > 1. ( ) text/plain (*) text/html > > Thierry Volpiatto ***@***.***> writes: > >>> Where possible use the provider specific handler functions such >>> icon-for-mode or icon-for-file are used. >> >> I tried previously to avoid this in places where performance is >> important, these functions make unnecessary checking and BTW are slow. > > The checks that you ended up doing o look very much the same like the > ones ended up in Helm. Sorry I don't understand what you mean.

The checks that Helm did to check e.g. for a directory which is associated with a specific directory or if a mode has a specific icon associated with it do look very much like the same checks that all-the-icons and nerd-icons do.

>> Thus some of these functions like all-the-icons-icon-for-mode are wrong, >> returning a symbol (the mode in this case) instead of a string which >> lead to errors. > > They return the icon not a string. Just checked. Yes sorry, the icon, but when it fails to find an icon it returns a symbol which is plain wrong (your bookmark code fails immediately with error). Please try your code with both nerd-icons and all-the-icons.

I tried my code with both but I didn't test the case when an icon wasn't found. Did you report this bug to all-the-icons? The docstring of none of the all-the-icons functions state that a symbol should be returned in case no match was found.

> I didn't handle Spacemacs or did I? (defcustom helm-icons-mode-icons-alist '((dired-mode . "dir-closed") (emacs-lisp-mode . "el") (spacemacs-buffer-mode . "el")) "Alist describing which mode should use which icon. If MODE isn't contained in list fallback to icon provider." :type '(alist :key-type symbol :value-type sexp))

Ah I that was a workaround that isn't required anymore. I'll remove it.

>> - Avoid in helm code when-let/if-let, they are not compatible with >> old emacs. > What is "old"? These are available since Emacs 25.1 which already quite > old. > 25.1 is also the minimum version for helm so it should be fine. No it is not, it needs a require (subr-x), anyway I don't want these in Helm code.

It's a lisp macro and subr-x preloaded. What's the issue with subr-x? Emacs 25 is from 2016 that's old IMHO, if one uses such an old Emacsen that it is very likely that they don't but no need to argue.

> Supporting more than the last two major versions isn't a good idea IMHO. For now we try to support 25.1, we have still some users using this version (and others 26 etc..). I may limit support to 26 once emacs-30 is out.

Another option to support older Emacsen and move away from deprecated features is to use the compat package which backports some features to older Emacsen. On a site note: I noticed that cl-lib is loaded also when not compiling. Does Helm use any functions (not macros) of cl-lib?

[Thaodan]

Both helm-icons.el and the helm-icons package provide the same functionality. What is the use of having both installed in parallel? The existing icon support in Helm already clashed the helm-icons package.

[thierryvolpiatto]

Björn Bidar ***@***.***> writes:

1. ( ) text/plain (*) text/html Thierry Volpiatto ***@***.***> writes: > Björn Bidar ***@***.***> writes: > >> 1. ( ) text/plain (*) text/html >> >> Thierry Volpiatto ***@***.***> writes: >> >>>> Where possible use the provider specific handler functions such >>>> icon-for-mode or icon-for-file are used. >>> >>> I tried previously to avoid this in places where performance is >>> important, these functions make unnecessary checking and BTW are slow. >> >> The checks that you ended up doing o look very much the same like the >> ones ended up in Helm. > > Sorry I don't understand what you mean. The checks that Helm did to check e.g. for a directory which is associated with a specific directory or if a mode has a specific icon associated with it do look very much like the same checks that all-the-icons and nerd-icons do.

It is not, especially for directories, see all-the-icons-icon-for-dir function.

>>> Thus some of these functions like all-the-icons-icon-for-mode are wrong, >>> returning a symbol (the mode in this case) instead of a string which >>> lead to errors. >> >> They return the icon not a string. Just checked. > > Yes sorry, the icon, but when it fails to find an icon it returns a > symbol which is plain wrong (your bookmark code fails immediately with > error). Please try your code with both nerd-icons and all-the-icons. I tried my code with both but I didn't test the case when an icon wasn't found.

Thus many lines don't have icons for unknown reasons.

Did you report this bug to all-the-icons? The docstring of none of the all-the-icons functions state that a symbol should be returned in case no match was found.

See the function all-the-icons-icon-for-mode.

>> I didn't handle Spacemacs or did I? > > (defcustom helm-icons-mode-icons-alist > '((dired-mode . "dir-closed") > (emacs-lisp-mode . "el") > (spacemacs-buffer-mode . "el")) > "Alist describing which mode should use which icon. > > If MODE isn't contained in list fallback to icon provider." > :type '(alist :key-type symbol :value-type sexp)) Ah I that was a workaround that isn't required anymore. I'll remove it. >>> - Avoid in helm code when-let/if-let, they are not compatible with >>> old emacs. > >> What is "old"? These are available since Emacs 25.1 which already quite >> old. >> 25.1 is also the minimum version for helm so it should be fine. > > No it is not, it needs a require (subr-x), anyway I don't want these in > Helm code. It's a lisp macro and subr-x preloaded.

No it is not preloaded.

What's the issue with subr-x? Emacs 25 is from 2016 that's old IMHO, if one uses such an old Emacsen that it is very likely that they don't but no need to argue.

Please let me decide what is good for helm.

>> Supporting more than the last two major versions isn't a good idea IMHO. > > For now we try to support 25.1, we have still some users using this > version (and others 26 etc..). I may limit support to 26 once emacs-30 > is out. Another option to support older Emacsen and move away from deprecated features is to use the compat package which backports some features to older Emacsen.

No, I don't want compat package.

On a site note: I noticed that cl-lib is loaded also when not compiling. Does Helm use any functions (not macros) of cl-lib?

Yes.

…

-- Thierry

[thierryvolpiatto]

Björn Bidar ***@***.***> writes:

1. ( ) text/plain (*) text/html Both helm-icons.el and the helm-icons package provide the same functionality. What is the use of having both installed in parallel?

Nothing, but be sure some people will have both installed.

The existing icon support in Helm already clashed the helm-icons package.

Yes, but at least names are not clashing.

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: ***@***.***>

-- Thierry

[Thaodan]

Thierry Volpiatto ***@***.***> writes:

Björn Bidar ***@***.***> writes: > Thierry Volpiatto ***@***.***> writes: > > The checks that Helm did to check e.g. for a directory which is > associated with a specific directory or if a mode has a specific > icon associated with it do look very much like the same checks that > all-the-icons and nerd-icons do. It is not, especially for directories, see all-the-icons-icon-for-dir function.

I'm comparing helm vs. nerd-icons/all-the-icons-icon-for-dir functions function calls till the icon is returned: - Helm: - helm-basename - file-name-nondirectory - directory-file-name - loop over all parts of the file extension via file-name-sans-extension - helm-basedir - file-name-directory - cl-loop - directory-file-name - file-name-as-directory - all-the-icons-match-to-alist+apply/all-the-icons-octicon "file-directory" Nerd-Icons/All-the-icons - file-name-base - all-the-icons-match-to-alist/nerd-icons-match-to-alist - expand-file-name - file-remote-p - file-symlink-p - file-exist-p dir/.git - Check if directory is submodule The difference in both seems to be the more detailed checks in helm-basename and helm-basedir vs. the predicates used by nerd-icons/all-the-icons for the symlink/remote detection.

>>>> Thus some of these functions like all-the-icons-icon-for-mode are wrong, >>>> returning a symbol (the mode in this case) instead of a string which >>>> lead to errors. >>> >>> They return the icon not a string. Just checked. >> >> Yes sorry, the icon, but when it fails to find an icon it returns a >> symbol which is plain wrong (your bookmark code fails immediately with >> error). Please try your code with both nerd-icons and all-the-icons. > > I tried my code with both but I didn't test the case when an icon wasn't > found. Thus many lines don't have icons for unknown reasons.

Many lines? Please be more verbose on what you mean. Anyway I only explained my reasoning. From my own testing with nerd-icons no lines have no icons. Anyway I will check all-the-icons too.

> Did you report this bug to all-the-icons? The docstring of none of the > all-the-icons functions state that a symbol should be returned in case > no match was found. See the function all-the-icons-icon-for-mode.

The function docstring doesn't say anything about what should happen in the error only that it returns the formatted icon for mode, not that it returns the symbol for the mode which it couldn't match.

>> >> No it is not, it needs a require (subr-x), anyway I don't want these in >> Helm code. > > It's a lisp macro and subr-x preloaded. No it is not preloaded.

Oh yeahs seems so. Anyway it's a macro so it is only loaded at compile time similar to cl-lib which is already used (at runtime).

> What's the issue with subr-x? Emacs 25 is from 2016 that's old IMHO, > if one uses such an old Emacsen that it is very likely that they don't > but no need to argue. Please let me decide what is good for helm.

Sure I didn't mean to tell you what is good for Helm but was merely asking what is the issue with subr-x macros I used here specifically outside of personal preference.

> On a site note: I noticed that cl-lib is loaded also when not compiling. > Does Helm use any functions (not macros) of cl-lib? Yes.

Which ones? The only one I could found was cl-subseq from cl-extra and cl-second in cl-lib.el an alias to cadr. The other uses where macros from cl-lib or cl-macs.el.

[Thaodan]

Thierry Volpiatto ***@***.***> writes:

Björn Bidar ***@***.***> writes: > Both helm-icons.el and the helm-icons package provide the same > functionality. > > What is the use of having both installed in parallel? Nothing, but be sure some people will have both installed.

Of course but there is no broken functionality. Helm-icons took a quite common name for such a functionality it was foreseeable that it will conflict. A announcement in the NEWS file to remove it should be sufficient.

> The existing icon support in Helm already clashed the helm-icons package. Yes, but at least names are not clashing.

OK, I give you that one.

[thierryvolpiatto]

I have pushed new changes that provide compatibility between all-the-icons and nerd-icons, the new file is called helm-x-icons.el.
If you want to do changes, please do on the base of this work.
Thanks.

[Thaodan]

Why do you first say you welcome my changes and then replicate them almost entire in addition to your comments? Without any credit. Further you unconditionally load both packages always no matter which provider is selected. Just replicating changes you get from contributors instead of collaborating with them on the changes they want to contribute is just disheartening. In this context (code) review comments would have been much more helpful instead of recreating my work.

[thierryvolpiatto]

Björn Bidar ***@***.***> writes:

Just replicating changes you get from contributors instead of collaborating with them on the changes they want to contribute is just disheartening.

I didn't replicate your changes, helm-x-icons is implemented completely differently.

In this context (code) review comments would have been much more helpful instead of recreating my work.

I asked you to change things but it seems you didn't agree.

…

-- Thierry

[Thaodan]

Thierry Volpiatto ***@***.***> writes:

Björn Bidar ***@***.***> writes: > Just replicating changes you get from contributors instead of > collaborating with them on the changes they want to contribute > is just disheartening. I didn't replicate your changes, helm-x-icons is implemented completely differently.

Agree to disagree.

> In this context (code) review comments would have been much more helpful > instead of recreating my work. I asked you to change things but it seems you didn't agree.

From my point of view you dictated changes rather than trying to agree to changes. I tried to point out my point of view and/or to understand yours, now it seems like that more a do it like I want or else situation. In any case just because I replied to your comments before making code changes doesn't mean I don't do them. To me it's quite telling that when I criticized something that there was no engagement with that criticism.