Selectrum: Force background on current candidate

[angrybacon]

I am trying to get rid of the background in candidates that are not selected. Currently my Org buffers have code blocks highlighted with a background set on the org-block face. When listing candidates for instance with consult-line, the background for lines in Org blocks also appears in the result buffer.

Selecting a line with no background set:

Selecting a line with background set (org-block)

I've tried adding a custom highlight function but the returned candidate string doesn't seem to use properties to render colors in the candidate buffer.

(use-package selectrum
  :custom
  (selectrum-extend-current-candidate-highlight t)
  (selectrum-fix-vertical-window-height t)
  (selectrum-highlight-candidates-function 'me/selectrum-candidates-identity-no-background))

(defun me/selectrum-candidates-identity-no-background (_input candidates)
  "Return CANDIDATES stripped of their background."
  (mapcar (lambda (candidate)
            (let* ((properties (get-text-property 0 'face candidate))
                   (face (plist-put properties :background 'unspecified)))
              (put-text-property 0 (length candidate) 'face face candidate)
              candidate))

What am I missing?

[minad]

I've also seen that issue. I think it should generally be addressed on the level of the theme.

[angrybacon]

@minad I'm not very fluent in the arcana of theming and text properties but I've faced this issue with all the themes I've used in the past and with swiper too. In my above attempt I couldn't identify where the background was coming from in the first place. Specifically, in the value carried by candidate within the mapcar lambda, where are the colors coming from if not from text properties?

[clemera]

@angrybacon The colors are coming from the text properties, if you strip them away like in you example the backgrounds should be gone, though your example applies the face of the first character to the whole candidate which is probably unintended. I'm also no expert when it comes to theming and faces, maybe it would make sense that the selection face of the current candidate always has precedence so one can always tell which is selected which isn't currently the case:

(completing-read "Check: "
         (list (propertize "this" 'face '(:background "red"))
               (propertize "out" 'face '(:background "green"))))

[angrybacon]

My attempt while buggy (thanks!) is not ideal as other users might prefer other properties than background to highlight the current candidate.

maybe it would make sense that the selection face of the current candidate always has precedence

So this sounds like a better idea :+1:

[clemera]

I hope it is okay to bat-signal @protesilaos here. Do you have any opinions/suggestions on this?

[protesilaos]

I think there are two issues here: (1) how some text properties are applied, (2) how Selectrum chooses to apply its current line highlight.

With regard to point 1, this is due to how Org adds text properties. You can get a sense of this if you let selectrum-current-candidate have a bold weight: every part of the text that is not propertised by Org will turn bold, while those that are, will not change at all. This is why the underlying background is retained. And I think this is down to Org, because I get the desired behaviour when I try it over other buffers that have existing styles, e.g M-x list-faces-display followed by M-x consult-line.

For point 2, point 1 is not relevant for all face-related blends as it only happens with Selectrum. I do not have any problem when I set embark's collections to highlight the current line (with hl-line-mode, albeit with a context-specific color). Here is the screenshot with both Embark completions and Selectrum working simultaneously (which is not what I normally do). The blue line is Embark's, the gray one in the minibuffer belongs to Selectrum.

[clemera]

In Selectrum we currently append the selection face but we could choose to prepend it instead. hl-line-mode uses an overlay this way its face always has precedence, by prepending the selection face we would mimic this behaviour I think?

[clemera]

@protesilaos Also thank you for looking into this!

[protesilaos]

You are welcome! I think changing that will affect how the current line intersects with the matching characters. I have a faint memory of this being discussed in the past in this repo's issue tracker, but I cannot find it now. Basically the issue was to allow characters to be "above" the current line, whereas the previous style (and the one you see in my screenshot for Embark) was for the current line to take precedence and, thus, override the character's faces.

[protesilaos]

This is the issue: https://github.com/raxod502/selectrum/issues/21

EDIT: Though this does not seem to be relevant, after all.

EDIT 2: Ignore the previous edit. This diff fixes the issue, but overrides the matches as I expected:

--- /home/prot/.emacs.d/elpa/selectrum-20210211.1016/selectrum.el
+++ #<buffer selectrum.el>
@@ -1519,7 +1519,7 @@
     (add-face-text-property
      0 (length str)
      face
-     'append str))
+     nil str))
   str)

 (defun selectrum--affixate (fun candidates)

[clemera]

I tested it and the matched chars are still shown except for the case that the match highlighting does use the same properties as selectrum-current-candidate. Currently selectrum-current-candidate inherits from highlight. Is it common that highlight uses other properties than background in themes? Maybe it would better to only allow to define a background color for the selection (instead of a face) in Selectrum to avoid potential issues of that nature.

[protesilaos]

Personally, I am fine with whatever you consider best for Selectrum. Check my edit 2 here: https://github.com/raxod502/selectrum/issues/425#issuecomment-777520569

[clemera]

I did not consider to use background color to highlight matches :) Your example isn't optimal but probably better than a cut off/not visible selection.

[clemera]

Personally my match faces use :foreground to highlight the matches, which looks great and you won't be aware of the issue. When :background is used it would be nice to merge the backgrounds somehow so one can still see the matched parts through the highlighting.

[clemera]

In your example it seems your highlight face from which selectrum-current-candidate inherits does also define a :foreground? We should probably not inherit from highlight and just define a :background so the selection doesn't override the foreground in case the highlight face sets it (or just allow to define a background color as I suggested earlier).

[clemera]

When :background is used it would be nice to merge the backgrounds somehow so one can still see the matched parts through the highlighting.

I just discovered ivy is using the colir library for exactly this purpose.

[minad]

I just discovered ivy is using the colir library for exactly this purpose.

My preference would be to not implement such automatic merging, but only use explicit faces. I didn't read through the discussion here - I hope this issue can be fixed without such trickery.

[protesilaos]

In your example it seems your highlight face from which selectrum-current-candidate inherits does also define a :foreground?

Yes, it does. But this is due to the accessibility requirements of the themes I'm using. Otherwise I think it makes sense to just use a background as you suggest.

[clemera]

What about the following: Keep things like they are but prepend the :background of selectrum-current-candidate face. This will still be a bit unpleasant if the matched chars are indicated by background color only but overall seems like a good compromise.

[clemera]

@angrybacon The background of selectrum-current-candidate gets now prepended by default, could you check if #431 fixes your issue?

[angrybacon]

@clemera Thank you for the quick update, I've tested and replied directly on the commit with more details https://github.com/raxod502/selectrum/pull/431/files?file-filters%5B%5D=.el#r575172859

[clemera]

The problem is we can't simply remove the background color of a candidate because we don't know which parts belong to the candidate and which parts are colored from match highlighting, if any. If highlighting would always be a separate step it could be handled but completion-styles do filtering and highlighting in one step so we can't intercept otherwise we could prepend the selection face on the current candidate before match highlighting is done.

[clemera]

While playing around with different ways to indicate the selection I found an interesting approach to indicate the selected candidate by inserting separators with the insertion function, posting it here for later reference:

(defun selectrum--vertical-display-style
    (win cb nrows ncols
         &optional index max-index first-index-displayed last-index-displayed
         max-num
         settings)
  (ignore ncols first-index-displayed last-index-displayed settings)
  (let* ((rows (or max-num nrows))
         (first-index-displayed
          (if (not index)
              0
            (selectrum--clamp
             (1+ (- index (max 1 (/ rows 2))))
             0
             (max (- (1+ max-index) rows)
                  0))))
         (displayed-candidates
          (funcall cb first-index-displayed rows)))
    (when (window-minibuffer-p win)
      (insert "\n"))
    (let ((n 0))                                  
      (dolist (cand displayed-candidates)
        (let ((sep (propertize ;; Display hack from magit
                    (concat (propertize "\s" 'display '(space :height (2)))
                            (propertize "\n" 'line-height t)))))
          (when (eq n (- index first-index-displayed))
            (setq sep (propertize sep
                                  'face '(:background "black" :extend t))))
          (insert sep)
          (insert cand "\n")
          (insert sep))
        (cl-incf n))
      n)))

This is like having an extended underline + overline but also avoids any "bumping" of the candidates when changing the selection. It looks like this:

Yet another option is to display indicators in the fringes, for example using:

(propertize
 "x" 'display
 `(left-fringe right-arrow (:background "black")))

[protesilaos]

(propertize
"x" 'display
`(left-fringe right-arrow (:background "black")))

What happens if someone disables the fringes with (fringe-mode 0)?

[clemera]

What happens if someone disables the fringes with (fringe-mode 0)?

Then they wouldn't be able to see it ;) I don't propose to use this as a general workaround for the issue, just exploring different ways how the selection could be customized, sorry for the confusion.

[clemera]

Turns out the bubbling can easily be disabled by setting overline-margin.

[angrybacon]

@clemera I gave it another shot and tried to instead unset the background for all candidates earlier in the insertion. That is to avoid messing with prefix and suffix that might be concatenated to the candidate as well as overriding the eventual properties set by completion matching faces (orderless in my case). This should be controlled by a defcustom (nil by default), but before I commit to a working PR, I'd like to find out why the simplest implementation doesn't do what I expect :thinking:

 (defun selectrum--display-string (str)
   "Return display string of STR.
 Any string display specs in STR are replaced with the string they
 will display as. This avoids prompt bleeding issues that occur
 with display specs used within the after-string overlay."
   (let ((len (length str))
         (pos 0)
         (chunks ()))
     (while (not (eq pos len))
       (let* ((end (next-single-property-change pos 'display str len))
              (display (get-text-property pos 'display str))
              (chunk (if (stringp display)
                         display
                       (substring str pos end))))
+        (let* ((faces (get-text-property 0 'face chunk))
+               (face `(:background nil :inherit ,faces)))
+          (put-text-property 0 (length chunk) 'face face chunk))
         (push chunk chunks)
         (setq pos end)))
     (apply #'concat (nreverse chunks))))

I'm testing with this:

(completing-read "Check: " `(,(propertize "1 Lorem ipsum" 'face '(:background "red"))
                             ,(propertize "2 Lorem ipsum" 'face '((:background "blue")
                                                                  (:background "green")))
                             "3 Lorem ipsum"
                             ,(propertize "4 Lorem ipsum" 'face '(:inherit holiday))
                             ,(propertize "5 Lorem ipsum" 'face 'holiday)
                             ,(propertize "6 Lorem ipsum" 'face '(holiday isearch))
                             ,(propertize "7 Lorem ipsum" 'face nil)))

With the above display string function, I'd expect to see no background while keeping the initial foreground color. If the above cannot be fixed, do you have any idea where the background stripping better take place instead?

[clemera]

Thank you for working on it! There is one general problem: For completion styles the matching/highlighting is done in one step during refinement so when selectrum--display-string runs it is to late, the highlighting already happened (when using orderless as completion-style) and the default selectrum-refine-candidates-using-completions-styles.

[angrybacon]

Thanks for the helpful details! With that in mind, what would be a better place to try and strip candidates from their background? Some place candidates are available as string so that text properties can be changed, but before any additional properties are forged. For context, I'm using Selectrum together with Orderless.

[clemera]

I think sadly there is no general solution to this, we would need to strip the backgrounds before filtering the candidates which wouldn't be performant and you would loose all the backgrounds not only the one from the selected candidate.

[clemera]

As the org block thing might be the most common scenario for this problem we could explicitly strip that off from the displayed candidates if present (as a workaround).

[clemera]

But maybe those backgrounds should be stripped right away when the lines are collected? consult-line might be the only command which ever passes candidates with backgrounds (cc @minad). Icomplete should have the same problem it just doesn't use a background by default to indicate current candidate (uses icomplete-first-match face).

[clemera]

@minad Personally I would like to disable all text properties for consult-line, I also strip text properties for kill ring items by default using filter-buffer-substring-function.

[minad]

Hmm, I would like to keep the highlighting. But you can also do this using some additional filtering function. I am using opinionated matching styles with the modus theme, which works very well. But I also have the conflict between backgrounds and the selected line highlighting, for example when searching org files/org blocks.

[clemera]

Could there be a user option? There is also the computation cost in big buffers when fontifying everything which I would like to avoid.

[minad]

There is also the computation cost in big buffers when fontifying everything which I would like to avoid.

You can disable the fontification cost by setting a very small fontification limit. But this is certainly not nice since you still get partial fontification. It is probably better to do something like this:

~~~ elisp
(defun no-fontification (orig)
(font-lock-mode -1)
(unwind-protect
(funcall orig)
(font-lock-mode 1)))

(advice-add #'consult--line-candidates :around #'no-fontification)
~~~

[minad]

I wonder about the downsides of disabling fontification locally, it is also not without costs since you have to refontify then. But this cost maybe more acceptable thanks to lazy fontification? I am a bit hesitant to introduce too many configuration options if it can be avoided.

[clemera]

Ah I missed that there is consult-fontify-max-size already. Note there are also some modes which apparently have problems with refontification, see swiper-font-lock-exclude.

[minad]

Yes, I know. But I am checking the jit-lock-mode variable instead of font-lock. This seems to work mostly. If I pick a few examples: I have no problems with org-agenda, nor with treemacs, nor with eww.

[clemera]

Okay, sounds good. I have chosen to (setq consult-fontify-max-size -1) and replace buffer-substring with buffer-substring-no-properties in consult--line-candidates.

[minad]

Okay, sounds good. I have chosen to (setq consult-fontify-max-size -1) and replace buffer-substring with buffer-substring-no-properties in consult--line-candidates.

That's always an option if you are willing to maintain a patched version.

[clemera]

I just hope

(define-advice consult--line-candidates (:around (fun &rest args) no-props)
  (cl-letf (((symbol-function #'buffer-substring)
             #'buffer-substring-no-properties))
    (apply fun args)))

is future proof ;)

[minad]

I don't think this works since buffer-substring is compiled to a bytecode instruction.

[clemera]

Hm, there are no colors anymore so it appears to work.

[minad]

You are probably using an interpreted version of consult--line-candidates.

[clemera]

You are right, thanks it doesn't work when byte compiled :(

[clemera]

@minad Please reconsider adding a filter function variable for consult functions which retrieve buffer contents. The way completion-styles work I see no no way we can fix this issue generally on the Selectrum side (the same should apply to icomplete) without always initially looping through candidates before any filtering happens to strip certain text properties which can't be a solution to this.

[minad]

Hmm, this part is kind of performance critical. There are not many parts of the code which are, but this is. Going through a function call should probably not make a difference. But it would need some testing. And this change should also be applied to all other line based functions I guess?

[minad]

If you want to strip properties for selectrum you can do it by locally overwriting the selectrum highlight function. There is nothing which should prevent you from doing that. It is just not as comfortable.

[clemera]

If you want to strip properties for selectrum you can do it by locally overwriting the selectrum highlight function. There is nothing which should prevent you from doing that. It is just not as comfortable.

The problem is you can't distinguish which parts are from the highlighting (via completion-styles) and which the candidate brought already with it, it has to be done beforehand.

[minad]

Ah okay. I never have problems with this since I am using this expressive opinionated modus highlighting (see https://raw.githubusercontent.com/minad/consult/main/images/consult-line.png). I think it would be fine to add an option such that instead of buffer-substring we are using buffer-substring-no-properties guarded by a conditional. This substring function could be made a defsubst. Maybe one could use the consult-fontify-max-size for the check. If it is <= 0, use buffer-substring-no-properties.

[clemera]

Looks nice, but you still have the issue with org block backgrounds or doesn't this theme highlight them? For my preference your proposal would be ideal, some might only want to strip the backgrounds but when this is too performance sensitive it might make sense to only allow for this faster not retrieving them in the first place.

[minad]

See https://github.com/minad/consult/commit/78eb14c56bf91355019eae2ba1248243071cd59a.

[clemera]

@angrybacon As a workaround you can now unset consult-fontify-preserve

[angrybacon]

Thanks for the update, I'll be using this until I get more time to battle with the background situation. This conversation at least helped me see how the different mechanisms interact with each other so thank you for that :-)

I'll let you close this issue if you think that's good enough, as we can always link to it in a future issue on the same subject.

[clemera]

Okay, you should be able to also set the variable buffer locally for org mode buffers only if you prefer to get the syntax highlighting in the usual case.