protesilaos / mct

Enhancements for the default minibuffer completion UI of Emacs.
https://protesilaos.com/emacs/mct
GNU General Public License v3.0
33 stars 3 forks source link

Conflict with global-hl-line-mode #1

Closed alternateved closed 2 years ago

alternateved commented 2 years ago

Hello Prot,

During usage of your mct package I've noticed that there is small inconsistency when cycling through completions in *Completions* buffer.

With global-hl-line-mode set to t, first completion has a blue stripe over it (sometimes also second but it is not consistent) and rest of rows get currently set active hl-line face. With global-hl-line-mode set to nil everything seems to work properly as you presented in demo on your YouTube channel.

I understand that the solution could be to either set hooks for hl-line-mode only for major modes where it is really needed or to remove hook for the *Completions* buffer, nevertheless I wanted to bring your attention to this behavior as it might not be intended.

Best wishes, alternateved

protesilaos commented 2 years ago

Hello @alternateved and thanks for taking the time to report this!

With global-hl-line-mode set to t, first completion has a blue stripe over it (sometimes also second but it is not consistent) and rest of rows get currently set active hl-line face. With global-hl-line-mode set to nil everything seems to work properly as you presented in your demo on your YouTube channel.

I can reproduce your problem, though I am not sure yet what the best solution is.

The reason hl-line takes precedence over our highlight is because we explicitly set its overlay to a lower priority: this way the optional stripes (the variable mct-apply-completion-stripes) are not hidden by the highlight.

I understand that the solution could be to either set hooks for hl-line-mode only for major modes where it is really needed or to remove hook for the *Completions* buffer, nevertheless I wanted to bring your attention to this behavior as it might not be intended.

That could do it. Though I still wonder whether we should change something in the code. Otherwise we have to write documentation about the issue.

-- Protesilaos Stavrou https://protesilaos.com

alternateved commented 2 years ago

It seems that below snippet of code might be enough to fix this in user configuration:

(add-hook 'completion-list-mode-hook (lambda () (setq-local global-hl-line-mode nil)))

That situation also got me wondering - I see that you are hardcoding the face of the highlighting with mct-highlight-candidate. Is there a way to somehow get this face from the currently loaded theme instead of hardcoding it for light and dark variants?

I apologise if that should have another thread. Apart from that inquiry, I think that it would be good to mention it somewhere in documentation that this conflict exists.

protesilaos commented 2 years ago

It seems that below snippet of code might be enough to fix this in user configuration:

(add-hook 'completion-list-mode-hook (lambda () (setq-local global-hl-line-mode nil)))

Good!

That situation also got me wondering - I see that you are hardcoding the face of the highlighting with mct-highlight-candidate. Is there a way to somehow get this face from the currently loaded theme instead of harcoding it for light and dark variants?

You mean to inherit the highlight face directly? That is perfectly fine. The current specification is there from the old days of the code and there is no pressing need to keep it that way. It could be simplified to this:

(defface mct-highlight-candidate
  '((t :inherit highlight :extend t))
  "Face for current candidate in the completions' buffer."
  :group 'mct)

Then themes who might support mct may choose to use their own style (I have to do that for the modus-themes as well).

I apologise if that should have another thread. Apart from that inquiry, I think that it would be good to mention it somewhere in documentation that this conflict exists.

I will include a note in the manual about this. Thanks again for looking into it!

-- Protesilaos Stavrou https://protesilaos.com

protesilaos commented 2 years ago

Just added the note to the manual:

** Avoid global-hl-line-mode in the Completions
:PROPERTIES:
:CUSTOM_ID: h:ee6aa793-3129-4cf0-8138-1224b6052546
:END:

MCT uses its own overlay to highlight the candidate at point.  To ensure
that it does not interfere with the optional stripes (provided by the
user option ~mct-apply-completion-stripes~) the highlight's priority is
set to a custom value.  This, in turn, means that when the user enables
~global-hl-line-mode~, its highlighted line takes precedence over the MCT
highlight.  The solution to this conflict is to disable the hl-line
locally for the =*Completions*= buffer like this:

#+begin_src emacs-lisp
(add-hook 'completion-list-mode-hook (lambda () (setq-local global-hl-line-mode nil)))
#+end_src
alternateved commented 2 years ago

Just added the note to the manual:

** Avoid global-hl-line-mode in the Completions
:PROPERTIES:
:CUSTOM_ID: h:ee6aa793-3129-4cf0-8138-1224b6052546
:END:

MCT uses its own overlay to highlight the candidate at point.  To ensure
that it does not interfere with the optional stripes (provided by the
user option ~mct-apply-completion-stripes~) the highlight's priority is
set to a custom value.  This, in turn, means that when the user enables
~global-hl-line-mode~, its highlighted line takes precedence over the MCT
highlight.  The solution to this conflict is to disable the hl-line
locally for the =*Completions*= buffer like this:

#+begin_src emacs-lisp
(add-hook 'completion-list-mode-hook (lambda () (setq-local global-hl-line-mode nil)))
#+end_src

That looks really good! Thank you for adding this. I think that might be it for this issue.

protesilaos commented 2 years ago

Very well! I am closing this issue.

[ I will update the modus-themes to add explicit support for mct-highlight-candidate in order to make it work with modus-themes-completions. ]