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

Fix for previewing on not current window #670

Closed
wants to merge 2 commits into from

Conversation

aki2o
Copy link

@aki2o aki2o commented Oct 15, 2022

consult--jump-preview could not work with the feature that manages display-buffer-function like e2wm.el.
That's because consult--buffer-action don't have a consideration for the case that current-buffer won't be switched.

So, I tried to fix this trouble and it looks to work well with this fix.

@@ -469,7 +469,7 @@ called from the corresponding command. Note that the options depend on
the private `consult--read' API and should not be considered as stable
as the public API.")

(defvar consult--buffer-display #'switch-to-buffer
(defvar consult--buffer-display #'display-buffer
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using e2wm.el and it must support switch-to-buffer.
However, it happens to collapse window layout in my environment.

Anyway, I believe display-buffer is more appropriate for this package role. So, I hope this fix be included

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, switch-to-buffer is the better choice here. You can set switch-to-buffer-obey-display-actions or change the variable in your configuration. Please change back.

;; Handle positions with overlay information
(consult--with-jump (or (car-safe cand) cand)
(setq consult--preview-previous-invisibles (consult--invisible-open-temporarily)
consult--preview-previous-overlays
Copy link
Author

@aki2o aki2o Oct 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel invisible should be defined for not local but global because void variable error happened.
And I changed the name to be more explicit.
Is my understanding wrong?

Copy link
Owner

@minad minad Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is wrong. Please revert the change to the overlays/invisibles variable.


(defun consult--jump (pos)
"Push current position to mark ring, go to POS and recenter."
(mapc #'funcall consult--preview-previous-invisibles)
(mapc #'delete-overlay consult--preview-previous-overlays)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the variables need to be cleared here as well.
And I guess there is a timing that's more appropriate to do this procedure but I couldn't find it.

,@body
(run-hooks 'consult-after-jump-hook))))
(if (window-live-p w)
(with-selected-window w (funcall f))
Copy link
Author

@aki2o aki2o Oct 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is taking care of the cases like

  • current-buffer is not the target buffer
  • the window of the target buffer is not selected window

(funcall f)))))

(defvar consult--preview-previous-invisibles nil)
(defvar consult--preview-previous-overlays nil)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these variables.

@minad
Copy link
Owner

minad commented Oct 16, 2022

I close this PR in favor of #671. We should first fix consult--buffer-preview to handle display-buffer properly before trying to fix consult--jump-preview.

@minad minad closed this Oct 16, 2022
minad added a commit that referenced this pull request Oct 19, 2022
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.

2 participants