Skip to content

fix(ediff): restore chat side-window after ediff session - #41

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

@ericdallo please try this fix.

@ericdallo

Copy link
Copy Markdown
Member

@CsBigDataHub Unfortunatelly same behavior, the eca-chat becomes the only displayed window for some reason, differently than via smerge

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

@CsBigDataHub Unfortunatelly same behavior, the eca-chat becomes the only displayed window for some reason, differently than via smerge

Maybe something to do with how doom emacs manage the windows? I cannot replicate it with my configs.

@ericdallo

Copy link
Copy Markdown
Member

Yeah maybe, I have no idea tho

@ChetanKoneru

ChetanKoneru commented Sep 12, 2025 •

Copy link
Copy Markdown
Contributor Author

Doom's popup system (+popup-buffer-mode) interferes with standard set-window-configuration by automatically repositioning buffers according to its internal rules.

https://github.com/doomemacs/doomemacs/tree/master/modules/ui/popup

enhance ediff integration to detect and adapt to Doom Emacs popup contexts
prevent eca-diff and Ediff control buffers from being managed by Doom popups
avoid saving/restoring window configuration when in Doom popup to prevent conflicts
add doom-aware cleanup and error recovery, using Doom's popup restoration when needed
use longer delay for window restoration under Doom for reliability
ensure chat buffer is re-displayed correctly after ediff session or error
refactor buffer and window management logic for improved compatibility and robustness
@ChetanKoneru

ChetanKoneru commented Sep 12, 2025 •

Copy link
Copy Markdown
Contributor Author

@ericdallo Please test now, added support to doom popup rules.
I do not use doom-emacs so I cannot test. everything works fine in my config.

Comment thread eca-chat.el Outdated
@ericdallo

Copy link
Copy Markdown
Member

@CsBigDataHub yeah, that works!

I'm just wondering the complexity we added for that, especially with the previous PR, I'm wondering if we should have a eca-diff file or something to move all those complex logic, WDYT?

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

Yes and no.

For code maintainability and elegance - yes.

No, as I do not anticipate many improvements to this code.

I vote for YES, please do not merge this. let me decouple and push.

@ChetanKoneru

Copy link
Copy Markdown
Contributor Author

@ericdallo please test, decoupled diff from eco-chat.el.

Works well on my end.

Comment thread eca-chat.el Outdated
Comment thread eca-chat.el Outdated
Comment thread eca-chat.el Outdated
Comment thread eca-diff.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.

@CsBigDataHub I tested and it works, looks good but added some comments in code

Comment thread eca-diff.el Outdated
Comment thread eca-chat.el
Comment on lines +1100 to +1122
(defun eca-chat--parse-unified-diff (diff-text)
"Compatibility wrapper that delegates to `eca-diff-parse-unified-diff'.

DIFF-TEXT is the unified diff string to parse and returns the parsed
plist produced by `eca-diff-parse-unified-diff'."
(eca-diff-parse-unified-diff diff-text))

(defun eca-chat--show-diff-ediff (path diff)
"Compatibility wrapper delegating to `eca-diff-show-ediff'.

PATH is the file path being shown and DIFF is the unified diff text.
This wrapper passes the current buffer as CHAT-BUF so `eca-diff' can
restore the chat display after Ediff quits."
(eca-diff-show-ediff path diff (current-buffer) (lambda (b) (ignore-errors (eca-chat--display-buffer b)))))


(defun eca-chat--show-diff-smerge (path diff)
"Compatibility wrapper delegating to `eca-diff-show-smerge'.

PATH is the file path being shown and DIFF is the unified diff text.
This wrapper passes the current buffer as CHAT-BUF so `eca-diff' can
restore the chat display after smerge quits."
(eca-diff-show-smerge path diff (current-buffer) (lambda (b) (ignore-errors (eca-chat--display-buffer b)))))

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.

Do we need these 3 functions? can't we just do what they do inside the pcase below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just want to keep it clean and maintainable.

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.

I see low level and more indirection having like that, but no big deal too, I'm ok keeping it

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.

Thank you!!

@ericdallo
ericdallo merged commit aaaf67b into editor-code-assistant:master Sep 12, 2025
12 checks passed
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.

2 participants