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

consult--read: Introduce new :state function protocol (BREAKING API CHANGE) #546

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

minad
Copy link
Owner

@minad minad commented Apr 7, 2022

We need a fine-grained protocol in order to undo buffer preview (See #354).
See consult--with-preview for details:

consult/consult.el

Lines 1434 to 1475 in a79b620

(defmacro consult--with-preview (preview-key state transform candidate &rest body)
"Add preview support to BODY.
STATE is the state function.
TRANSFORM is the transformation function.
CANDIDATE is the function returning the current candidate.
PREVIEW-KEY are the keys which triggers the preview.
The state function takes two arguments, an action argument and the
selected candidate. The candidate argument can be nil if no candidate is
selected or if the selection was aborted. The function is called in
sequence with the following arguments:
1. 'setup nil After entering the minibuffer (minibuffer-setup-hook).
⎧ 2. 'preview CAND/nil Preview candidate CAND or reset if CAND is nil.
⎪ 'preview CAND/nil
⎪ 'preview CAND/nil
⎪ ...
⎩ 3. 'preview nil Reset preview.
4. 'exit nil Before exiting the minibuffer (minibuffer-exit-hook).
5. 'return CAND/nil After leaving the minibuffer, CAND has been selected.
The state function is always executed with the original window selected,
see `minibuffer-selected-window'. The state function is called once in
the beginning of the minibuffer setup with the `setup' argument. This is
useful in order to perform certain setup operations which require that
the minibuffer is initialized. During completion candidates are
previewed. Then the function is called with the `preview' argument and a
candidate CAND or nil if no candidate is selected. Furthermore if nil is
passed for CAND, then the preview must be undone and the original state
must be restored. The call with the `exit' argument happens once at the
end of the completion process, just before exiting the minibuffer. The
minibuffer is still alive at that point. Both `setup' and `exit' are
only useful for setup and cleanup operations. They don't receive a
candidate as argument. After leaving the minibuffer, the selected
candidate or nil is passed to the state function with the action
argument `return'. At this point the state function can perform the
actual action on the candidate. The state function with the `return'
argument is the continuation of `consult--read'. Via `unwind-protect' it
is guaranteed, that if the `setup' action of a state function is
invoked, the state function will also be called with `exit' and
`return'."

…HANGE)

We need a fine-grained protocol in order to undo buffer preview (See #354).
See `consult--with-preview` for details.
@minad minad merged commit 3668df6 into main Apr 7, 2022
@minad minad changed the title Introduce new preview function protocol: setup, preview, exit, action consult--read: Introduce new :state function protocol (BREAKING API CHANGE) Apr 7, 2022
@minad
Copy link
Owner Author

minad commented Apr 7, 2022

I apologize for the breakage! The API change was necessary to implement preview
unwinding properly for buffers. The new preview protocol is generally useful
since it gives the preview function more control. I considered implementing this
in a backward compatible way but then we would have ended up with many
duplicated preview functions or duplicated logic.

This API change affects these MELPA projects which make use of preview:

  • consult-eglot and consult-yasnippet by @mohkale
  • consult-lsp by @gagbo
  • consult-notmuch by @jaor
  • consult-org-roam by @jgru

@oantolin
Copy link
Contributor

oantolin commented Apr 7, 2022

Do you think it would be a good idea for embark-consult to add pre and post actions hooks that disable and reenable the preview for every action? I've been thinking about that for a while now, but the old exposed preview function didn't let me do that if I recall correctly. See oantolin/embark#352, for example.

@minad
Copy link
Owner Author

minad commented Apr 7, 2022

I am not sure I understand 352 correctly. When I preview a bookmark, switch to the buffer, move the point around, go back to the minibuffer press C-. s then I can update the position. But sure, we can discuss extensions to the preview mechanism which make this more controllable from Embark.

(EDIT: Ah I misunderstood oantolin/embark#352. I got it now and I will think about it.)

@oantolin
Copy link
Contributor

oantolin commented Apr 7, 2022

We can discuss that issue over there to keep this place free for the people you pinged about their packages to ask questions.

jgru added a commit to jgru/consult-org-roam that referenced this pull request Apr 8, 2022
Change the consult-org-roam--node-preview to conform with consult's
API change (announced in minad/consult#546)
@jgru
Copy link

jgru commented Apr 8, 2022

Thanks for informing me about the breaking change, @minad.
I modified consult-org-roam's preview-function in 1fc314c101cb4e9455c8aa294d587873507b6a5e to conform with consult's API.

Best regards,
jgru

@jaor
Copy link

jaor commented Apr 8, 2022 via email

@minad
Copy link
Owner Author

minad commented Apr 8, 2022

@jaor There was an issue. Should be fixed again in e926d59.

@minad
Copy link
Owner Author

minad commented Apr 8, 2022

Regarding the correctness of your preview function:

  • (funcall (consult--temporary-files)) does not look right. This should be a nop.
  • I suggest to take a look at consult--file-preview and consult--buffer-preview for inspiration.

@jaor
Copy link

jaor commented Apr 8, 2022 via email

@minad
Copy link
Owner Author

minad commented Apr 8, 2022

it seems that, with the new protocol, i only need to care of the preview
action...

@jaor Yes if you don't need additional setup/cleanup.

@jaor
Copy link

jaor commented Apr 8, 2022 via email

mohkale added a commit to mohkale/consult-yasnippet that referenced this pull request Apr 9, 2022
mohkale added a commit to mohkale/consult-eglot that referenced this pull request Apr 9, 2022
@mohkale
Copy link
Contributor

mohkale commented Apr 9, 2022

Hi. I've updated consult-eglot and consult-yasnippet. Thanks for the heads up :-).

@jaor
Copy link

jaor commented Oct 11, 2022 via email

@minad minad deleted the rework-preview branch January 31, 2024 14:54
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.

5 participants