gptel: Improve handling of read only buffers

[pabl0]

• gptel.el (gptel-read-only-response-buffer, gptel--handle-pre-insert): Introduce a new defcustom and don't hard code "LLM response".

• gptel-transient.el (gptel--describe-suffix-send): Inform the user when the source buffer is read-only and clarify that output will be directed to the buffer specified in gptel-read-only-response-buffer. Additionally, remove misleading claims about killing content from a read-only source buffer in "respond in place" mode.

---

Not sure if the defcustom is too much, but perhaps it is in the spirit of Emacs to maximal customizability?
Also I think "read-only" might be more correct than "read only" but then again English is not my first language?

[pabl0]

(Ah, added a check to actually not attempt to kill-region if read-only.)

BTW, how the transient menu should behave in case the user selects a read-only buffer as a destination?

[karthink]

Coincidentally, I pushed a read-only behavior change a short while before I saw this PR. There will be a merge conflict.

• gptel.el (gptel-read-only-response-buffer, gptel--handle-pre-insert): Introduce a new defcustom and don't hard code "LLM response".

I don't like this solution. The LLM response buffer was just a workaround, a clumsy approach in search of a better one. Making it a defcustom enshrines it and makes it difficult to change in the future.

I think we should take a couple of steps back and think about what a good solution would be here.

Not sure if the defcustom is too much, but perhaps it is in the spirit of Emacs to maximal customizability?

As stated, I think this problem is in need of a better solution.

But even more generally, I don't agree with the idea that everything should be customizable as a user option. Everybody has a worse time when you do this across a package:

• The author pays a cost because this locks in the design, making it harder to change without breaking the user's configuration.
• The user pays a cognitive cost because the namespace is polluted, making it difficult to find more important settings.
• In turn this is a support cost to be borne by the community or package authors.

Org mode is a good example of what happens when everything is made infinitely customizable at the user and document level. The code base slowly became a nightmare, held together with strings and a prayer, and it becomes difficult to find what you're looking for. The number of man hours it takes to support niche, decades old options in Org that fewer than fifty people in the world still use is scary.

Emacs remains customizable via hooks and advice. A motivated user can modify the code however they like, especially with advice, and with the understanding that if something breaks it's their problem.

Also I think "read-only" might be more correct than "read only" but then again English is not my first language?

Yes, read-only is more correct.

BTW, how the transient menu should behave in case the user selects a read-only buffer as a destination?

Is there something wrong with its current behavior?

[pabl0]

Coincidentally, I pushed a read-only behavior change a short while before I saw this PR. There will be a merge conflict.

Noted, will rebase.

• gptel.el (gptel-read-only-response-buffer, gptel--handle-pre-insert): Introduce a new defcustom and don't hard code "LLM response".

I don't like this solution. The LLM response buffer was just a workaround, a clumsy approach in search of a better one. Making it a defcustom enshrines it and makes it difficult to change in the future.

Ok, agreed. Should it still be a defvar instead of hardcoding in multiple places?

BTW, how the transient menu should behave in case the user selects a read-only buffer as a destination?

Is there something wrong with its current behavior?

Well not really, it does the right thing and outputs to the LLM response buffer instead. I was just thinking should the interface give some warning already at the stage when the user selects an r/o buffer, before proceeding to send the actual query. Maybe I am overthinking this a bit and it's fine as-is.

Other than this, is this PR something you'd be willing to merge after dropping the defcustom and solving the conflict?

[karthink]

This needs a little more work.

1. If the destination is not the current buffer (echo area, kill ring or another buffer) don't bother checking or indicating that the source is read-only. This information would be irrelevant.
2. If the destination is the current buffer, check for buffer-read-only and for (get-char-property (point) 'read-only). If either one is true, set dest to the fallback buffer.

[pabl0]

2. If the destination is the current buffer, check for `buffer-read-only` and for `(get-char-property (point) 'read-only)`.  If either one is true, set `dest` to the fallback buffer.

Hah, turned out to be much more nuanced than that! See the newest version. I didn't really change anything in gptel--suffix-send, just the UI part. Tested this a lot with different use cases like read-only snippets, with and without front-sticky, etc, but I am sure there is some corner case that I did not take into account.

It seems to me BTW that the i-mode does not really work as expected: it says it's replacing lines 1-(last line), but actually replaces up to the last assistant response.

[karthink]

Other than this, is this PR something you'd be willing to merge after dropping the defcustom and solving the conflict?

Yeah, it's fine otherwise. 👍

[karthink]

Thanks, will take a look.

It seems to me BTW that the i-mode does not really work as expected: it says it's replacing lines 1-(last line), but actually replaces up to the last assistant response.

Yes, I am aware of this 👍

[karthink]

I took another look at this and wow, gptel--describe-suffix-send has gotten really complicated. To the point where it's difficult to make any changes with confidence, as there are so many intertwined branches. I'll have to come back to this after tagging the new release.