tminor / jsonnet-mode

Emacs major mode for editing Jsonnet files.
GNU General Public License v3.0
46 stars 17 forks source link

Display the eval buffer using the default display-buffer-alist #29

Closed russell closed 3 years ago

russell commented 3 years ago

There is so rather complex implicit behavior when using jsonnet-eval-buffer, it was really really crazy before #27 when @tminor picked up bad scoping in with-current-buffer. I actually don't know how anyone used it before #27, it was essentially burying the buffer on execute, which was the confusing behavior i was seeing, turned out one machine was running the melpa version on was running my fork. Anyway, I was really trying to make small changes, because i don't like it when people go to edit one sexp and then end up re-writing the entire function, so this PR is only covering how buffer display works,

So to recap, the current implementation works as follows

  1. delete 1 window containing jsonnet-output
  2. run jsonnet command and populate buffer
  3. display-buffer using a default set of rules
    1. display-buffer-pop-up-window (this will almost always succeed)
    2. display-buffer-reuse-window (this will never succeed, we deleted the window in step 1, unless you have multiple windows all with the jsonnet-output buffer then it will be reused)
    3. display-buffer-at-bottom (starts splitting and displaying the buffer in new windows)
    4. display-buffer-pop-up-frame (creates a new frame to display the buffer)

Ok, so i really don't like this behavior, mostly because it begins with deleting my existing window, this leads to either window flapping where i happens to have the buffer in an already created side-window, or worse, window thrashing, where the display is rearranging as windows are deleted and then new ones appear miraculously. I don't think it ever made it to 3 or 4.

I have very minimal customization of my window display configuration, but even with no display-buffer-alist the behavior here feels unpredictable. It's trying to force the window from where ever it is into a side window at the bottom of the screen, and it's not straight forward to prevent it, because once the window is deleted, it's not obvious how to return it to it's original location. and even if i did it would still be flapping about windows disappearing only to reappear in the same place

So what i propose here is minimalism, the flow works like this

  1. run jsonnet command and populate buffer
  2. call display-buffer with no arguments other than '(inhibt-same-window . t) this will inhibit the default same window behavior that can sometimes occur.

This is the simplest way i can imagine it working, i press C-c C-c and then the buffer evaluates and is displayed somewhere, not over the top of the current window. If the output window is already visible, then it's reused.

If users want to customize the behavior and delete the window every time they execute the function, the could advise it, and if they want to change the display to use a side-window, then they can customize the alist like so

(customize-set-variable
 'display-buffer-alist
 '(("\\*jsonnet out\\*"
    (display-buffer-pop-up-window display-buffer-reuse-window display-buffer-at-bottom display-buffer-pop-up-frame))))

WDYT? I realise this is very different to what is there, but it's more inline with other compile buffer behavior and it's aiming to not be opinionated. Something simple that people can build on

Sorry for the long post, got a bit carried away

Have you written tests for your change?

Types of changes

Checklist:

tminor commented 3 years ago

No worries about the long post! I prefer it over something terse and unclear.

I said earlier that I don't customize display-buffer-alist in a way that should affect Jsonnet's eval buffer... but that's not true. My related customization is extensive and I missed a regular expression that matches "*jsonnet output*". Here's a short snippet extracted from the my customized display-buffer-alist value demonstrating what I'm referring to:

`((,(rx string-start (or "*Apropos"
                         ;; Many lines ommitted here
                         (and (1+ not-newline) " output*")))
   (display-buffer-reuse-window display-buffer-in-side-window)
   (direction . rightmost)
   (side . right)
   (window-width . 80)
   (window-height . 0.45)))

I of course like this behavior but wouldn't want to force it on all jsonnet-mode users.

I'm in favor of minimally invasive display-buffer behavior; I find it infuriating when packages demolish my window configuration without providing a way to prevent such rude behavior (cough org-mode).

I think it would be nice to give users a customizable setting for Jsonnet's particular case, though; display-buffer-alist isn't very user friendly and I certainly wouldn't tell a new Emacs user to "just customize display-buffer-alist if you want different behavior." We could add some simple instructions in the README for such a customization. Does that seem reasonable?

russell commented 3 years ago

That is vastly more complex then what i have. I don't know how i managed to use Emacs for 10 years and never configured the display-buffer-alist I think i spent 5 years assuming that i didn't understand how Emacs worked and the latter 5 years thinking that there must be a good reason for the seemingly random behavior of some windows. :facepalm:

I think it would be nice to give users a customizable setting for Jsonnet's particular case, though; display-buffer-alist isn't very user friendly and I certainly wouldn't tell a new Emacs user to "just customize display-buffer-alist if you want different behavior." We could add some simple instructions in the README for such a customization. Does that seem reasonable?

I'm not totally opposed to it adding customization option to jsonnet-mode, but it will add more complexity to your mode and will not help those users when it comes to dealing with other buffers of a similar nature. Another thing to consider is if we provide a way to make the jsonnet-mode output in a side window, should we also provide a key binding to window-toggle-side-windows? Because i can't find one by default

Another option might be to point them towards existing libraries like https://github.com/karthink/popper. it was my entry into how to better use side-windows, because once you make a side window, there isn't really an easy way promote it, or even to dismiss it, especially since they are usually for auxiliary information, you often want to dismiss them.

What do you think about adding a section to the README about dealing with the output, stderr and stdout buffers via a example customisation of the display-buffer-alist we can also link to https://github.com/hlissner/doom-emacs/blob/develop/modules/ui/popup/README.org#set-popup-rule-and-set-popup-rules and https://github.com/emacsorphanage/popwin (used by spacemacs e.g. https://github.com/syl20bnr/spacemacs/blob/235aa8d44e1eaa23c00bd3fdd6c550be9fcab87d/layers/%2Bspacemacs/spacemacs-visual/packages.el#L105-L118) and popper. I assume that most people who are not going to bother configuring via the display-buffer-alist are probably using a distro, so targeting how to integrate into them is probably the best bet for new user friendliness

tminor commented 3 years ago

That is vastly more complex then what i have. I don't know how i managed to use Emacs for 10 years and never configured the display-buffer-alist I think i spent 5 years assuming that i didn't understand how Emacs worked and the latter 5 years thinking that there must be a good reason for the seemingly random behavior of some windows. facepalm

According to one of Emacs's change logs, display-buffer-alist was added in 2011 so I don't think you can blame 10-years-ago-you for not knowing about it! Learning about it and customizing it was certainly a huge improvement for my quality of life.

I think it would be nice to give users a customizable setting for Jsonnet's particular case, though; display-buffer-alist isn't very user friendly and I certainly wouldn't tell a new Emacs user to "just customize display-buffer-alist if you want different behavior." We could add some simple instructions in the README for such a customization. Does that seem reasonable?

I'm not totally opposed to it adding customization option to jsonnet-mode, but it will add more complexity to your mode and will not help those users when it comes to dealing with other buffers of a similar nature.

Fair point about additional complexity. It's probably worth surveying other modes and how they handle buffer display. Here are a couple that immediately came to mind:

The first example (IIUC) displays the output in the current buffer (no window splitting, etc.). You mentioned that you don't like that kind of behavior but I don't mind it (and I'm sure you could find someone else with a third opinion). The second example relies on compile and a subsequent call to compile-start to handle buffer display. In your PR submission, you proposed a simple solution and said:

it's more inline with other compile buffer behavior and it's aiming to not be opinionated

I agree that this is the best approach (for the sake of simplicity). Referring back to the second example above: compile-start does exactly what I think you originally proposed in its display-buffer call:

(display-buffer outbuf '(nil (allow-no-window . t)))

There's a comment right above that which contains a link to a thread very similar to this one.

Another thing to consider is if we provide a way to make the jsonnet-mode output in a side window, should we also provide a key binding to window-toggle-side-windows? Because i can't find one by default

No, I think we should avoid doing that sort of thing. Similar to weird buffer display behavior, I really dislike when modes unexpectedly clobber my customized key bindings.

Another option might be to point them towards existing libraries like https://github.com/karthink/popper. it was my entry into how to better use side-windows, because once you make a side window, there isn't really an easy way promote it, or even to dismiss it, especially since they are usually for auxiliary information, you often want to dismiss them.

Interesting. I don't think I've ever come across that package. I'm not opposed to adding helpful tips to whatever documentation exists in the README, but I'd like to avoid anything more than a passing reference. I assume you have a grasp of display-buffer customization, but if you've never seen it, The Zen of Buffer Display is very helpful.

What do you think about adding a section to the README about dealing with the output, stderr and stdout buffers via a example customisation of the display-buffer-alist we can also link to https://github.com/hlissner/doom-emacs/blob/develop/modules/ui/popup/README.org#set-popup-rule-and-set-popup-rules and https://github.com/emacsorphanage/popwin (used by spacemacs e.g. https://github.com/syl20bnr/spacemacs/blob/235aa8d44e1eaa23c00bd3fdd6c550be9fcab87d/layers/%2Bspacemacs/spacemacs-visual/packages.el#L105-L118) and popper. I assume that most people who are not going to bother configuring via the display-buffer-alist are probably using a distro, so targeting how to integrate into them is probably the best bet for new user friendliness

That sounds fine to me. I have to admit that I really don't know much about display-buffer best practices. It seems discussions like this one are fairly common but the outcome generally seems to be the conclusion we're gravitating towards: a simple, unopinionated approach.

russell commented 3 years ago

Fair point about additional complexity. It's probably worth surveying other modes and how they handle buffer display. Here are a couple that immediately came to mind:

* [`mermaid-mode`](https://github.com/abrochard/mermaid-mode/blob/master/mermaid-mode.el#L165)

* [`rspec-mode`](https://github.com/pezra/rspec-mode/blob/master/rspec-mode.el#L857-L859)

The first example (IIUC) displays the output in the current buffer (no window splitting, etc.). You mentioned that you don't like that kind of behavior but I don't mind it (and I'm sure you could find someone else with a third opinion). The second example relies on compile and a subsequent call to compile-start to handle buffer display. In your PR submission, you proposed a simple solution and said:

it's more inline with other compile buffer behavior and it's aiming to not be opinionated

I agree that this is the best approach (for the sake of simplicity). Referring back to the second example above: compile-start does exactly what I think you originally proposed in its display-buffer call:

(display-buffer outbuf '(nil (allow-no-window . t)))

There's a comment right above that which contains a link to a thread very similar to this one.

I really want the mode to do the bare minimum, if I see weird behaviour I'll try and customise it, but take magit for example, I actually don't know it's behaviour, I think magit-status always displays in other-window, but it's something that haven't had to think about much because there was never a need to. The more complicated the solution we provide the more likely it will clash with someone else's workflow, if we provide a simple solution, then the people with complex needs will already have a way to integrate it into their workflow.

I was originally going to submit this patch with that exact code from compile mode, but it felt a bit like cargo cult programming, i couldn't see why they were allowing the buffer to have no display in some cases. I assumed it was related wanting to have compile-mode buffers stay in the background, but it might also have been to help with that grep case. The previous implementation if you use git blame on the code base, is (display-buffer outbuf) so in the first instance they had no defined behaviour. I'd be happy to go with the compile mode one, but i think other window is probably more what i would expect form a preview buffer.

I also looked at a few modes and (display-buffer outbuf) is pretty common. I did a search here https://github.com/search?p=1&q=org%3Aemacsorphanage+display-buffer&type=Code and I did a github wide search. I think that not specifying any behaviour is common. The only other mode i looked at was markdown-mode they have a preview other window function and a way to specify how the screen is split, but they always force the buffer into other-window.

Interesting. I don't think I've ever come across that package. I'm not opposed to adding helpful tips to whatever documentation exists in the README, but I'd like to avoid anything more than a passing reference. I assume you have a grasp of display-buffer customization, but if you've never seen it, The Zen of Buffer Display is very helpful.

I actually took the buffer config for this PR from the top of that page, but i haven't read the page fully, I do plan on going back to it, but TBH i don't really know what i want out of buffer display at the moment. I've just started using side-windows, and i really like/dislike them, i first put shells in side windows, but found them really hard to work with, i probably should start using winner mode and just enjoy the seemingly random nature of window arrangement.

What do you think about adding a section to the README about dealing with the output, stderr and stdout buffers via a example customisation of the display-buffer-alist we can also link to https://github.com/hlissner/doom-emacs/blob/develop/modules/ui/popup/README.org#set-popup-rule-and-set-popup-rules and https://github.com/emacsorphanage/popwin (used by spacemacs e.g. https://github.com/syl20bnr/spacemacs/blob/235aa8d44e1eaa23c00bd3fdd6c550be9fcab87d/layers/%2Bspacemacs/spacemacs-visual/packages.el#L105-L118) and popper. I assume that most people who are not going to bother configuring via the display-buffer-alist are probably using a distro, so targeting how to integrate into them is probably the best bet for new user friendliness

That sounds fine to me. I have to admit that I really don't know much about display-buffer best practices. It seems discussions like this one are fairly common but the outcome generally seems to be the conclusion we're gravitating towards: a simple, opinionated approach.

Perhaps something simple like this for the readme.

# Output Buffers

Jsonnet-mode has several output buffers that can be created depending on what command was issued.

* `*jsonnet output*`
* `*jsonnetfmt stderr*`
* `*jsonnetfmt stdout*`

The default display behaviour of these buffers the can be changed using `display-buffer-alist` see [The Zen of Buffer Display](https://www.gnu.org/software/emacs/manual/html_node/elisp/The-Zen-of-Buffer-Display.html) for examples of how to do this.

I see main reason to put something in the readme is to deflect bugs, by providing a way for users to learn more about buffer configuration. If you like i can mention something about other minor-modes that can help, but that will probably go out of date.

The discussion on https://github.com/pezra/rspec-mode/pull/79 is exactly what I would hope to avoid by adding something to the readme. There are so many ways to integrate a mode into your personal workflow, what we want is something that can be extended, not something that is ridged and complex.

I'm 100% for a simple opinionated approach

russell commented 3 years ago

Wow, those original documentation commits i made were trash, sorry about that. I merged the commits and rebased it in case you want to merge as opposed to squash/rebase

tminor commented 3 years ago

No worries! Thanks for the PR.