Git Product home page Git Product logo

Comments (147)

oantolin avatar oantolin commented on September 18, 2024 1

Ups, sorry I misunderstood, you can forget the code snippet it's useless ;)

OK.

And about exiting or not from embark-occur: I think we can leave it as is, always going to top-level, and let people make a feature request if they ever want something else.

I will add the (ignore (embark-target)) so people can bind it as an action.

from embark.

oantolin avatar oantolin commented on September 18, 2024 1

In other news, the direct binding of action commands is now (428b238, edit: d6f9f76) a minor mode (which it should have been all along, I think). It can be used in an embark-occur buffer, but also in any completions buffer.

I think I should also make embark occur a major mode.

from embark.

clemera avatar clemera commented on September 18, 2024 1

Did you patch your selectrum to bind minibuffer-completion-table?

Yup, I did, there is also a pending PR for merging this patch upstream.

from embark.

oantolin avatar oantolin commented on September 18, 2024 1

Thanks for the feedback! I fixed the name of the dired buffers and now apply directory-file-name to all the candidates (the docstring doesn't say what it does with non-directories, but it seems to keep them the same.)

I took care of the "Back to top level" message and so we wouldn't forget to do that again, I introduced an embark-after-exit macro that hides all our uses of run-at-time, top-level and inhibit-message.

Having C-u mean "act on all candidates" is a cool idea, I'll think about it.

from embark.

oantolin avatar oantolin commented on September 18, 2024 1

I think changing your

(let ((completing-read-function
       (lambda (&rest _)
         (embark-target))))
  ...)

to

(let* ((old-crf completing-read-function)
       (completing-read-function
        (lambda (&rest args)
          (if-let ((target (embark-target)))
              target
            (apply old-crf args)))))
  ...)

is safe in the editing-not-allowed case, even for commands with multiple arguments. Maybe this can even stay in embark so we don't have to count on completion UIs being highly tuned.

from embark.

oantolin avatar oantolin commented on September 18, 2024 1

Hi everyone, @clemera, @karthink, @protesilaos, @a13! The live-update branch (5f94b92) has a version of embark-occur that updates live while you type. It still needs a little work, but it definitely gives the idea. (I guess this isn't very attractive for Selectrum users, but for default tab completion users --or even Icomplete users, because of the annotations-- it can be pretty useful.)

In that branch the embark-exit-and-occur behaves like the embark-occur of master.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Excellent idea, I will definitely do this. Already simply popping up the completions buffer and renaming it to *embark-occur* is like 70% of the way there (this will be the backup for when your embark-occur-alist doesn't have a specific entry for the completion type).

from embark.

clemera avatar clemera commented on September 18, 2024

this will be the backup for when your embark-occur-alist doesn't have a specific entry for the completion type

I had this thought, too. The completion list buffer does not work without an active minibuffer but it shouldn't be too hard to adjust this.

from embark.

oantolin avatar oantolin commented on September 18, 2024

The completion list buffer does not work without an active minibuffer but it shouldn't be too hard to adjust this.

Pressing RET doesn't work, but embark actions do now! Maybe that's enough.

from embark.

clemera avatar clemera commented on September 18, 2024

Pressing RET doesn't work, but embark actions do now!

That is great! Yes maybe RET is not needed

from embark.

karthink avatar karthink commented on September 18, 2024

Perhaps an embark-default-action can be added that calls the command that opened the minibuffer in the first place? Then calling it will be equivalent to pressing RET. Perhaps it can even be bound to RET in the completions buffer.

from embark.

oantolin avatar oantolin commented on September 18, 2024

That suggestion works fine if you are completing the first argument of the command that opened the minibuffer, but not if you are completing the second or higher argument. But that's probably good enough.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Here's a first version of embark-occur: 2711169

You can call it from either the minibuffer or the completions buffer. What you get is just a *Completions* buffer with a few modifications:

  • It's renamed to *Embark Occur*. :)
  • It has RET bound as @karthink suggested: to the command that opened the minibuffer from which you ran embark-occur in the first place.
  • It has all the keys in the specific embark keymap bound directly to the actions they execute, that is, you don't need to run embark-act to use them. So, for example, if you were doing file completion, d is bound to deleting the file at point, ! to running a shell command on it, etc.

The bindings in completion-list-mode-map (the map for completions buffers) are still active in the embark occur buffer, so you can use the arrow keys to move between completions. Also, if you have a binding for embark-act in completions buffers, you can also use that (to use the full range of your normal keybindings as actions).

from embark.

oantolin avatar oantolin commented on September 18, 2024

I'm starting to think that maybe that's exactly what embark-occur should be, simple and unassuming, just a way to not have to press the keybinding for embark-act before each action. :)

And that @clemera's suggestion of opening a dired buffer for file completion, an ibuffer for buffer completion, etc., is also useful (probably even more useful than this), but is something different and should be a different command. In a dired buffer you won't have your file actions available (of course dired has better versions of all of them, so they aren't necessary), so dired isn't really analogous to ivy-occur.

If people agree with this idea of a separate command, how about the name embark-export for that?

from embark.

karthink avatar karthink commented on September 18, 2024
  • It has all the keys in the specific embark keymap bound directly to the actions they execute, that is, you don't need to run embark-act to use them. So, for example, if you were doing file completion, d is bound to deleting the file at point, ! to running a shell command on it, etc.

Cannot test at the moment, but I'm not sure this is a good idea. In non-editable buffers Emacs and other packages usually bind n,p etc as navigation commands (dired, xref, and many more). Evil users will want to use j,k etc.

With the embark keymap active, many of the navigation shortcuts will be taken and this is not possible any more.

EDIT: Thinking about it some more, dired manages to have navigation and action without the keybinds stepping over each other, so maybe this is just a matter of finding sane defaults.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Currently our only j/k/n/p binding is k for kill-buffer. I guess we could move kill-buffer to d.

To be honest I was moving between completions with the arrow keys, so I wasn't worried about collisions with embark actions.

But if collisions are a worry, an alternative plan for embark-occur would be to leave only RET and tell people to bind embark-act to a very comfortable key in completion-list-mode-map. I'm using ; and it feels pretty decent, a would be another good choice.

from embark.

karthink avatar karthink commented on September 18, 2024

Yeah my point was not about j/k/n/p specifically, but collisions between keymaps in general.

The user can bind p to project-find-file in the embark actions keymap, for instance, then be annoyed when p goes to the previous line in an embark-occur buffer.

bind embark-act to a very comfortable key in completion-list-mode-map.

This is ivy's solution too. I believe the dispatch key in an ivy-occur buffer is just o as opposed to M-o from the minibuffer. This keeps things mnemonic while making it easier to type.

from embark.

karthink avatar karthink commented on September 18, 2024

embark-occur should probably use switch-to-buffer-other-window or pop-to-buffer instead of switch-to-buffer to respect display-buffer-alist (or people's shackle.el configurations,etc)

from embark.

oantolin avatar oantolin commented on September 18, 2024

OK, sure, that sounds good. But I don't like forcing bindings on people. I prefer to tell people to pick a binding they like.

from embark.

oantolin avatar oantolin commented on September 18, 2024

embark-occur should probably use switch-to-buffer-other-window or pop-to-buffer instead of switch-to-buffer to respect display-buffer-alist (or people's shackle.el configurations,etc)

Good point.

from embark.

karthink avatar karthink commented on September 18, 2024

Okay I hopped into emacs for a bit to check how occur runs, and I do enjoy having direct access to the embark actions keymap despite a couple of collisions (I have custom navigation keybinds in completion-list-mode-map.) Also the embark actions keymap overrides completion-list-mode-map so my hyopthetical above (with project-find-file) can't happen.

Doing "C-h k" on many embark keymap binds (like "!") in the occur buffer gives:

This lambda is an interactive function without a source file.

even though this is bound to shell-command in embark-file-map. Needs fixing.

from embark.

karthink avatar karthink commented on September 18, 2024

I'm going to be unable to test for a few hours, will update later in the day or tomorrow.

In the spirit of orthogonality, should the functionality for turning a completions buffer into an occur be its own little package? Of course this makes the integration with embark jankier.

from embark.

oantolin avatar oantolin commented on September 18, 2024

I'm using pop-to-buffer now (thanks, it feels much nicer), and I'm generating some function names and copying docstrings for the directly bound actions.

from embark.

clemera avatar clemera commented on September 18, 2024

If people agree with this idea of a separate command, how about the name embark-export for that?

I like that, I think having a separate command for that is a good idea.

But if collisions are a worry, an alternative plan for embark-occur would be to
leave only RET and tell people to bind embark-act to a very comfortable key in
completion-list-mode-map.

I also think collisions could be a problem. I would expect to be able to configure to navigate with n,p,f,b and wouldn't like to either have to take care to avoid collisions or to be forced to use the arrow keys. Personally I would prefer to configure it so that RET becomes the act key. RET RET could then invoke the default action.

Another problem I have is that this currently does not work when there is no completions buffer. With selectrum you don't have one, it would be needed to create one and populate it with selectrum--refined-candidates.

from embark.

oantolin avatar oantolin commented on September 18, 2024

I think one convenient solution could be to bind the act key to RET. RET RET could then invoke the default action.

I think I'm leaning towards a configuration option for whether the direct bindings should be created or not. And if not the user is free to create any binding for embark-act, including RET as you suggest.

Another problem I have is that this currently does not work when there is no completions buffer.

I thought the code was supposed to create the completions buffer if it wasn't there (I'm pretty sure I tested it from live-completions, icomplete and default completion). Does Selectrum do something special to keep the completions buffer from even being created?

EDIT: I bet Selectrum does: it includes the candidates in the minibuffer, and that's probably enough to make the completion machinery fail. (This is just a guess, I am not at my computer.)

from embark.

clemera avatar clemera commented on September 18, 2024

Does Selectrum do something special to keep the completions buffer from even being created?

The completions buffer is very tied to the default completion method. I think it would be a lot of effort to make it "just work" with any completion framework which doesn't simulate the default one.

from embark.

clemera avatar clemera commented on September 18, 2024

I think I'm leaning towards a configuration option for whether the direct bindings should be created or not. And if not the user is free to create any binding for embark-act, including RET as you suggest.

That would be great :)

from embark.

clemera avatar clemera commented on September 18, 2024

Another benefit of implementing a way to create a completion buffer from scratch is that you could then call the occur command also from embark-exit-and-act which I tried first and had to look at the code to see why it doesn't work.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Another benefit of implementing a way to create a completion buffer from scratch is that you could then call the occur command also from embark-exit-and-act

That doesn't sound like it quite makes sense to me: embark-exit-and-act is used to call actions, actions are expected to act on the current target (and must consume the target either by having a minibuffer prompt where it can be injected or by calling embark-target directly). Creating an occur buffer isn't an action at all, it has no use for the target. I guess you could say ignore-the-target-and-create-an-occur-buffer is an action, by adding an explicit (ignore (embark-target)), but why would you?

from embark.

clemera avatar clemera commented on September 18, 2024

Okay it might not fit exactly into the purpose of embark-exit-and-act ;) It's just how I would like to bind my keys but I can handle that problem (I basically want to use one prefix key from which I call any embark command/action).

I think you can ignore selectrum or other frameworks for now. Those should probably provide an occur command themselves.

EDIT: But it would be nice if it would be easy for them to hook into embark-occur using the context info from embark.

from embark.

karthink avatar karthink commented on September 18, 2024

But it would be nice if it would be easy for them to hook into embark-occur using the context info from embark

This is a good idea, assuming it can be done. Ditto for embark actions.

from embark.

clemera avatar clemera commented on September 18, 2024

Ditto for embark actions.

For embark actions framework users can already use embark-target-finders and embark-classifiers that should be enough. I currently use embark with selectrum without problems (with a currently unmerged patch for selectrum though). I will post the needed config on the wiki once it's all settled.

from embark.

oantolin avatar oantolin commented on September 18, 2024

I basically want to use one prefix key from which I call any embark command/action

This works as an action:

(defun embark-ignore-target-and-occur ()
  (interactive)
  (ignore (embark-target))
  (embark-occur))

from embark.

clemera avatar clemera commented on September 18, 2024

This works as an action:

Thanks, as always, with some Elisp no one can stop you ;)

from embark.

oantolin avatar oantolin commented on September 18, 2024

But maybe all along embark-occur really should have done what embark-ignore-target-and-occur does, and should be bound to some key in embark-general-map.

I made it a non-action command because of some silly "conceptual purity", but maybe everyone would prefer to have it bound under the embark-act "pseudo-prefix".

from embark.

clemera avatar clemera commented on September 18, 2024

I guess it doesn't harm to include the (ignore (embark-target)) line inside embark-occur? This way people can easily bind it as an action but don't have, too.

from embark.

oantolin avatar oantolin commented on September 18, 2024

I guess it doesn't harm to include the (ignore (embark-target)) line inside embark-occur? This way people can easily bind it as an action but don't have, too.

Brilliant, yes!

from embark.

oantolin avatar oantolin commented on September 18, 2024

Mmh. It currently goes to top-level, which is what you'd expect for embark-exit-and-act. Should it be possible to call it from embark-act without exiting the minibuffer?

I think ivy-occur always exits, which is why I did that way, but there no real reason to exit. Maybe people want to take a "snapshot" of the candidates as an occur buffer and keep going.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Should it be possible to call it from embark-act without exiting the minibuffer?

As a data point, isearch-occur doesn't terminate the isearch.

from embark.

clemera avatar clemera commented on September 18, 2024

I don't think I would use it that way but would it work to use:

(unless (eq last-command 'embark-act)
  (ignore (embark-target)))

?

from embark.

oantolin avatar oantolin commented on September 18, 2024

I'm confused about what that snippet is supposed to achieve.

from embark.

clemera avatar clemera commented on September 18, 2024

Ups, sorry I misunderstood, you can forget the code snippet it's useless ;)

from embark.

oantolin avatar oantolin commented on September 18, 2024

I can add a embark-completions-buffer-makers variable (like the embark-classifiers or embark-target-finders variable) and Selectrum users can add this function to it:

(defun selectrum-completions-buffer ()
  (interactive)
  (pop-to-buffer "*Completions*")
  (let ((inhibit-read-only t))
    (delete-minibuffer-contents) ; says minibuffer, deletes anyway!
    (completion--insert-strings selectrum--refined-candidates)))

from embark.

clemera avatar clemera commented on September 18, 2024

That would be awesome! Another idea for a name I had would be embark-occur-fillers and later for the export feature embark-export-fillers.

from embark.

oantolin avatar oantolin commented on September 18, 2024

That would be awesome! Another idea for a name I had would be embark-occur-fillers and later for the export feature embark-export-fillers.

I like those names better!

from embark.

clemera avatar clemera commented on September 18, 2024

For the export feature it probably does make less sense to have a embark-export-fillers hook because the filler functions should look the same regardless of the framework. The differing factor would be how to get the candidates. There is already embark-target-finders so maybe embark-targets-finders would do or maybe embark-export-finders.

from embark.

oantolin avatar oantolin commented on September 18, 2024

I think having the contract for functions in embark-occur-fillers be "you must create a buffer named *Completions* in completion-list-mode" is little weird, and it only happened because it's easy for the types I considered: for completions buffers it's just ignore, and for minibuffers (under standard completion) its just minibuffer-completion-help.

What would make more sense is just having this embark-targets-finders (plural targets) and using it both for embark-occur and embark-export. That sounds like a sensible contract: "return a list of all possible targets", not "build a certain buffer in a certain mode".

We do need a better name than embark-targets-finders, though. How about embark-candidate-collectors? (So that "candidate" means "candidate for being a target".)

from embark.

clemera avatar clemera commented on September 18, 2024

I agree with all what you said. Is the plan that frameworks like selectrum can set something like embark-candidate-collectors and occur would just work?

from embark.

oantolin avatar oantolin commented on September 18, 2024

Yes, I think that can be done, and attempting to do it is the plan. The eventual embark-export should work too, with just setting that variable.

(Is there a better name for that too? I'm not so sure how I feel about export. Maybe embark-dwim is a more Emacsy name.)

from embark.

clemera avatar clemera commented on September 18, 2024

I liked the embark-export name, too but here are some other suggestions: embark-abroad or something like embark-overseas.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Big update! I think the occur functionality is done (I still need to write the related export functionality). 4940aab Edit: f0dcce4 has some minor fixes.

  • New variable embark-candidate-collectors. There are currently collectors for minibuffer completions and for completions listed in a completions buffer. For selectrum a function that computes (when selectrum--active-p selectrum--refined-candidates) should be valid (but it needs to be put in the list before embark-minibuffer-candidates).

  • New variable embark-annotators-alist. Currently the only annotations are for symbols: the first line of the docstring. The annotations are displayed in the occur buffer.

  • The Embark Occur buffers now have their own major mode (derived from tabulated list mode). Should it have a default binding for embark-act? Currently the docstring says:

    You should either bind embark-act in embark-occur-mode-map or enable embark-occur-direct-action-minor-mode in embark-occur-mode-hook.

These features should all be strict improvements over the previous version except for one small thing: the occur buffer is now just a vertical list, not a grid. This was done to make room for annotations. Would it be worthwhile to bring back an annotationless grid view as an option?

from embark.

karthink avatar karthink commented on September 18, 2024

from embark.

oantolin avatar oantolin commented on September 18, 2024

A toggle it is, then @karthink. (I prefer grids too.) It's easy to do, I was just being lazy. I'll work on it and report back. (But don't wait for that functionality to test the update if you're curious!)

from embark.

oantolin avatar oantolin commented on September 18, 2024

Done, there is now also a grid view (with no annotations), per @karthink's request. I also added annotations for files and for buffers. There is a variable to control which view is the initial one (should that maybe be an alist, so there can be different default for different completion types?).

I added some keybindings to embark-occur-mode-map. I'm pretty sure I want bindings for those functions, but I am not at all sure I want those keys. Please help select better default keys!

I'm pretty happy with the occur functionality now, it feels fairly polished.

from embark.

karthink avatar karthink commented on September 18, 2024

from embark.

oantolin avatar oantolin commented on September 18, 2024
  • I like n and p, will do.
  • RET already runs default action (it's bound in each indivudal button), so we don't need to also bind it here.
  • I like M-q!
  • binding embark-act to whatever it is bound in minibuffer-local-completion-map only works if people bind it there. I guess we can check that map and also minibuffer-local-map (where I bet @clemera binds it, on account of Selectrum). We should think of a fallback binding in case we don't find one. It seems you like my a binding but want it uppercase, that's fine.
  • embark-occur-direct-actions-minor-mode can't be unconditionally bound to meta + binding for embark-act: what if that binding already has meta (mine does: M-;)?

And only the list view for buffer and files are poor imitations of ibuffer and dired! The grid view is still maybe worth it. And even the list view has two features ibuffer and dired don't: toggling to grid view (!), and RET runnning the default action, that is, whatever command you were in the middle of when you ran embark-occur.

I do want to be able to bring up a dired or ibuffer, but I also want the grid view... I'll think some more about it, but I'm currently thinking something like: list view by default for symbols, grid view by default for files or buffer, separate embark-export command to call dired/ibuffer/package-list, etc.

from embark.

karthink avatar karthink commented on September 18, 2024

from embark.

oantolin avatar oantolin commented on September 18, 2024

My suggestion was to bind embark-occur-direct-actions-minor-mode to A, not embark-act.

Whoops, sorry, got mixed up.

Yes, the grid view is exactly the USP for me

Had to look that up: unique selling proposition. I agree.

I'm hesitant to bind things in keymaps that don't come with embark: minibuffer-local-completion-map or dired-map.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Honestly I'm not looking for much beyond what the completions buffer offers, so I don't expect to use embark-occur heavily.

If you aren't auto-updating the completions buffer, I actually think embark-occur is a slight upgrade from the completions buffer: it doesn't start with an annoying message, it has a true grid view (the completions buffer gets misaligned all the time), and it has a toggle to go to a list view that shows a small amount of extra information. I think I might use it instead of the completions buffer.

from embark.

clemera avatar clemera commented on September 18, 2024

I guess we can check that map and also minibuffer-local-map (where I bet @clemera binds it, on account of Selectrum)

Guilty as charged ;)

list view by default for symbols, grid view by default for files or buffer, separate embark-export command to call dired/ibuffer/package-list, etc.

Could this be configurable? I don't think I would like the grid view as default for files/buffers. I would rather have a consistent behaviour and toggle to grid view on demand.

Don't mixing occur with export feature was a good idea IMO. There could be a t case in the export handler list which could be used to configure the default (fallback to occur for example). This way people would have full freedom to configure the behaviour they want.

from embark.

clemera avatar clemera commented on September 18, 2024

For selectrum a function that computes (when selectrum--active-p selectrum--refined-candidates) should be valid (but it needs to be put in the list before embark-minibuffer-candidates)

Can confirm this works with selectrum. Great work, I love it! Currently the candidate is not automatically inserted and RET pressed for you like when calling actions from the minibuffer right? Is this by accident or intentional?

from embark.

oantolin avatar oantolin commented on September 18, 2024

Currently the candidate is not automatically inserted and RET pressed for you like when calling actions from the minibuffer right? Is this by accident or intentional?

Definitely by accident. I didn't actually test with Selectrum. I'll look into it.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Could this be configurable? I don't think I would like the grid view as default for files/buffers. I would rather have a consistent behaviour and toggle to grid view on demand.

Yeah, I'll put in a configurable default view per category.

from embark.

clemera avatar clemera commented on September 18, 2024

Definitely by accident. I didn't actually test with Selectrum. I'll look into it.

After some debugging I think the reason is that embark-target-finders doesn't contain a finder function for embark-occur-mode.

from embark.

oantolin avatar oantolin commented on September 18, 2024

After some debugging I think the reason is that embark-target-finders doesn't contain a finder function for embark-occur-mode.

It does: embark-button-label.

from embark.

clemera avatar clemera commented on September 18, 2024

Oops, sorry, either I just evaled the buffer after fetching the new commits and the new value wasn't picked up because it's defined by defcustom or it was overridden by my config. Either way it works now!

from embark.

oantolin avatar oantolin commented on September 18, 2024

Oops, sorry, either I just evaled the buffer after fetching the new commits and the new value wasn't picked up because it's defined by defcustom or it was overridden by my config. Either way it works now!

I was about to say, embark seems to be working fine with selectrum over here. Of course, my config isn't much fun since here embark has no idea how to classify selectrum completion sessions (expect for M-x, it knows those are commands). Did you patch your selectrum to bind minibuffer-completion-table?

from embark.

oantolin avatar oantolin commented on September 18, 2024

OK! An initial version of embark-export is done (22836cd). There are currently just exporters for files (to dired) and buffers (to ibuffer), everything else defaults to exporting to embark-occur.

I want some better integration between occur and export. The plan is to make embark-occur work in dired and ibuffer; and to make embark-export work in Embark Occur mode buffers. That way @karthink can easily go back and forth between occur grid view and export.

from embark.

oantolin avatar oantolin commented on September 18, 2024

And the two-way bridge is done (b195eaf). You can export from occur and viceversa.

I think this issue is finally near closing. A few pending matters:

  • Update the package commentary and README.
  • If you call occur from dired or ibuffer (or occur, but why would you?) there is no default action. I guess I should just say the default default action for files is find-file, for buffers is switch-to-buffer, etc.
  • This needs some more extensive testing!
  • I'd love some more feedback on keybindings, and default settings.

I think I'll just use it for a while, make sure everything makes sense.

from embark.

clemera avatar clemera commented on September 18, 2024

I hadn't much time to test this yet but what I tested went well, thank you very much!

I had another idea not directly related to the occur feature which is to be able to execute an action on all candidates in sequence. Maybe this could replace the current meaning of the the prefix argument of embark-act and embark-exit-and-act.

from embark.

clemera avatar clemera commented on September 18, 2024

A few more things I noticed:

  • the dired buffers are named like the default directory. Maybe it would be preferable to have a name like *embark-export-dired-%s* where %s denotes the default directory or similar. This way there wouldn't be a possible name conflict with existing dired buffers.
  • the Back to top level message is currently not inhibited.
  • I have to convert the candidates with directory-file-name or I get errors sometimes and directory entries ending in double slashes (if the candidates contain full pathes like with recentf-list).

from embark.

clemera avatar clemera commented on September 18, 2024

Having C-u mean "act on all candidates" is a cool idea, I'll think about it.

After thinking about it that might not work well for the general case. Maybe only certain actions should be enabled for this if at all.

On a related note for some commands repeating the last command (done by and embark-occur-select and embark-exit-and-act ) does not work well currently. If the candidates for completing-read are computed by a computationally expensive process (describe symbols, grepping and such) it is slow to recompute them. Maybe completing-read-function could be temporary overridden (maybe also read-file-name-function and read-buffer-function) to immediately return the chosen candidate so the whole candidates set wouldn't have to be computed again:

(defun embark--bind-actions (exitp)
  "Set transient keymap with bindings for type-specific actions.
If EXITP is non-nil, exit all minibuffers too."
  (set-transient-map
   embark--keymap
   #'embark--keep-alive-p
   (lambda ()
     (setq embark--keymap nil)
     (run-hooks 'embark-pre-action-hook)
     (when (and exitp
                (not (memq this-command
                           '(embark-cancel embark-undefined))))
       (embark-after-exit
         (this-command prefix-arg embark--command embark--target-buffer)
         (let ((completing-read-function
                (lambda (&rest _)
                    (embark-target))))
           (command-execute this-command)))))))

(defun embark-occur-select (_entry)
  "Run default action on ENTRY."
  (setq this-command embark--command)
  (embark--setup)
  (let ((completing-read-function
                (lambda (&rest _)
                  (embark-target))))
    (call-interactively this-command)))

That would be an alternate route for inject/cleanup for commands which use completing-read.
My first tests seem to work well and occur feels much snappier with this. But we would have to look how to integrate this with the use of embark-setup-hook, embark-setup-overrides and embark-allow-edit-default. Maybe those should then only be used to setup commands which don't use completing-read. Alternatively we could decide to run them along the lines of this:

(let* ((embark-setup-hook
        (or (alist-get this-command embark-setup-overrides)
            embark-setup-hook))
       (completing-read-function
        (if embark-allow-edit-default
            completing-read-function
          (lambda (&rest _)
            (with-temp-buffer
              (save-excursion
                (insert (embark-target)))
              (run-hooks 'embark-setup-hook)
              (buffer-string))))))
  (command-execute this-command))

But I'm not sure if that could be useful.

from embark.

oantolin avatar oantolin commented on September 18, 2024

After thinking about it that might not work well for the general case. Maybe only certain actions should be enabled for this if at all.

Yes, I was thinking that too. Also, most completion UIs only show you a tiny number of candidates, say 15 or fewer, it would be scary to run an action on all candidates when you can't even see them. I think if you really want mass action, it's likely to be on files and buffers, and embark-export has you covered: dired and ibuffer have good UIs for acting en masse.

On a related note for some commands repeating the last command (done by and embark-occur-select and embark-exit-and-act ) does not work well currently. If the candidates for completing-read are computed by a computationally expensive process (describe symbols, grepping and such) it is slow to recompute them.

Do you have a particularly slow example so I can test locally? (I tested with describe-symbol and it feels instantaneous with Emacs 26.1 on my cheap 6-year old Chromebook.) Otherwise I guess I can fake it with a hand-crafted completing-read call involving sit-for.

But even before that: I'm not sure how changing completing-read is supposed to help. I thought you said the problem was that computing the candidate list was slow? That gets computed before completing-read is even called, so it still gets computed with your modifications. Probably the cause of the slowdown is something else then, no?

I think more research is needed before changing anything in embark.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Did this slowness happen with Selectrum? Maybe it's the problem? Maybe it generates all possible candidates (i.e., the completions of the empty string) and their annotations, even though the target is inserted right away? Do you see the same slowness with Icomplete, for example?

I remember when I tried Selectrum thinking that it could use a tiny delay before computing candidates so the minibuffer could pop up right away and then the candidates could fill-in when they were ready. As it was when I tested it, the minibuffer waited for the candidates to be ready and they popped up at the same time. (I remember feeling happy to discover I didn't have to wait for the minibuffer to start typing, and thinking that the reason I knew I didn't have to wait with Icomplete is that the minibuffer prompt appeared even before the candidates were computed.)

from embark.

clemera avatar clemera commented on September 18, 2024

I thought you said the problem was that computing the candidate list was slow? That gets computed before completing-read is even called, so it still gets computed with your modifications.

Oh yes your are right, it's selectrums fault.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Oh yes your are right, it's selectrums fault.

I think a tiny delay at the beginning of the selectrum process would fix this: a delay just long enough to give time for really fast input. It's great for automated insertion like embark does, but also for those times when you know before-hand you are just going to press RET to accept the default (in which case you are likely to press it pretty darn fast for a human). A small delay means that those uses can happen before any completion candidates are computed.

Also, if the delay is placed correctly, this would allow the minibuffer prompt to appear earlier in Selectrum, which is a great visual cue that you can start typing (you can start anyway, and Selectrum doesn't loose the keystrokes, but it is disconcerting to type with no prompt).

from embark.

clemera avatar clemera commented on September 18, 2024

That is a good idea and might work, I have to look into it. For now do you think it would be okay to rebind completing-read-function to completing-read-default as long as embark-allow-edit-default is nil? I think the user won't notice which completion framework is used anyway when he doesn't get a chance to edit the input.

from embark.

oantolin avatar oantolin commented on September 18, 2024

That is a good idea and might work, I have to look into it. For now do you think it would be okay to rebind completing-read-function to completing-read-default as long as embark-allow-edit-default is nil? The user won't notice which completion framework is used anyway when he doesn't get a chance to edit the input.

Rebinding around the entire run of the next command is not such a great idea for commands that read more than input, like rename-file! Maybe when editing is not allowed, one could save the completing-read-function, set it to completing-read-default, and then after inserting the target restore the old value.

from embark.

clemera avatar clemera commented on September 18, 2024

Right reading input more then one time is a problem :(

from embark.

oantolin avatar oantolin commented on September 18, 2024

Hey, repeating the command just makes this worse, but you probably see the problem even with embark-act, right (maybe with only half the pause)?

from embark.

clemera avatar clemera commented on September 18, 2024

but you probably see the problem even with embark-act, right (maybe with only half the pause)?

No the the candidates are already computed there, it just picks the selected one.

from embark.

clemera avatar clemera commented on September 18, 2024

Oh you mean for occur? I haven't tried that.

from embark.

oantolin avatar oantolin commented on September 18, 2024

No the the candidates are already computed there, it just picks the selected one.

I don't understand this. For example, if I'm in execute-extended-command and I run embark-act to use describe-symbol on the selected command, the candidates aren't already computed, they aren't even the same candidates.

Oh you mean for occur? I haven't tried that.

No, I did mean embark-act, as I tried to explain above.

from embark.

clemera avatar clemera commented on September 18, 2024

Okay I think I got it wrong. But there isn't a delay in this case, I currently don't know why.

from embark.

oantolin avatar oantolin commented on September 18, 2024

Okay I think I got it wrong. But there isn't a delay in this case, I currently don't know why.

How about this?

  1. Try inserting (sit-for 0.01) right at the beginning of selectrum-completing-read and see if it solves your problem.

  2. If not, use the completing-read-function trick (maybe my amended one for multiple inputs). If you configure embark to never allow editing, this is enough. Otherwise I guess you might want to check in each case if editing is allowed before using the modified completing-read-function.

  3. We wait before committing any changes to embark until the situation is clearer.

from embark.

clemera avatar clemera commented on September 18, 2024

Try inserting (sit-for 0.01) right at the beginning of selectrum-completing-read and see if it solves your problem.

This won't work because the minibuffer isn't opened when selectrum computes the candidates. The computing of the candidates would probably have to be moved to a stage where the minibuffer is open already.

We wait before committing any changes to embark until the situation is clearer.

Of course I was totally wrong headed with my assumptions. I will investigate a bit and report back.

from embark.

oantolin avatar oantolin commented on September 18, 2024

This won't work because the minibuffer isn't opened when selectrum computes the candidates. That would probably have to be done when the minibuffer is open.

Of course! Silly me.

And just to make sure I'm not being wrong headed: did you test the things were you saw the slowdown in Icomplete, to make sure it's to do with Selectrum?

from embark.

clemera avatar clemera commented on September 18, 2024

did you test the things were you saw the slowdown in Icomplete, to make sure it's to do with Selectrum?

No I haven't but I'm pretty sure it's because of selectrum and some weird interaction with how I have configured embark for it. I will check tomorrow.

from embark.

oantolin avatar oantolin commented on September 18, 2024

No I haven't but I'm pretty sure it's because of selectrum and some weird interaction with how I have configured embark for it. I will check tomorrow.

Thanks for checking! It might still be something that embark needs to handle differently.

from embark.

clemera avatar clemera commented on September 18, 2024

But there isn't a delay in this case, I currently don't know why.

I used embark-describe-symbol which doesn't compute any candidates. I also don't have the delay issue at all with icomplete. I have an idea how to fix it on selectrums side and will add that to my PR there.

a version of embark-act that updates live while you type

I'm not sure what that means, did you meant embark-occur version that updates live?

from embark.

oantolin avatar oantolin commented on September 18, 2024

I used embark-describe-symbol which doesn't compute any candidates. I also don't have the delay issue at all with icomplete. I have an idea how to fix it on selectrums side and will add that to my PR there

Excellent!

I'm not sure what that means, did you meant embark-occur version that updates live?

Yes, that's what I meant. Thanks. (I edited the original comment.)

from embark.

oantolin avatar oantolin commented on September 18, 2024
  (setq completing-read-function
        (defun embark-completing-read (&rest args)
          "A completing read function that shows candidates with embark-occur."
          (run-with-idle-timer 0.3 nil
           (lambda () (when (minibufferp) (embark-occur))))
          (apply #'completing-read-default args)))

My new completion "framework"... 😄 (I'm crazy enough to actually be using that right now.)

from embark.

protesilaos avatar protesilaos commented on September 18, 2024

My new completion "framework"

Good idea! Is there scope for an embark-occur-do macro which will provide this on a per-function basis?

from embark.

oantolin avatar oantolin commented on September 18, 2024

Good idea! Is there scope for an embark-occur-do macro which will provide this on a per-function basis?

Yes, of course! I knew you'd want such a macro and I'll make one. I was thinking you should be able to specify either what category you want the completion to be treated as, or directly what annotation function and action key map you want.

from embark.

protesilaos avatar protesilaos commented on September 18, 2024

I was thinking you should be able to specify either what category you want the completion to be treated as, or directly what annotation function and action key map you want.

I like this!

from embark.

clemera avatar clemera commented on September 18, 2024

My new completion "framework"... smile (I'm crazy enough to actually be using that right now.)

That is cool! With a bit more polish this could become pretty neat actually.

from embark.

oantolin avatar oantolin commented on September 18, 2024

I'm not sure how best to handle refocusing the Embark Occur buffer, @clemera, but you can handle inhibiting using the same window with:

(defun from-embark-occur-p (_buffer _actions)
  "Is current buffer in Embark Occur mode?
Note this does not test _BUFFER, the buffer to be displayed, but
rather the buffer current when the request to display _BUFFER was
made."
  (derived-mode-p 'embark-occur-mode))

(push '(from-embark-occur-p . (display-buffer-pop-up-window
                                (inhibit-same-window . t)))
      display-buffer-alist) 

from embark.

clemera avatar clemera commented on September 18, 2024

Nice thank you! I have deleted my comment because I thought I didn't wanted to bother everyone with my personal config needs ;) I have currently the following using some hacky reselect of the window:

(defun display-from-occur-p+ (_name _action)
  ;; The buffer might have already changed so get the buffer from which the
  ;; display action was invoked from the still selected window.
  (with-current-buffer (window-buffer)
    (derived-mode-p 'embark-occur-mode)))

(defun display-from-occur+ (buf _alist)
  (prog1 nil
    (unless (window-dedicated-p)
        (set-window-dedicated-p (selected-window) 'yes))
    ;; reselect the occur window in case it loosed focus 
    (let ((curr (selected-window)))
      (run-at-time 0 nil (lambda ()
                           (select-window curr))))))
(setq display-buffer-alist
      (cons '(display-from-occur-p+ (display-from-occur+))
            display-buffer-alist))

from embark.

oantolin avatar oantolin commented on September 18, 2024

I have deleted my comment because I thought I didn't wanted to bother everyone with my personal config needs ;)

I think this particular configuration desire might be fairly popular. Would you mind writing about this in the wiki?

from embark.

clemera avatar clemera commented on September 18, 2024

I can do that but my current version does not work well. It still needs some tweaking I'm on it ;) Will add it to the wiki when I figured it out.

from embark.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.