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

Using this-command for selectrum-repeat is unreliable #98

Open
clemera opened this issue May 25, 2020 · 11 comments
Open

Using this-command for selectrum-repeat is unreliable #98

clemera opened this issue May 25, 2020 · 11 comments

Comments

@clemera
Copy link
Collaborator

clemera commented May 25, 2020

For example after read-string, this-command is exit-minibuffer. You can call the following command to test this:

(defun example-command ()
  (interactive)
  (read-string "Example: ")
  (message "%s" this-command))

That means any completing-read called after read-string will save exit-minibuffer as selectrum--last-command but it should have saved example-command. This breaks selectrum-repeat and also other commands which want to make use of selectrum--last-command.

@clemera clemera changed the title selectrum--last-command is unreliable this-command for selectrum--last-command is unreliable May 25, 2020
@clemera
Copy link
Collaborator Author

clemera commented May 25, 2020

Maybe we can just save every command in pre-command-hook as long as there isn't an active minibuffer. Then when selecturm-read gets called use that saved command as selectrum--last-command.

@raxod502
Copy link
Member

raxod502 commented Jun 3, 2020

Yes, that sounds reasonable. Thank you for the report.

@clemera clemera changed the title this-command for selectrum--last-command is unreliable Using this-command for selectrum-repeat in unreliable Aug 8, 2020
@clemera clemera changed the title Using this-command for selectrum-repeat in unreliable Using this-command for selectrum-repeat is unreliable Aug 11, 2020
@clemera
Copy link
Collaborator Author

clemera commented Dec 15, 2020

In Emacs 28 there will be minibuffer-this-command which we can use, this will also useful for command based configuration.

@minad
Copy link
Contributor

minad commented Dec 15, 2020

Another this command 💩

@clemera
Copy link
Collaborator Author

clemera commented Dec 15, 2020

Yes, but it makes sense because this is a value you just had no access to before.

@minad
Copy link
Contributor

minad commented Dec 15, 2020

I guess it makes some sense. But still - the design around this-command feels fundamentally flawed.

@clemera
Copy link
Collaborator Author

clemera commented Dec 15, 2020

Yes, all those this/that/last commands are hard to get right...

@clemera
Copy link
Collaborator Author

clemera commented Dec 15, 2020

Oh, I got the name wrong above, too 😆 It will be called current-minibuffer-command

@minad
Copy link
Contributor

minad commented Dec 15, 2020

🙈

@clemera
Copy link
Collaborator Author

clemera commented Dec 15, 2020

I think that name is actually a good choice, less confusing and pretty intuitive.

@raxod502
Copy link
Member

Okay, that's actually pretty helpful. I seem to recall spending several hours trying to work around the problem you pointed out with your read-string example above, and I was unable to figure out anything that would be even remotely robust. Perhaps with this new variable we can solve the issue.

I've found the previous issue, which has more details and examples: #219

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