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

Introduce embark--minibuffer-point #200

Merged
merged 2 commits into from
Apr 9, 2021
Merged

Conversation

minad
Copy link
Contributor

@minad minad commented Apr 9, 2021

(- (point) (minibuffer-prompt-end)) can be negative

I had problems with this in Marginalia (minad/marginalia@2d19497) and Vertico (minad/vertico@c72e8de).


Then I have a question about Embark metadata access

(defun embark--metadata ()
  "Return current minibuffer completion metadata."
  (completion-metadata
   (buffer-substring-no-properties (field-beginning) (point))
   minibuffer-completion-table
   minibuffer-completion-predicate))

Icomplete and Icomplete-vertical uses this:

(completion--field-metadata (icomplete--field-beg))

which is equivalent to

  (completion-metadata
   (buffer-substring-no-properties (minibuffer-prompt-end) (point))
   minibuffer-completion-table
   minibuffer-completion-predicate))

I wonder about (point) < (minibuffer-prompt-end).

In Vertico I am now using this, which is probably wrong.

  (let* ((content (minibuffer-contents-no-properties))
         (metadata (completion-metadata content
                                        minibuffer-completion-table
                                        minibuffer-completion-predicate))

See also oantolin/icomplete-vertical#27, where I have been confused for the first time.

(- (point) (minibuffer-prompt-end)) can be negative
@minad
Copy link
Contributor Author

minad commented Apr 9, 2021

I suspect all of the above methods to obtain the metadata are actually wrong. The correct way to obtain the metadata may be this:

(completion-metadata (buffer-substring-no-properties (minibuffer-prompt-end)
                                                     (max (minibuffer-prompt-end) (point)))
                     minibuffer-completion-table
                     minibuffer-completion-predicate)

minad added a commit to minad/vertico that referenced this pull request Apr 9, 2021
@minad
Copy link
Contributor Author

minad commented Apr 9, 2021

Okay, I actually solved this in a simpler way now in Vertico. I simply do not update the UI when the point is outside the legal region, see minad/vertico@b0f5699. (EDIT: But this will then break the vertical movement commands. Maybe keeping the point checks is better after all.)

However I think I keep the fix in Marginalia (minad/marginalia@2d19497) for robustness.

And in Embark the problem is still there if you move the point to the prompt and run embark-act. Then you get out of range errors.

minad added a commit to minad/vertico that referenced this pull request Apr 9, 2021
@oantolin
Copy link
Owner

oantolin commented Apr 9, 2021

People should not be moving the point into the prompt! What's wrong with these people? 😆

I'm not completely convinced this really needs to be fixed, but I agree that the best fix is probably what you did.

@minad
Copy link
Contributor Author

minad commented Apr 9, 2021

Yes, it is very wrong :-P

  1. I would have made the prompt an overlay.
  2. I have this cursor-sensor-mode activated in order to prevent me from doing this

But there is a but. If you move the point right after the prompt I still had an issue in Vertico where I got -1, maybe because the sensor mode got triggered to late or because of the overlays. I have no idea. Then I thought it is better to fix this such that it works even for the people who like to move into the prompt :D

@oantolin oantolin merged commit 5f30978 into oantolin:master Apr 9, 2021
@oantolin
Copy link
Owner

oantolin commented Apr 9, 2021

Merged! I was tempted instead to document that calling embark commands from within the minibuffer prompt is undefined behavior, and to add code to embark so that it uninstalls itself if you do that.

@minad
Copy link
Contributor Author

minad commented Apr 9, 2021

In any case it is good to merge this. Now that I made this embark--minibuffer-point function you have a central place where you can define the undefined and insert (package-delete "embark").

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

Successfully merging this pull request may close these issues.

None yet

2 participants