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

Move from display-buffer-overriding-action to display-buffer-base-action #82

Open
bmag opened this issue Feb 19, 2016 · 3 comments
Open

Comments

@bmag
Copy link
Owner

bmag commented Feb 19, 2016

Currently Purpose uses display-buffer-overriding-action to plug into display-buffer, but that makes it harder to use display-buffer-alist for customizing display rules. We might get better results by using display-buffer-base-action.

Pros:

  • display rules are customizable by display-buffer-alist. We can get rid of purpose-special-action-sequences and purpose-action-function-ignore-buffer-names.
  • simplified purpose--action-function, as it won't need to re-implement parts of display-buffer.

Cons:

  • integration with other packages needs to be re-evaluated (especially Popwin can be problematic).
  • need to find reliable way to run purpose-display-buffer-functions after display-buffer displays a buffer. To be fair, current situation isn't ideal either.
  • drop on users a major change of display configuration
  • switch-to-buffer advice falls back to correct switch-to-buffer behavior when purpose-mode is disabled?
@bmag bmag added this to the 2.0 milestone Feb 21, 2016
@bmag bmag changed the title Consider moving from display-buffer-overriding-action to display-buffer-base-action Move from display-buffer-overriding-action to display-buffer-base-action Feb 21, 2016
@bmag bmag added the refactor label Feb 21, 2016
@bmag
Copy link
Owner Author

bmag commented Feb 12, 2021

Another pro: the docstring of display-buffer-overriding-action was changed in 26.2 to say specifically that it should not be changed permanently by elisp programs. Meaning, Purpose is doing something that contradicts the intended usage of display-buffer-overriding-action. Looks like some packages and modes have started utilizing this variable as intended by Emacs: #167, #168, #169.

@wyuenho
Copy link
Collaborator

wyuenho commented Feb 12, 2021

I just had a first pass of implementing the ideas in this ticket. As a minimal change, simply swapping display-buffer-overriding-action with display-buffer-base-action is enough to obviate the need for #167 #168 and #169. I haven't run into any issues thus far. Perhaps we should break this ticket down, and try doing just that.

display rules are customizable by display-buffer-alist. We can get rid of purpose-special-action-sequences and purpose-action-function-ignore-buffer-names.
simplified purpose--action-function, as it won't need to re-implement parts of display-buffer.

Yes, but clean up can be done separately.

need to find reliable way to run purpose-display-buffer-functions after display-buffer displays a buffer. To be fair, current situation isn't ideal either.

Good old advice-add to display-buffer to run the hooks after invocation? I'm not sure what the problems are currently. This needs some explaination. purpose-display-buffer-functions is left as nil by default and I've never found a need to use it for the many years I've been using this package.

drop on users a major change of display configuration

All minor details as a consequence of the clean up effort. It can be done separately and I'm sure there are ways to minimize the impact.

integration with other packages needs to be re-evaluated (especially Popwin can be problematic).

TBH, we should just remove this integration. The popwin extension works perfectly fine. Anything else that doesn't work with it can easily be done by modifying purpose-x-popwin-buffer-names and friends.

switch-to-buffer advice falls back to correct switch-to-buffer behavior when purpose-mode is disabled?

While I'm not sure what this is referring to, I'm sure switch-to-buffer-obey-display-actions introduced in 27.1 will actually have an effect instead of ignored as now.

wyuenho referenced this issue in syl20bnr/spacemacs Mar 4, 2021
problem:
trying to open a magit buffer after
reloading the configuration: `SPC f e R`

shows the message:
>Lisp nesting exceeds ‘max-lisp-eval-depth’

cause:
purpose-x-magit-multi-on can't be called twice,
without turning it off first: purpose-x-magit-off

Spacemacs, purpose-x-magit-multi-on, Lisp nesting exceeds ‘max-lisp-eval-depth’
bmag/emacs-purpose#178

solution:
in this case we'll only call: purpose-x-magit-multi-on
once per Emacs session.

notes:
this also removes the call to: with-eval-after-load 'magit
because it's handled upstream:
https://github.com/bmag/emacs-purpose/blob/c85dd3c9f70cd593d6bc79c40855a240e55b2575/window-purpose-x.el#L243
@bmag
Copy link
Owner Author

bmag commented Mar 17, 2021

I actually started implementing some of it in base-action branch, that is based on develop branch. The develop branch was meant to become version 2.0 of Purpose, but that work has stalled about 4-5 years ago and it didn't keep up with changes from master. I'll break the changes from develop and base-action into separate detailed issues/PRs against master soon and create a "project" to track them.

There will also be a PR for moving to display-buffer-base-action, and I'll also break down this current ticket, then it will be easier to work on each part separately.

I think we can get rid of purpose-display-buffer-functions, yeah, I'd like to deviate a bit less from stock Emacs behavior.

TBH, we should just remove this integration. The popwin extension works perfectly fine.

IIRC the integration prevents popwin from deleting the purpose-dedicate window parameter. It can hit users that have popwin as a dependency of some other package, and not under their direct control.

switch-to-buffer-obey-display-actions

I'd like to rely on it and remove the advice, but I'd also like to support Emacs 26. The issue with advising switch-to-buffer and also moving away from display-buffer-overriding-action, is that the buffer might be displayed by some function in display-buffer-alist which most likely won't display the buffer in the currently selected window. For now this is something to keep in mind and test once there is a PR.

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

No branches or pull requests

2 participants