emacs-circe / circe

Circe, a Client for IRC in Emacs
GNU General Public License v3.0
391 stars 51 forks source link

lui-track-bar: option to use a fringe indicator instead of a bar #362

Closed jaor closed 4 years ago

jaor commented 4 years ago

And a new command to jump to last unread line. #

wasamasa commented 4 years ago

Thanks! One thing bothering me about this is that with this change the current name of the module becomes misleading because it's no longer necessarily a bar that's being displayed. Another is that this change assumes that there will be only two ways of signalling new messages since the last time you checked, it would be better for future extension to instead have a customizable holding a symbol of which way is currently enabled and a cond dispatching on that symbol (as opposed to the proposed boolean variable and an if dispatching on it).

jaor commented 4 years ago

your comments sound fair enough to me (even though i cannot think at the moment of other ways of signalling). how about renaming lui-track-bar to simply lui-track?

wasamasa commented 4 years ago

That could work. In any case, some measures are needed to preserve backwards compatibility, like creating aliases of the old variables/commands/functions

jaor commented 4 years ago

yes. please, let me know if/when you'd like me to do that. thanks.

jaor commented 4 years ago

i just went ahead and created lui-track. thanks.

jaor commented 4 years ago

not totally sure i understand. this function is new in this PR, so it's normal nobody used it :)

jaor commented 4 years ago

just curious: what's wrong with case? it's just an alias for cl-case...

wasamasa commented 4 years ago

This is what the rest of the codebase happens to do. That aside it's an official recommendation, to quote the Emacs Lisp reference:

• If you need Common Lisp extensions, use the ‘cl-lib’ library rather than the old ‘cl’ library. The latter does not use a clean namespace (i.e., its definitions do not start with a ‘cl-’ prefix). If your package loads ‘cl’ at run time, that could cause name clashes for users who don’t use that package.

There is no problem with using the ‘cl’ package at compile time, with ‘(eval-when-compile (require 'cl))’. That’s sufficient for using the macros in the ‘cl’ package, because the compiler expands them before generating the byte-code. It is still better to use the more modern ‘cl-lib’ in this case, though.

I've finally got around to test your changes and everything works fine, both in the case of my config unaware of the change and my config adjusted to make use of the new name and customizable. For this reason I've merged it as is and added some adjustments to mark the old functions and variables obsolete. Thanks for your patience and contribution!

jaor commented 4 years ago

thanks a lot, very glad to contribute!