Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bleeding prompt background #235

Closed
clemera opened this issue Nov 26, 2020 · 46 comments
Closed

Bleeding prompt background #235

clemera opened this issue Nov 26, 2020 · 46 comments

Comments

@clemera
Copy link
Collaborator

clemera commented Nov 26, 2020

EDIT: This was the display property issue which moved to #411, for the bleeding prompt see discussion starting here

It is possible to display candidates as different strings using the display property. Currently there is no indication of the current selection in this case.

(completing-read "Candidates: "
                 `(,(propertize "GNU" 'display "some-prefix: GNU")
                   ,(propertize "Emacs" 'display "other-prefix: Emacs")))
@minad
Copy link
Contributor

minad commented Nov 26, 2020

I mentioned this before in minad/consult#1. Is this proper api use or some kind of undocumented bending of the api which somehow works by change?

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

The ability to display strings as something different is a standard Emacs feature so I think it would be good to support that. Helm and ivy also display the selection correctly in this case.

@minad
Copy link
Contributor

minad commented Nov 26, 2020

Good, then I think it is ok to use that. The highlighting works in helm and ivy? Is it broken only in selectrum?

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

Yes the highlighting of the selected candidate works correctly with the others. What doesn't work is the highlighting of the matched parts as the matching happens against the strings and not how they are displayed. Because of this the usefulness of this is limited but still something I think we should support (looking into why the other frameworks already display the selection correctly in this case might also reveal something else we should do differently with regard to highlighting).

@minad
Copy link
Contributor

minad commented Nov 26, 2020

It seems to work if one prepends the candidate with a single space and display-propertizes that. I am trying to use that for consult-line. However if starting the selection, the line numbers are bold and after pressing some keys they become normal weight?? Probably has to do with selectrum overwriting some weight property somewhere.

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

And afterwards you simply strip the space again, nice hack :)

@minad
Copy link
Contributor

minad commented Nov 26, 2020

Not necessary, the space is simply not displayed. And the selected candidate goes through the alist after selection.

@minad
Copy link
Contributor

minad commented Nov 26, 2020

Now I only have to manage to make consult--buffer selectrum independent...

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

That is nice! I don't know if consult--buffer can work without selectrum. There would have to be a way to change what is considered as the current input string.

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

It is possible to trick minibuffer-contents and minibuffer-prompt-end into believing the prefix chars belong to the prompt by putting the field property on them. Here is a POC which works with icomplete:

(icomplete-mode 1)

(define-key minibuffer-local-completion-map (kbd "SPC") 'self-insert-command)

(let ((coll nil)
      (colla '("this" "is" "coll" "a"))
      (collb '("and" "now" "coll" "b")))
  (defun test (str pred action)
    (cond ((string= (minibuffer-contents) "a ")
	   (goto-char (minibuffer-prompt-end))
	   (delete-char 2)
	   (insert (propertize "a " 'field t 'rear-nonsticky t))
	   (run-at-time 0 nil (lambda () (goto-char (point-max))))
	   (setq coll colla))
	  ((string= (minibuffer-contents) "b ")
	   (goto-char (minibuffer-prompt-end))
	   (delete-char 2)
	   (insert (propertize "b " 'field t 'rear-nonsticky t))
	   (run-at-time 0 nil (lambda () (goto-char (point-max))))
	   (setq coll collb)))
        (if (eq action 'metadata)
            `(metadata
              (cycle-sort-function . identity)
              (display-sort-function . identity))
	  (complete-with-action action coll str pred))))

(completing-read "Check: " 'test)

Not nice but may be a start. If you input "a " you get completion for collection colla and if you delete the input and input "b " you get the candidates of collb.

@minad
Copy link
Contributor

minad commented Nov 26, 2020

@clemera I am not sure about this issue. As I described, if we always do the prefix like this (concat (propertize " " 'display "prefix string") "candidate")) highlighting continues to work and we don't need an extra special annotation property etc etc. And as you proposed we can always strip this whitespace in the end since every candidate has it. I think it is not even wrong if highlighting does not work if you overwrite the complete candidate with 'display. Then the highlighting happens on the string "living below" the 'display. So to me it is not fully clear - are we looking at a bug here?

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

Good point, but from a user point of view it is weird that you can't select the candidates as usual and ivy/helm behave better IMO. Though as this is probably a rare case anyway it may not be worth worrying about to much.

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

Now I only have to manage to make consult--buffer selectrum independent...

Another way to fake the current minibuffer content would be to rebind mininbuffer-contents and minibuffer-prompt-end around the completing-read call using cl-letf, which is probably the better approach.

@minad
Copy link
Contributor

minad commented Nov 26, 2020

I would rather like to keep the hacks to a minimum. I think it is ok to fallback on the selectrum-api if there is no other possibility. And we can always improve things as soon as the upstream api improves. I mean my goal is to have something which works (and if its nicely written thats a big bonus). I don't think trying to fit stuff onto some api at all costs makes sense.

@clemera
Copy link
Collaborator Author

clemera commented Nov 26, 2020

I'm just sketching possible approaches which would make this work using regular completing-read since you asked if it is possible. Now we know it works that doesn't mean you have to use it for consult of course. The last approach isn't to bad I think, I tried it quickly and it even worked with helm. I wouldn't use it as a general solution or for a completion framework but for single commands it wouldn't be too bad.

@minad
Copy link
Contributor

minad commented Nov 27, 2020

@clemera Sure, please do! Thank you a lot!

...which would make this work using regular completing-read since you asked if it is possible.

For me this question has been resolved negatively, I also meant it in a more restricted sense - if we can make this work while adhering to the standard api without using any special edge case behavior or tricks which make assumptions about the system lying below the api. For example reading the minibuffer-contents is using a side-effect which goes beyond what the api offers.

If I look at the consult--buffer code (or the original code from the wiki), it is very straightforward. The completion function is just a pure function which does not use any effects. Therefore I had expected something like this to work:

(progn
(let ((coll nil)
      (colla '("this" "is" "coll" "a"))
      (collb '("and" "now" "coll" "b")))
  (defun test (str pred action)
    (cond ((string-prefix-p "a " str)
	   (setq coll colla
                 str (substring str 2)))
	  ((string-prefix-p "b " str)
	   (setq coll collb
                 str (substring str 2)))
          (t (setq coll (append colla collb))))
        (if (eq action 'metadata)
            `(metadata
              (cycle-sort-function . identity)
              (display-sort-function . identity))
	  (complete-with-action action coll str pred))))
(completing-read "Check: " 'test)
)

@clemera
Copy link
Collaborator Author

clemera commented Nov 27, 2020

This would ideally work but it this is Emacs - unexpected side effects everywhere ;) Maybe we should propose this to the mailing list.

@clemera
Copy link
Collaborator Author

clemera commented Nov 27, 2020

I experimented a bit more and looked a bit more into the API and the following seems to work without advice almost as you would expect for default completion (also works with icomplete):

(let ((colla '("this" "is" "the" "colla"))
      (collb '("now" "showing" "collb")))
  (defun test (str pred action)
    (if (eq action 'metadata)
        `(metadata
          (cycle-sort-function . identity)
          (display-sort-function . identity))
      (let ((coll (cond ((string-prefix-p "a " str)
			 colla)
			((string-prefix-p "b " str)
			 collb)
			(t
			 (append colla collb)))))
	(cond ((null action)
	       (cond ((or (string-prefix-p "a " str)
			  (string-prefix-p "b " str))
		      (concat (substring str 0 2)
			      (try-completion (substring str 2) coll pred)))
		     (t (try-completion str coll pred))))
	      ((eq action t)
	       (all-completions
		(cond ((or (string-prefix-p "a " str)
			   (string-prefix-p "b " str))
		       (substring str 2))
		      (t str))
		coll pred))
	      (t nil))))))

You need to strip the prefix from the result afterwards if it was used.

@minad
Copy link
Contributor

minad commented Nov 27, 2020

I tried it with selectrum and with icomplete and it doesn't seem to work for me. If I press "a SPC" it always completes to "colla".

@clemera
Copy link
Collaborator Author

clemera commented Nov 27, 2020

This sounds like you haven't rebound SPC?:

(define-key minibuffer-local-completion-map (kbd "SPC") 'self-insert-command)

It doesn't work with selectrum, we first need to fix #114.

@clemera
Copy link
Collaborator Author

clemera commented Nov 27, 2020

I assume you probably also use orderless? I just discovered that it doesn't work with it set as completion style but works fine with the default completion styles, I currently don't know the reason for this, the minibuffer completion code confuses me every time I look at it.

@minad
Copy link
Contributor

minad commented Nov 27, 2020

Yes, yes, yes.

This sounds like you haven't rebound SPC?: (define-key minibuffer-local-completion-map (kbd "SPC") 'self-insert-command)

I have not. Forgot it. It is easier if we exchange complete snippets.

It doesn't work with selectrum, we first need to fix #114.

I expected that, tried nevertheless.

I assume you probably also use orderless

I started using selectrum with orderless and haven't found a reason to switch to prescient yet.

@clemera
Copy link
Collaborator Author

clemera commented Nov 27, 2020

I also use orderless for filtering and prescient for sorting. Just noticed I got the same behaviour you described with orderless outside my testing sandbox.

@minad
Copy link
Contributor

minad commented Nov 27, 2020

Ok, sorting is something I haven't really understood yet. Which sorting is used by selectrum if I only use orderless?

@minad
Copy link
Contributor

minad commented Nov 27, 2020

The default sorting and filtering in Selectrum is quite primitive. First candidates are sorted alphabetically, and then they are filtered down to those that contain your input as a substring. The part of each candidate that matches your input is highlighted. This default behavior is intended as a lowest common denominator that will definitely work.

Ah this in the readme. Sounds reasonable. I am not sure if I need prescient yet.
Why do you combine prescient with orderless? Prescient claims to have the same or similar filtering as orderless.

@clemera
Copy link
Collaborator Author

clemera commented Nov 27, 2020

I use orderless for filtering as it also provides a completion-style which means you get the same filtering in default completion. Orderless is also faster for filtering as it uses C functions for filtering. Sorting is a separate task which allows to show recently/often used candidates first which orderless doesn't handle but prescient does, that's why I use both.

@minad
Copy link
Contributor

minad commented Nov 27, 2020

Ok I see, maybe it would be a good idea to narrow the scope of prescient then or merge them, but I've just seen that you proposed that before :) Maybe it is also good to have some alternatives.

@minad
Copy link
Contributor

minad commented Dec 4, 2020

@clemera I tried again the example you wrote above with SPC=self-insert-command and icomplete but it does not seem to work for me. When I press "a " I don't see anything, it shows "no matches". Is there anything else which could be wrong.

EDIT: Sorry, I forgot. If I run it in emacs -Q without orderless, I can confirm it works. I will ask on the orderless issue tracker too regarding this.

I would like to address the dynamic candidate issue at some point but it seems I cannot even get this to work with icomplete ;)
And then selectrum has #114. It would be great to get this working at some point since that would open up quite a few possibilities.

(let ((colla '("this" "is" "the" "colla"))
      (collb '("now" "showing" "collb")))
  (defun test (str pred action)
    (if (eq action 'metadata)
        `(metadata
          (cycle-sort-function . identity)
          (display-sort-function . identity))
      (let ((coll (cond ((string-prefix-p "a " str)
			 colla)
			((string-prefix-p "b " str)
			 collb)
			(t
			 (append colla collb)))))
	(cond ((null action)
	       (cond ((or (string-prefix-p "a " str)
			  (string-prefix-p "b " str))
		      (concat (substring str 0 2)
			      (try-completion (substring str 2) coll pred)))
		     (t (try-completion str coll pred))))
	      ((eq action t)
	       (all-completions
		(cond ((or (string-prefix-p "a " str)
			   (string-prefix-p "b " str))
		       (substring str 2))
		      (t str))
		coll pred))
	      (t nil))))))

(define-key minibuffer-local-completion-map (kbd "SPC") 'self-insert-command)

(completing-read "Check: " 'test)

@clemera
Copy link
Collaborator Author

clemera commented Dec 4, 2020

You have to type "a" and then a space, then it should work.

@minad
Copy link
Contributor

minad commented Dec 4, 2020

@clemera Sure, I wrote "a " above, meaning "a SPC". The problem I had now (and I forgot about that) might to be orderless. At least it works in "emacs -Q" after enabling icomplete-mode. Maybe @oantolin has an idea. I asked him on the orderless tracker. If it is possible to sort this out such that it works robustly with icomplete, it would be interesting to tackle the selectrum issue #114.

@clemera
Copy link
Collaborator Author

clemera commented Dec 4, 2020

Ah, okay no problem, I wrongly assumed you missed it.

@clemera
Copy link
Collaborator Author

clemera commented Dec 4, 2020

This is kind of the last big feature which one needs to use selectrum-read for, would be really cool if we can get this to work.

@minad
Copy link
Contributor

minad commented Dec 4, 2020

This is kind of the last feature which makes calling selectrum-read necessary, would be really cool if we can get this to work.

Yes, exactly! While I was skeptical in the beginning, I am pretty much convinced now that the standard apis are the way to go. I am testing with icomplete-vertical-mode from time to time and everything seems to work pretty well there too. Selectrum works better for me (faster etc, behaves somehow more natural to me). But for example this speed issue regarding icomplete - I was always wondering about that. Icomplete just feels somehow slow, is this due to delays? Even if I use a compution delay of 0. And I think I did also some profiling once and there icomplete performed pretty badly, but maybe I don't remember this correctly. I think I will try to profile this again now because I am wondering.

@minad
Copy link
Contributor

minad commented Dec 4, 2020

Haha, interestingly the new marginalia-annotate-command-binding is appearing quite prominently now when profiling M-x in both selectrum/icomplete. This should not be, I think. But I guess I will investigate this profiling issue a bit more in detail.

@clemera
Copy link
Collaborator Author

clemera commented Dec 4, 2020

I think icomplete recomputes the table when you input something? The reason selectrum does not do this is the main reason for #114. I think we can handle this in selectrum by only doing recomputation for dynamic tables which are not know to be "static" on input like for example help--symbol-completion-table.

@minad
Copy link
Contributor

minad commented Dec 4, 2020

Hmm, yes that is probably the issue for the slowness. But if something similar as icomplete is done for selectrum, the performance will also degrade? How can this be avoided? By using the whitelists? I am not a big fan of such an approach.

@clemera
Copy link
Collaborator Author

clemera commented Dec 4, 2020

I think usually dynamic tables aren't too expensive to compute but there are a few built-in exceptions which we could exclude (see my edit in above comment).

@minad
Copy link
Contributor

minad commented Dec 4, 2020

Ok, I agree (e.g. for the virtual buffer example it costs nothing), but then the question remains why icomplete is not as snappy as selectrum. Edit: Probably it is still the recomputation thing. If I try consult-buffer it is also fast on icomplete, but M-x is slow - so the recomputation cost can be significant?

@clemera
Copy link
Collaborator Author

clemera commented Dec 4, 2020

Yes, but I think this is mostly for the built in symbol tables so I hope by excluding those from recomputation it will work fine for most use cases.

@sheijk
Copy link

sheijk commented Jan 11, 2021

I see the same issue when using embark (which uses 'display property in embark-completing-read-prompter). Additionally with a minibuffer-prompt with a purple background I see the prompt background bleeding into selectrum's candidates if the minibuffer input is empty:
2021-01-11-232922_413x187_scrot
And after typing one or more chars:
2021-01-11-232942_414x116_scrot
Which doesn't bleed the background but does not show the highlight for current candidate as described in the original post here.

Ivy handles this. Helm does too and also shows the candidates using 'display in bold so maybe their fix can be adapted.

@minad
Copy link
Contributor

minad commented Jan 11, 2021

@sheijk I am observing this too. It also affects consult-line line numbers for example. But I think ist also happens with icomplete vertical.

@clemera
Copy link
Collaborator Author

clemera commented Jan 12, 2021

It seems using font-lock-append-text-property would fix the original issue. The bleeding of the prompt is something I also noticed when trying to prepending strings to the prompt, I don't know a fix for that. @minad What issue do you see with consult-line? I think that the line numbers aren't included in the current candidate highlight could make sense? At least it is also how display-line-number-mode behaves with hl-line in buffers.

@minad
Copy link
Contributor

minad commented Jan 12, 2021

Yes they are not included in the hl. But there is also some bleeding of the prompt face into the line number face as long as nothing has been entered. Exactly the same issue which @sheijk observed.

@clemera
Copy link
Collaborator Author

clemera commented Jan 12, 2021

But there is also some bleeding of the prompt face into the line number face as long as nothing has been entered.

It seems I don't get this with consult-line:

consult-line

@minad
Copy link
Contributor

minad commented Jan 12, 2021

Not all properties. The background seems to be overwritten. But for example if the prompt is bold the boldness bleeds into the line number. This is what I am seeing. But it depends on the theme. If the theme sets the weight of the line numbers it won't bleed.

@clemera clemera changed the title Highlighting of selected candidate when using the display property Bleeding prompt background Jan 28, 2021
@clemera
Copy link
Collaborator Author

clemera commented Jan 29, 2021

Fixed by #413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants