oantolin / embark

Emacs Mini-Buffer Actions Rooted in Keymaps
GNU General Public License v3.0
915 stars 56 forks source link

embark-consult: which consult commands should be bound in action and become maps? #122

Closed minad closed 3 years ago

minad commented 3 years ago

Idea by @astoff in https://github.com/minad/consult/pull/171/files#r560111176. The general-map is maybe too general?

minad commented 3 years ago

The same applies to consult-file-externally, which could be added to the file map. Through the embark-consult package we can provide such things nicely by default. Maybe there are even more useful consult commands which could be provided as actions by default?

minad commented 3 years ago

Maybe it is better to keep such things in the config?

diff --git a/README.org b/README.org
index c8334b1..1605918 100644
--- a/README.org
+++ b/README.org
@@ -243 +243,3 @@ reasonable starting configuration:
-    ("C-S-a" . embark-act))              ; pick some comfortable binding
+    (("C-S-a" . embark-act) ;; pick some comfortable binding
+     :map embark-file-map
+     ("x" . consult-file-externally))) ;; if you use consult
oantolin commented 3 years ago

I forgot to add consult-file-externally to the file map! I definitely meant to: that function used to be in Embark, if you recall, and I removed it to avoid duplication with Consult.

I may be confused about what consult-isearch does, I haven't been following it very closely. I thought it was for selecting a previous search string from isearch's history?

oantolin commented 3 years ago

The consult-embark package could also add a bunch of Consult commands to the become keymaps.

minad commented 3 years ago

I may be confused about what consult-isearch does, I haven't been following it very closely. I thought it was for selecting a previous search string from isearch's history?

You can use it during isearch to change the search string or you can also use it to start a completely new search by either typing in a new string or by selecting from the history. This may make it useful for embark, to start a search. But the same applies to consult-line and other utilities. So maybe it is not really a good idea to add all these as actions.

EDIT: My motivation for this issue was mostly that the idea by @astoff does not get lost. And as you said there may be more actions to be added to keymaps.

oantolin commented 3 years ago

I have been using consult-line as an action a lot, but I use M-g l for it, instead of adding it to an Embark keymap. I guess I'll start using consult-isearch the same way.

We definitely shouldn't add too many of these to the general map. I worry about which-key users and don't want to overwhelm them with options, specially since we seem to recommend they use no paging. (I worry less about embark-completing-read-prompter users, since unlike which-key users, they can narrow). But maybe adding one, say consult-isearch, is reasonable.

minad commented 3 years ago

I have been using consult-line as an action a lot, but I use M-g l for it, instead of adding it to an Embark keymap. I guess I'll start using consult-isearch the same way.

Agree, a dedicated binding works and is probably the better approach since it just works everywhere! Which keybinding do you propose for consult-isearch. I have M-s s right now in the example configure, but I am not super fond of it.

We definitely shouldn't add too many of these to the general map. I worry about which-key users and don't want to overwhelm them with options, specially since we seem to recommend they use no paging. (I worry less about embark-completing-read-prompter users, since unlike which-key users, they can narrow). But maybe adding one, say consult-isearch, is reasonable.

Okay, let's keep the maps small. But we still want to add a few of the consult commands to the become keymaps. We should carefully select those.

astoff commented 3 years ago

We definitely shouldn't add too many of these to the general map.

consult-isearch should get a pass here, since isearch is exceptional in not reading from the minibuffer. So, for Embark, binding it to C-s in the general map is the dwim solution!

Personally, I'm not going to bind consult-isearch globally; I'll rather access my search history via C-s C-r, exactly as in Swiper. For the general public, my suggestion would be to bind consult-isearch in isearch-mode-map, shadowing the global-map keybinding of consult-history.

astoff commented 3 years ago

I worry about which-key users

Is there maybe a symbol property to hide stuff from which-key?

minad commented 3 years ago

No symbol property as far as I know. It wouldn't make much sense to impose this on the user. But the user can configure which-key, replace, reformat and hide entries. Therefore cluttered which-key menus are not really a problem if someone cares about tuning this. Usually I remove commands from keymaps instead. For example in the help menu I removed some of the gnu propaganda, warranty and copying information. Probably I have lost the warranty.

oantolin commented 3 years ago

OK, let's not worry too much about which-key users then, they can tune the display if they wish. (But let's not go overboard either. 😛) By the way, is the no-paging really necessary? Paging seemed to work fine in my quick tests.

So lets bind consult-file-externally to x in the file map, and lets bind consult-isearch to C-s, but in which map? I guess the main contenders are the general map and the symbol map. Does anyone have a good use case for consult-isearch as an action which is not on the symbol at point?

What other consult commands should we bind in action maps? And in become maps?

oantolin commented 3 years ago

I guess isearch "feels" general even if we always use it on the symbol at point...

minad commented 3 years ago

Off topic, but I wonder about symbol at point again. Did you resolve the issue with non-elisp symbols such that they do not get the keymap cluttered with describe-symbol etc?

astoff commented 3 years ago

I guess it would be the identifier map if not the general one. But since <embark> C-s just fails miserably if C-s is isearch-forward, I see no drawback in adding it to the general map.

oantolin commented 3 years ago

Off topic, but I wonder about symbol at point again. Did you resolve the issue with non-elisp symbols such that they do not get the keymap cluttered with describe-symbol etc?

I think the issue was adequately resolved. @astoff never signed off on the strategy, but he didn't complain either. Now (thing-at-point 'symbol) is always a valid target. In major modes descending from Emacs Lisp it gets classified as symbol, in other programming major modes it gets classified as identifier, and in non-programming major modes, it gets classified again as symbol, but only if it actually has been interned before.

As a little convenience, in org mode buffers if the target starts and ends with = or starts and ends with ~, those are assumed to be markup and stripped from the name. (In markdown mode this is not necessary because the backtick is not a symbol constituent.)

So above when I said I'd bind C-s for symbols, I also meant to include identifiers, I just forgot.

minad commented 3 years ago

In major modes descending from Emacs Lisp it gets classified as symbol, in other programming major modes it gets classified as identifier, and in non-programming major modes, it gets classified again as symbol, but only if it actually has been interned before.

Ah perfect, the identifier target! This is a good solution. I had just forgotten that discussion. Thank you for reminding me again.

oantolin commented 3 years ago

I think it makes more sense to bind C-s in the general map, even if in the default configuration it is only useful for symbols and identifiers. Users can add new "thing at point" targets, and might want to search for other occurrences of them.

astoff commented 3 years ago

I think the issue was adequately resolved. @astoff never signed off on the strategy, but he didn't complain either.

Quem cala consente (the Spanish version of that must be fabulous).

oantolin commented 3 years ago

Quem cala consente (the Spanish version of that must be fabulous).

I don't know about fabulous, but it is widely used, «El que calla, otorga».

oantolin commented 3 years ago

Huh. It's funny how I automatically switch from "..." to «...» for Spanish. Do you guys do that with German quotes too?

oantolin commented 3 years ago

I'm very happy about this, because I often tried using isearch as an action only to be reminded it doesn't actually work. I really like using query-replace as an action too.

astoff commented 3 years ago

No, we do it American style. But I would call your style French, Germans do it »this way« or „this way“.

oantolin commented 3 years ago

No, we do it American style. But I would call your style French, Germans do it »this way« or „this way“.

No, what I meant is do you find yourself using English quotes for English text and German quotes for German text, even if the German text is being quoted in the middle of an English conversation?

I knew about „this way“, but not about »this way«

astoff commented 3 years ago

Yes, I often do it when I need to throw some German in the middle of an English sentence, but only if I want to give my message a funky vibe. Not that there is anything funky about those quotes to the average German speaker, but they do look very creative to me...

On Sun, Jan 24, 2021, 19:51 Omar Antolín Camarena notifications@github.com wrote:

No, we do it American style. But I would call your style French, Germans do it »this way« or „this way“.

No, what I meant is do you find yourself using English quotes for English text and German quotes for German text, even if the German text is being quoted in the middle of an English conversation?

I knew about „this way“, but not about »this way«

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oantolin/embark/issues/122#issuecomment-766412000, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRTEJR2T7A6SQVLBZPYIQ3S3RT37ANCNFSM4WQOVK6A .

oantolin commented 3 years ago

I'll keep this issue open (and change the title) for further discussion of what consult commands embark-consult should bind.

astoff commented 3 years ago

I just realized there is an isearch-edit-string command which would work just as well as consult-isearch for Embark's purposes.

@minad Since isearch-consult is a nearly drop-in replacement for isearch-edit-string, this suggests a binding for the former in isearch-mode-map, namely M-e.

minad commented 3 years ago

@astoff

Since isearch-consult is a nearly drop-in replacement for isearch-edit-string, this suggests a binding for the former in isearch-mode-map, namely M-e.

What about setting the initial input to the current search string if a search is active such that it really becomes a drop-in replacement?

astoff commented 3 years ago

I just realized there is an isearch-edit-string command with would work just as well as consult-isearch for Embark's purposes.

Maybe not quite: isearch-edit-string gives an error if Isearch has never been used in this Emacs session. After the first Isearch, it seems to always work fine.

minad commented 3 years ago

Maybe note quite: isearch-edit-string gives an error if Isearch has never been used in this Emacs session. After the first Isearch, it seems to always work fine.

Well it does ot matter if isearch-edit-string does not work under all circumstances. If consult-isearch acts as a drop-in in the isearch-mode-map, we are good.

oantolin commented 3 years ago

Correct me if I'm wrong, but consult-isearch always seaches forward and isearch-edit-string searches in the direction of the last isearch.

minad commented 3 years ago

@oantolin See https://github.com/minad/consult/issues/184

astoff commented 3 years ago

Correct me if I'm wrong, but [...] isearch-edit-string searches in the direction of the last isearch.

Probably, but just because isearch leaves it state as global variables. It's horrible, really. If you want Embark's C-s not to depend on Consult, I'd suggest a little function like this

(defun embark-isearch ()
  (interactive)
  (isearch-mode t nil nil nil 'isearch-symbol-regexp)
  ;; or just (isearch-mode t), to use the user's default mode
  (isearch-edit-string))
oantolin commented 3 years ago

Thanks @astoff, I decided to go with that in the embark package, and removed the bindings for consult-isearch in the embark-consult package, so that even people who don't use consult get a usable C-s (not that I expect many people that use embark but not consult, but this reduces a dependency for that functionality).

minad commented 3 years ago

We can still propose consult-isearch as an alternative command for this keybinding. Or would it not be useful since you don't allow editing and directly trigger the action in that case? Furthermore consult-isearch should be improved a bit more (https://github.com/minad/consult/issues/184).

EDIT: I just checked, the consult-isearch does not allow editing. embark-isearch is really the much better solution!

minad commented 3 years ago

The search commands could be bound as actions with the option to allow editing of the search term. I would probably bind them under a prefix map:

Would it make sense to bind C-s (embark-isearch) to the region map to search for the marked region? Same with the consult search menu.

But I am not sure about these proposals, you don't win much over saving, invoking the global keybinding, yanking. And there is discoverability.

oantolin commented 3 years ago

I think discoverability is probably reason enough to include them, especially under a prefix where they don't really clutter up any lists. I actually use consult-line and consult-imenu as actions to search for the symbol at point pretty often. I did put them on the allow edit list as you suggest, but for consult-imenu and consult-outline I also have a setup hook that, when there is a unique match selects it and exits the minibuffer.

As for the region versions, I think I'd rather bring back a way to act on the region contents, even if you just get the general actions offered (but of course, you'd still be able to run any command). I don't know if you remember, but before there was an embark-act-on-region-contents bound to RET in embark-region-map that would rerun embark-act for you but instead of getting offered the region actions, you'd be offered actions appropriate for the region's contents. Back then the classifiers were still separate and I used them to classify the region's contents and pick the appropriate action map. I removed this when I unified target acquisition and classification.

minad commented 3 years ago

I think discoverability is probably reason enough to include them.

Okay, sounds good. Maybe add even the version with the setup hook to embark-consult?

but before there was an embark-act-on-region-contents

No, I didn't remember this. Is it possible to add this embark-act-on-region-contents back in some non-intrusive form? But I consider this low priority, it was just a thought that this could be useful.

oantolin commented 3 years ago

Okay, sounds good. Maybe add even the version with the setup hook to embark-consult?

OK, will do.

Is it possible to add this embark-act-on-region-contents back in some non-intrusive form? But I consider this low priority, it was just a thought that this could be useful.

I think so, I'll try something quick and if it doesn't work, just put this on the todo list.

oantolin commented 3 years ago

I added embark-act-on-region-contents again. dcddbe57fc2de46be955276db3e952450ba47d06

oantolin commented 3 years ago

I think I'll also add that prefix map with the Consult search commands to embark-become-keymaps, so those commands can easily become each other.

I like the prefix c, but it conflicts with a few things you might want for the thing at point:

Maybe the copy-file overlap is fine, I don't think wanting to search for other occurrences of the the file name at point is that common.

The other two maybe I could rebind? Info-goto-emacs-command-node could be I for info (not i, because I don't want to shadow embark-insert).

The variable map currently has:

Maybe the last two could be v and V instead, or maybe even keep the C for customize-variable and just change c to v.

Alternatively, I could pick a different prefix for the Consult search commands. Like M-c.

minad commented 3 years ago

I like u for customize. Marginalia also uses u. Then use C for consult?

oantolin commented 3 years ago

I think the setup hook that when the match is unique just goes there without further intervention from the user should only be used for consult-outline, consult-imenu and consult-project-imenu. For the other commands if there is only one match for the thing at point, you are mostly likely on it!, and you will probably want to tweak the minibuffer input instead of the action sliently jumping to the current location, making it look like nothing happened!

oantolin commented 3 years ago

I like u for customize. Marginalia also uses u. Then use C for consult?

Sure that seems fine. But I think I might change Info-goto-emacs-command-node to I anyway, since c is kind of a weird binding for that command.

minad commented 3 years ago

Sounds good. I would only bind consult-project-imenu and not consult-imenu. I am also proposing that in the consult example configuration. If the user prefers consult-imenu, it is easy to replace in the keymap.

oantolin commented 3 years ago

What does consult-project-imenu do if consult-project-root-function is nil? I found it confusing just now.

oantolin commented 3 years ago

Oh, if consult-project-root-function is nil it just gives you all buffer in the same major mode as the current one?

minad commented 3 years ago

I think it uses every buffer with the same mode. Maybe I should change it such that it falls back to consult-imenu and only shows the current buffer in that case. What do you think?

oantolin commented 3 years ago

This sounds like one of those personality tests: if consult-project-root-function is nil, an optimist says "all buffers in the current major mode are a single project!" and a pessimist says "the only buffer you can be sure is in the current project is the current buffer". 😆

minad commented 3 years ago

There is a third option - if no project root function is set just just the menus from all buffers since in that case everything is lost or show nothing at all :stuck_out_tongue_closed_eyes:

oantolin commented 3 years ago

I think I'm a pessimist, maybe it should fallback to consult-imenu if consult-project-root-function is nil. But also maybe consult-project-root-function should default to vc-root-dir or something in terms of project-root?