Skip to content

cider-popup-buffer-display: honor special-display-buffer-names if customized for a given CIDER buffer name #3568

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Nov 3, 2023

Before this PR, if I (add-to-list 'special-display-buffer-names '("*cider-inspect*" my/display-fn)), the buffer would be rendered twice - once through one's custom function, and once again through CIDER.

I verified it works as intended.

Cheers - V

… customized for a given CIDER buffer name
@vemv vemv requested a review from bbatsov November 3, 2023 22:52
@bbatsov
Copy link
Member

bbatsov commented Nov 4, 2023

I've never heard of this Emacs functionality, but now that we support it we should probably mention it somewhere in the docs. Even I'm not sure why would the end users want to play with this right now, so some examples would be welcome.

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

I considered it, but most of all I didn't find an adequate page in the manual.

How about: for each possible target (Inspector, etc), mention in their specific page:

You can control how the popup is rendered using the special-display-buffer-names variable blah blah....

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

I'll merge this so as not to pile up PRs (few incoming), but LMK about the best page(s) to doc this

@vemv vemv merged commit c14080e into master Nov 4, 2023
@vemv vemv deleted the cider-popup-buffer-display branch November 4, 2023 16:05
@bbatsov
Copy link
Member

bbatsov commented Nov 4, 2023

How about: for each possible target (Inspector, etc), mention in their specific page:

That sounds reasonable. It can also mentioned only once around the popup-related settings.

@rrudakov
Copy link
Contributor

rrudakov commented Nov 4, 2023

This update introduced a bug: when I evaluate code using C-c C-p the result should be rendered in the *cider-result* buffer, now this buffer is empty.

Before:

image

After:

image

@rrudakov
Copy link
Contributor

rrudakov commented Nov 4, 2023

Also according to the docs, variable special-display-buffer-names is obsolete since emacs version 24.3:

special-display-buffer-names is a variable defined in ‘window.el’.

Its value is nil

This variable is obsolete since 24.3; use ‘display-buffer-alist’
instead.

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

Thanks!

I think I know the cause - there were two kinds of code paths for raising popups.

Checking

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

Anyway, what I find odd is that you perceive any difference given that your special-display-buffer-names value is nil.

The code branch that does not handle special-display-buffer-names is essentially untouched - can you spot a bug in the diff?

@rrudakov
Copy link
Contributor

rrudakov commented Nov 4, 2023

The only difference I can see is that function cider-popup-buffer-display returns string now (buffer-name) and before it was returning a buffer. I guess this returned value was used to insert the result to the new buffer.

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

Good catch - that was it!

Pushing to master now

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

I'll also see about the deprecated var. The old one has worked for me for some 5 years but yeah, we should primarily support the modern alternative

That can wait though

vemv added a commit that referenced this pull request Nov 4, 2023
@rrudakov
Copy link
Contributor

rrudakov commented Nov 4, 2023

I've been using display-buffer-alist for a while to control windows behavior, wasn't even aware of special-display-buffer-names until this moment :)

@vemv
Copy link
Member Author

vemv commented Nov 4, 2023

I'll have to check it out. Had you successfully used it with cider buffers?

@rrudakov
Copy link
Contributor

rrudakov commented Nov 4, 2023

Yes, sure.

Here's the relevant part of my config if you're curious (pretty big, but it's easy to spot CIDER-related buffers): https://git.sr.ht/~rrudakov/dotfiles/tree/feature/dotfiles-ng/item/templates/emacs.d/emacs.org#L630-859

Also:

@bbatsov
Copy link
Member

bbatsov commented Nov 4, 2023

This variable is obsolete since 24.3; use ‘display-buffer-alist’

CIDER only supports Emacs 26+, so it's pointless to support the legacy variable IMO (so we can completely revert those changes). I didn't know about it, but I certainly know about display-buffer-alist. :-)

@vemv
Copy link
Member Author

vemv commented Nov 5, 2023

I'll revert it today after migrating my personal stuff special-display-buffer-names -> display-buffer-alist and verifying it keeps working as usual w/ cider buffers.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants