karthink / gptel

A simple LLM client for Emacs
GNU General Public License v3.0
1.21k stars 122 forks source link

Line breaks #12

Closed minad closed 8 months ago

minad commented 1 year ago

Another small issue is that gptel inserts the responses as long single lines. Do you use this package with visual-fill-column or would it make sense to reformat the answer with manual line breaks? Maybe this could break some formatting of code blocks or equations?

karthink commented 1 year ago

The response is inserted as it is received. Markdown and text buffers should both have visual-line-mode turned on my gptel. Doesn't that solve the issue?

Manual line breaks will break both formatting (sometimes) and window resizing, won't it?

minad commented 1 year ago

The response is inserted as it is received. Markdown and text buffers should both have visual-line-mode turned on my gptel. Doesn't that solve the issue?

Sure, but I sometimes want the lines to be shorter than the window if my window is large. I guess I should use visual-fill-column mode?

karthink commented 1 year ago

Yes, I'm using visual-fill-column-mode or olivetti-mode for that. I don't think it makes sense to add manual line breaks here.

minad commented 1 year ago

Yes, I'm using visual-fill-column-mode or olivetti-mode for that. I don't think it makes sense to add manual line breaks here.

I use neither of those modes currently. I prefer manual line breaks, because of diffs if I save the files and also because of generally better behavior of short lines in Emacs. Maybe we could introduce some configurable filter machinery for the inserted text, which we would also use for #8.

karthink commented 1 year ago

Maybe we could introduce some configurable filter machinery for the inserted text, which we would also use for https://github.com/karthink/gptel/issues/8.

About this, I looked for a filter-chain function that threads its inputs through hook functions, something like this:

(defun gptel--get-response (content-str)
  (let ((filtered-str content-str))
    (dolist (filter-func gptel-response-filter-functions filtered-str)
      (condition-case nil
          (setq filtered-str
                (funcall filter-func filtered-str))
        (error nil)))))

Here gptel-response-filter-functions is an abnormal hook, and each hook function transforms its input. However, Emacs provides no thread-hooks function to do this. The above dolist gets complicated if we need to handle buffer-local hooks, etc. I have a draft of this but I'm currently implementing the markdown->org conversion directly instead of using gptel-response-filter-functions. Do you have any suggestions?

minad commented 1 year ago

Sounds good. I would just accept the complexity of the addition. It seems worth it. You can probably use run-hook-wrapped which should be equivalent to the thread-hooks you are asking for.

karthink commented 1 year ago

I implemented gptel-response-filter-functions when adding Org support, but I think filtering the response string through a chain of functions is not idiomatic or clean. run-hook-wrapped does not thread the results, it runs all the hooks sequentially for side effects like run-hooks.

Currently each filter function receives a string and the interaction buffer as arguments and should return the transformed string. What do you think about changing it to an abnormal hook that's called in the buffer with the begin and end points of the response as arguments? Each hook function is free to do whatever transformation it wants to the buffer, and this can be run with the usual run-hooks. Buffer-local hooks are automatically handled too.

minad commented 1 year ago

run-hook-wrapped does not thread the results, it runs all the hooks sequentially for side effects like run-hooks.

Yes, it doesn't but you can use a local variable as accumulator and implement thread-hooks this way. I only mentioned run-hook-wrapped since it essentially implements a map over the hook variable and handles local hooks and the t value correctly.

What do you think about changing it to an abnormal hook that's called in the buffer with the begin and end points of the response as arguments? Each hook function is free to do whatever transformation it wants to the buffer, and this can be run with the usual run-hooks. Buffer-local hooks are automatically handled too.

In general I prefer pure functions (String -> String), but that's also okay. You should use markers such that changing the bounds will not lead to problems.

karthink commented 1 year ago

Yes, it doesn't but you can use a local variable as accumulator and implement thread-hooks this way. I only mentioned run-hook-wrapped since it essentially implements a map over the hook variable and handles local hooks and the t value correctly.

Ah, I see. That's a better idea than what I have going, I'll use this approach.

In general I prefer pure functions (String -> String), but that's also okay. You should use markers such that changing the bounds will not lead to problems.

I don't mind imperative code here, but I think I'll hold off on switching to a regular hook for now -- if we write the process filter for continuous updates this will get quite complicated. Pure functions can still be applied in the process filter with minimal adjustments to handle incomplete parse states.

karthink commented 8 months ago

Closing this since gptel-post-response-hook and gptel-post-stream-hook have been added to help the user do any further processing they'd like.