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

Improve history browsing with file completions #368

Conversation

clemera
Copy link
Collaborator

@clemera clemera commented Jan 14, 2021

Ensure the changes only affect the intended purpose and improve general behaviour:

  • When navigating history the candidates should be shown again when returning to the start
  • The history position should only reset when it was defined before
  • There should be no refresh for selectrum-insert-current-candidate in general only when browsing through history

@clemera clemera force-pushed the improve-selectrum-insert-current-candidate branch 5 times, most recently from f4f074f to 296bade Compare January 14, 2021 10:33
@clemera clemera force-pushed the improve-selectrum-insert-current-candidate branch from 296bade to 7b1a099 Compare January 14, 2021 10:45
@clemera clemera changed the title Improve selectrum-insert-current-candidate Improve history browsing with file completions Jan 14, 2021
@clemera clemera merged commit 6c8a009 into radian-software:master Jan 14, 2021
@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

@minad Do you think the current behaviour for file history browsing is sufficient? Or should there be a help message that you have to press TAB to refresh for the current history item?

@minad
Copy link
Contributor

minad commented Jan 14, 2021

Do you think the current behaviour for file history browsing is sufficient? Or should there be a help message that you have to press TAB to refresh for the current history item?

I am not sure I understand exactly what you mean but I never had problems with file browsing 😆

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

I mean when you press M-p the candidate list does not refresh, you have to press TAB to do that.

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

This avoid issues with tramp and unintended hitting of slow file systems when going through the items using M-p.

@minad
Copy link
Contributor

minad commented Jan 14, 2021

I mean when you press M-p the candidate list does not refresh, you have to press TAB to do that.

Yes, I've seen that. Why is that? It is not a big problem but I always felt that this is some kind of bug, but not a problematic one.

@minad
Copy link
Contributor

minad commented Jan 14, 2021

Okay, I see - because of TRAMP. I would just keep it as is then. No help message please. Usually I am annoyed by too many messages. I even have the following somewhere in my config:

;; completely suppress some standard messages
(defconst config--suppressed-messages
  '("Mark set" "Mark activated" "Mark deactivated"
    "Mark saved where search started"
    "Back to top level"
    "Beginning of buffer" "End of buffer"))
(defconst config--suppressed-errors
  '(beginning-of-buffer end-of-buffer))
(advice-add #'message :around
            (lambda (fun fmt &rest args)
              (if (and (null args) (member fmt config--suppressed-messages))
                  fmt
                (apply fun fmt args))))
(advice-add #'command-error-default-function :around
              (lambda (fun data &rest args)
                (unless (memq (car data) config--suppressed-errors)
                  (apply fun data args))))

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

I see 😆, okay no message then but maybe indicating it in the prompt like for describe-face (we do that for completing-read-multiple in general)?

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

But I would also prefer to avoid introducing ugly handling code because of that. Maybe i's obvious enough that you have to use TAB to refresh.

@minad
Copy link
Contributor

minad commented Jan 14, 2021

Hmm, I would also not add it to the prompt in particular if it needs special casing. It is really not important. In contrast, in the case of completing-read-multiple I find the message helpful and good, since otherwise it is not obvious that you have a completing-read-multiple. But maybe the message could be slightly more concise?

add more using TAB
TAB to add more
TAB for more
+TAB

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

That is a good idea, I think I will change it to TAB to add more, that reads better, too I think

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

Oh, no that is not possible, because sometimes you get a more elaborate message like add more using TAB and crm-separator

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

Or the word order would just be different then I guess that would be ok.

@minad
Copy link
Contributor

minad commented Jan 14, 2021

"..and crm-separator", that's an overly verbose message. I did't even understand it before looking at the source. And it is not helpful since it does not show you what the crm separator is. I would probably just show the keybinding since I guess this is what people are going to use. And it is also clear that something special is going on since a keybinding is shown. But it might not be super beginner friendly.

@minad
Copy link
Contributor

minad commented Jan 14, 2021

You could also change it to , or TAB for more and the character to show for the crm would have to be configured somehow in this crm alist.

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

If the regexp used is known we already do show the character instead of "crm-separator", that is only used if someone uses a not so common regexp not found in our alist.

@minad
Copy link
Contributor

minad commented Jan 14, 2021

Ah, I see. I didn't look that closely ;)

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

I think generally I prefer that history commands don't automatically trigger updates, it find it distracting when using M-p. I already worked on making this the general behaviour but the history behaviour for async commands would need to stay the old way, at least I couldn't see how it make it work. I'm still undecided, UI wise I would prefer it but not sure if its worth it.

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

An alternative could be to also update for file history by default but check for tramp paths and when there is one don't update and show a message. Either this or what I have written above.

@clemera
Copy link
Collaborator Author

clemera commented Jan 14, 2021

I went with the latter, see #370. Now only tramp paths are not refreshed and a message is shown in that case, I hope the message won't annoy you to much it only appears when using M-p and there is a tramp path.

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

Successfully merging this pull request may close these issues.

2 participants