Skip to content

Support ediff and smerge modes to view diffs - #39

Merged
ericdallo merged 10 commits into
editor-code-assistant:masterfrom
ChetanKoneru:master
Sep 12, 2025
Merged

ericdallo merged 10 commits into
editor-code-assistant:masterfrom
ChetanKoneru:master

Conversation

@ChetanKoneru

Copy link
Copy Markdown
Contributor
image
  • Put together a hack to cleanup buffers ediff creates, else there are going to be dangling buffers like so
image

Note: I have in-line comments for the review, will delete it before merging.

Should fix #13

ChetanAtGNU and others added 6 commits September 7, 2025 20:05
Add support for displaying file diffs using ediff, smerge, or text. Introduce a
custom variable to select the diff tool. Refactor content handling code to use
the new diff functions and improve tool call and progress handling.Add diff view
options to ECA chat

Added new function `eca-chat-diff-tool` to select display method for file-change
diffs.
Implemented three different display methods:
• Ediff: Interactive side-by-side comparison.
• Smerge: Merge-style conflict presentation with Smerge mode.
• Text: Plain text unified diff in a separate buffer.

Modified `eca-chat--show-diff` function to dispatch the selected view based on
`eca-chat-diff-tool`.
Updated tool call content display for fileChange tools, including new diff view
options.
@ericdallo

Copy link
Copy Markdown
Member

Looks interesting! will review later today

@ChetanKoneru

ChetanKoneru commented Sep 10, 2025 •

Copy link
Copy Markdown
Contributor Author

Looks interesting! will review later today

Cool, in both ediff and smerge cases we are creating a new frame and deleting frame(and diff buffers) after we quit q the diff. I thought this is a cleaner approach rather than using the same frame we are working on.

BTW the frame always opens full screen, I prefer it and I think users would also prefer viewing side-by-side diff in a full screen frame.

Comment thread eca-chat.el

@ericdallo ericdallo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CsBigDataHub I tested and that looks really nice!

But can we please do not use frames for this feature? I understand your rationale but I don't think that's a good way, I never saw a emacs package open frames for anytihng, and there are so many cases to handle when going with that approach.
I'm pretty confortable in opening a new window or just the diff buffer like gptel.el, buffers are way easier for people to manage with their own rules if needed.

I will take a closer look to code then

@ericdallo

Copy link
Copy Markdown
Member

I did some improvement to show buttons on the bottom instead of right too, sorry for the conflicts, can you solve it please?
Also, you don't need to close the PR like before, you can do the changes in this branch/PR

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

@ericdallo fixed merge conflicts and creating windows in the same frame rather than creating new frames. There are in-line comments and function docs for your reference, I will clean these up when we are ready to merge.

@ericdallo

Copy link
Copy Markdown
Member

Thanks @CsBigDataHub, will test in a couple of mins

Comment thread eca-chat.el Outdated
Comment thread eca-chat.el Outdated

@ericdallo ericdallo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thank you a lot!

ediff still behaves weird to me, when quitting ediff, I lost the buffer I was and eca-chat beomes the main buffer which is weird, smerge tho behaves perfectly, typeing ESC quit the buffer and goes back to old behavior.
Will merge but let me know if you know how to improve that for ediff

@ericdallo
ericdallo merged commit 9fc90f0 into editor-code-assistant:master Sep 12, 2025
@ericdallo

Copy link
Copy Markdown
Member

I noticed we lost the changes I did to actions who in the next line, will check that

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

That's great, thank you a lot!

ediff still behaves weird to me, when quitting ediff, I lost the buffer I was and eca-chat beomes the main buffer which is weird, smerge tho behaves perfectly, typeing ESC quit the buffer and goes back to old behavior. Will merge but let me know if you know how to improve that for ediff

Huh, I tested it and did not notice the issue, I am capturing window configuration and restoring once user quit ediff or smerge.

Also I am binding q to quit both modes. Let me test again.

Should be a easy fix.

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

I noticed we lost the changes I did to actions who in the next line, will check that

Can you please let me know where? I mean which function.

@ericdallo

Copy link
Copy Markdown
Member

@CsBigDataHub Just pushed the fix, don't mind it!
2025-09-11_22-50

The ediff one would be nice to improve, what happens is:

  1. I have my eca-chat opened like that:
2025-09-11_22-53
  1. I click on view_diff and see the ediff as a whole window, so far so good
2025-09-11_22-54
  1. I type q and confirm, exists ediff, but now my chat is whole fullscreen, and I lost old buffer:
2025-09-11_22-55

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

oh, let me see if there is race condition introduced. Will test and push a fix tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve file change diffs to use ediff or a better diff

3 participants