FR : Make face used for line highlighting in biblio-selection-mode customizable

[aagon]

For the moment, defining a specific face and trying to set it as buffer local for hl-line-face in the biblio-selection-mode-hook does not work : the face used by hl-line-mode is still hl-line when we arrive in the biblio-selection-mode buffer.

It only works if the buffer-local setting is made in buffer-selection-mode before hl-line-mode is activated.

I propose the following :

• Create a face specific for line highlighting, like this :

  (defface biblio-highlight-extend-face `((t (:inherit highlight
                           ,@(and (>= emacs-major-version 27) '(:extend t)))))
  "Face used for highlighting lines."
  :group 'biblio-faces)

  It will inherit the highlight face, set by most themes (hence always with a high contrast to the non highlighted background), but will still be easily customizable.

• Set this face as the buffer-local face for hl-line-face in the definition of the minor mode, as such :

  (define-derived-mode biblio-selection-mode fundamental-mode biblio--selection-mode-name-base
  "Browse bibliographic search results.
  \\{biblio-selection-mode-map}"
  (set (make-local-variable 'hl-line-face) 'biblio-highlight-extend-face)
  (hl-line-mode 1)
  (visual-line-mode)
  (setq-local truncate-lines nil)
  (setq-local cursor-type nil)
  (setq-local buffer-read-only t)
  (setq-local mode-name '(:eval (biblio--selection-mode-name)))
  (setq-local
  header-line-format
  `(:eval
   (concat
    (ignore-errors
      (propertize " " 'display '(space :align-to 0) 'face 'fringe))
    (substitute-command-keys
     (biblio-join "   "
       "\\[biblio--selection-help]: Help"
       "\\[biblio--selection-insert],\\[biblio--selection-insert-quit]: Insert BibTex"
       "\\[biblio--selection-copy],\\[biblio--selection-copy-quit]: Copy BibTeX"
       "\\[biblio--selection-extended-action]: Extended action"
       "\\[biblio--selection-browse]: Open in browser"
       "\\[biblio--selection-change-buffer]: Change buffer"))))))

  This produces clearly visible and theme-consistent highlighting, regardless of theme.

If you agree, I can prepare the trivial PR with the changes.

Thanks again for this wonderful package,

Aymeric Agon-Rambosson

[cpitclaudel]

:+1: Sounds good to me. Shouldn't we inherit from hl-line instead of highlight? (and won't we inherit :extend from it? Of if it doesn't have :extend then why do we need it?)

Also, I prefer setq-local over set + make-local-variable

(It would be great to add a test for this, but it's not super high priority)

[aagon]

Shouldn't we inherit from hl-line instead of highlight?

Stock emacs makes hl-line inherit from highlight, but some themes (doom-themes in particular) set the background of each face differently : hl-line with low contrast to the normal background, to be used in an environment where the cursor is still visible, and highlight with high contrast to the normal background, perfect for environments where the cursor is hidden and the line highlighting is the only clue as to where the point is, as in biblio-selection-mode. In fact, I started this issue because with my theme (doom-molokai), the selected biblio entry was nigh-indistinguishable from the others, and setting a new face required modifying the minor mode.

So IMO best to inherit from highlight than from hl-line : doesn't change a thing for stock, but behaves better with some themes. And because we inherit from highlight, explicit :extend makes sense (I wanted to make the inheritance from highlight as similar as the of hl-line from highlight, which explicitly uses :extend).

Also, I prefer setq-local over set + make-local-variable

Done.

(It would be great to add a test for this, but it's not super high priority)

The only test I can think of would be to check whether the face is the right one in biblio-selection-mode, like this :

(it "has the right major mode"
            (with-current-buffer results-buffer
              (expect major-mode :to-equal 'biblio-selection-mode)))
;; New test :
(it "has the right face"
            (with-current-buffer results-buffer
              (expect (face-at-point) :to-equal 'biblio-highlight-extend-face)))

Let me know if you agree with this.

[cpitclaudel]

All of this makes complete sense. Thanks a lot.

[aagon]

Closed by #44