xenodium / chatgpt-shell

ChatGPT and DALL-E Emacs shells + Org babel 🦄 + a shell maker for other providers
https://xenodium.com
GNU General Public License v3.0
827 stars 74 forks source link

option to re-use window for showing results of chatgpt-shell-prompt #208

Closed solodov closed 4 months ago

solodov commented 5 months ago

When chatgpt-shell-prompt-query-response-style is set to other-buffer every prompt result is shown in a new window with a transient buffer in it. An option to reuse an existing window for the buffer would be great. This would allow to asynchronously send prompts without switching away from the main task and also not have emacs frame full of response windows.

I tried using display-buffer-alist for this:

(add-to-list 'display-buffer-alist
             `(,(rx bol (| "*chatgpt* " (+ nonl) "> " (+ nonl))) .
               (display-buffer-reuse-window display-buffer-use-least-recent-window)))

This only works if prompt is identical, ie. results in the same buffer name as another transient response buffer already shown. A new prompt still produces new window.

xenodium commented 4 months ago

Hi @solodov, thanks for #209 PR!

Trying things out... If I'm understanding correctly, you're using this to "send to shell" (like *chatgpt* 4o/General) and also "send to compose" (like *chatgpt* 4o/General> compose) buffers?

Would you mind capturing a screen grab of your preferred flow? Wanna make sure I'm seeing the behabiour you expect with the patch, but I also want avoid regressing other flows.

On a related note, I realized (retrospectively) that my existing code is being somewhat naughty by modifying display-buffer-alist on behalf of the user. I'd like to revisit that in the future and redesign a bit... While not directly applicable now, just a heads-up that things around buffer-handling may change in the future.

Btw, I'm thinking we can likely do without adding chatgpt-shell-prompt-reuse-transient-buffer-window and introduce a other-buffer-reuse-window style (or some other name, suggestions?) to chatgpt-shell-prompt-query-response-style. This way, we can prevent user misconfiguration if they forget to read the docstring stating This variable only has effect when chatgpt-shell-prompt-query-response-style is set to other-window.

If you want to skip theother-buffer-reuse-window change, I'm happy to follow up.

solodov commented 4 months ago

Here's what this looks like. I'm calling chatgpt-shell-prompt. First half is with this setting turned off (current behaviour), second half with it turned on:

https://github.com/xenodium/chatgpt-shell/assets/187738/57241da5-5da7-4dcc-aeae-1de39dd9499b

I first considered using other-buffer-reuse-window style. I decided to go with the flag because other-buffer style is used not just for chatgpt-shell-prompt, but also in other places where style gets overridden, like chatgpt-shell-eshell-whats-wrong-with-last-command and chatgpt-shell-eshell-summarize-last-command-output. The problem with introducing new style is potential user confusion where chatgpt-shell-prompt does one thing, but these functions do something different because of the style override. Either way, introducing new option does carry a risk of some confusion, but this approach appears less problematic to me.

xenodium commented 4 months ago

Here's what this looks like. I'm calling chatgpt-shell-prompt. First half is with this setting turned off (current behaviour), second half with it turned on

This is helpful thank you.

Lemme play with the PR a little. I think we need this solution in quite a few areas. For example, chatgpt-shell-prompt-compose should get this benefit also.

xenodium commented 4 months ago

Lemme play with the PR a little.

I've been playing with the proposed change without guarding with chatgpt-shell-prompt-reuse-transient-buffer-window and it's been working great. Mind removing chatgpt-shell-prompt-reuse-transient-buffer-window and keep everything else?

For example, chatgpt-shell-prompt-compose should get this benefit also.

This would need patching elsewhere, but nevermind making this change now. I can follow up.

solodov commented 4 months ago

I played with this a bit more because I wanted to fix other window management issues. Now I don't think this is necessarily the right approach here. One case is where this fix breaks is when a chatgpt transient buffer is shown in another frame.

I was able to achieve the desired behaviour with the following hack that implements a custom display buffer function. As far as I'm aware there's no built-in way to reuse a window showing a buffer with a matching name, it's either reuse mode window or reuse window showing buffer already.

(defvar chatgpt-buffer-name-regexp (rx (| (group "*chatgpt* " (+ nonl) "> " (+ nonl)) (group "ChatGPT> " (+ nonl)))))

(setq display-buffer-alist
      `((,chatgpt-buffer-name-regexp
         (display-buffer-reuse-named-window display-buffer-reuse-window display-buffer-below-selected)
         (reusable-frames . visible)
         (inhibit-switch-frame . t)
         (buffer-name-regexp . ,chatgpt-buffer-name-regexp))))

(defun display-buffer-reuse-named-window (buffer alist)
  (when-let* ((buffer-name-regexp (cdr (assq 'buffer-name-regexp alist)))
              (window (get-window-with-predicate
                       (lambda (w)
                         (and (string-match buffer-name-regexp (buffer-name (window-buffer w)))
                              (window-live-p w)))
                       'nomini 'visible)))
    (prog1 (window--display-buffer buffer window 'reuse alist)
      (unless (cdr (assq 'inhibit-switch-frame alist))
        (window--maybe-raise-frame (window-frame window))))))

This does require that chatgpt-shell stops mucking about with display-buffer-alist, but we already agree on that.

Perhaps the right fix here is:

xenodium commented 4 months ago

Thanks for this!

One case is where this fix breaks is when a chatgpt transient buffer is shown in another frame.

Ah, I hadn't played with reusing window from other frame. The original snippet we were trying out didn't reuse the window if it was displayed in a different frame.

Below is somewhat similar approach to the original proposal. Could you give that a try and see?

    (if-let* ((buffer-name-regex (rx (| (group "*chatgpt* " (+ nonl) "> " (+ nonl)) (group "ChatGPT> " (+ nonl)))))
              (window (catch 'found
                        (walk-windows (lambda (w)
                                        (when (string-match buffer-name-regex
                                                            (buffer-name (window-buffer w)))
                                          (throw 'found w)))
                                      nil t))))
    (progn
      (set-window-buffer window (chatgpt-shell-prompt-compose-buffer))
      (select-frame-set-input-focus (window-frame window)))
  (pop-to-buffer (chatgpt-shell-prompt-compose-buffer)))

remove code that changes display-buffer-alist

Need to think more about this one, but yeah I'd also like to move away from this. If the above works for you, we can adopt this in the short term.

xenodium commented 4 months ago

remove code that changes display-buffer-alist

Ok, I've done it!

I consolidated chatgpt-shell-prompt buffer handling with chatgpt-shell-prompt-compose, which should make chatgpt-shell-prompt buffers more feature-full.

In terms of being able to reuse windows, we can now use the more idiomatic display-buffer-alist. For example:

(add-to-list 'display-buffer-alist
             (cons '(major-mode . chatgpt-shell-prompt-compose-mode)
                   '((display-buffer-reuse-window
                      display-buffer-in-direction
                      display-buffer-select)
                     (reusable-frames . visible)
                     (direction . left)
                     (window-width . 0.35))))

Gonna close this issue, as display-buffer-alist should now be able to handle the initial intent. Please reopen if otherwise.