isamert / empv.el

An Emacs media player, media library manager, radio player, YouTube frontend
GNU General Public License v3.0
107 stars 19 forks source link

adding vertico as a dependency #10

Closed jasonhemann closed 2 years ago

jasonhemann commented 2 years ago

empv--completing-read relies on vertico-sort-function, but vertico was not required.

isamert commented 2 years ago

The reason empv--completing-read references to vertico-sort-function is that it's trying to disable/enable sorting for vertico users. Some functions requires absolutely no sorting (i.e. empv-playlist-select) as it may cause confusion. But as far as I know there isn't a generic solution to disable sorting on completing-read. vertico, selectrum, helm they all implement their custom interface to do sorting, so they all need to be handled differently. For vertico, you need to set vertico-sort-function to nil to disable sorting, for selectrum you need to set selectrum-should-sort to nil etc. empv--completing-read tries to handle these by binding these variables to nil or their original value. If these packages does not exist on the users system, this does not cause any problem as their bindings are simply ignored.

It may cause some linting problems though, but it's not a big deal.

jasonhemann commented 2 years ago

Ah, okay. Thank you @isamert for the detailed explanation. I might be doing something wrong on my system then; maybe you have advice. Right now, without vertico listed as a require at the top of the file, when I run e.g. empv-play-radio, I get back an error empv--get-radio-url: Symbol’s value as variable is void: vertico-sort-function. That's what originally made me think that I needed to require it. Any thoughts as to why that should be signalling an error? Perhaps I'm treating some file as lexically scoped when I oughtn't?

isamert commented 2 years ago

Good catch, I completely forgot to check if vertico-sort-function is bound or not before trying to assign it to itself. I should probably do general check if everything works as intended without the optional packages but the fix I applied should make the error you mentioned disappear. Thanks again!