Skip to content

Fix editor viewport placement and temporary recentf entries#447

Merged
tninja merged 1 commit into
tninja:mainfrom
cxa:fix/editor-viewport-window-placement
Jul 21, 2026
Merged

Fix editor viewport placement and temporary recentf entries#447
tninja merged 1 commit into
tninja:mainfrom
cxa:fix/editor-viewport-window-placement

Conversation

@cxa

@cxa cxa commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Use a deterministic editor viewport fallback order: below, right, left, then replace.
  • Keep viewport placement on the frame selected when the request begins and prevent frame-producing display actions.
  • Target a 12-line viewport below sources with at least 24 lines, accepting a minimum viewport height of 8 lines.
  • Use balanced side placement only when both the source and viewport remain at least 24 columns wide.
  • Split side-window sessions into normal windows to remain valid after window-layout-transpose.
  • Exclude temporary external-editor staging files from recentf without changing user configuration.

Motivation

Editor viewport requests could fail when below placement was unavailable, unexpectedly use another frame, or produce invalid same-side window trees after layout transposition. Temporary external-editor files could also appear in recentf.

This change makes placement deterministic, preserves usable source dimensions, rolls back failed layout attempts, and safely restores the original session after the viewport closes.

Verification

  • 54 editor viewport ERT tests pass.
  • Full suite: 1204 passed, 13 existing skips, 0 failures.
  • Repository byte compilation succeeds with no new warnings in touched files.
  • Checkdoc and git diff --check pass.
  • Tests cover all four side-window positions, layout transposition, restoration, dimension boundaries, selected-frame isolation, no-frame behavior, cancel/save flows, and recentf exclusion.

@cxa
cxa force-pushed the fix/editor-viewport-window-placement branch 4 times, most recently from 2303d59 to 1eb2264 Compare July 20, 2026 08:33

@tninja tninja left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for the thoughtful contribution and the thorough viewport coverage. I left three non-blocking suggestions for a few edge cases.

Comment thread ai-code-editor-viewport.el
Comment thread ai-code-editor-viewport.el Outdated
Comment thread ai-code-editor-viewport.el Outdated
@cxa
cxa force-pushed the fix/editor-viewport-window-placement branch from 1eb2264 to c1787ad Compare July 20, 2026 16:07
Keep editor viewports on the frame selected when placement starts and use a deterministic fallback order: below, right, left, then replace. Attempt below placement only when the source has at least 24 lines, target a 12-line viewport, accept an 8-line minimum, and accept side placement only when both the source and viewport remain at least 24 columns wide.

Split side-window sessions directly into normal viewport windows so all four side positions follow the same fallback policy without creating same-side siblings. Roll back failed attempts, preserve restoration after layout transposition, and keep frame-producing display actions out of the controlled chain.

Exclude external-editor staging files from recentf without changing user configuration. Cover dimension boundaries, normal and side-window geometry, fallback ordering, selected-frame isolation, no-frame behavior, layout transposition, restoration, cancel/save flows, and recentf bindings. Keep the transpose checks compatible with the minimum supported Emacs by exercising window-x only when it is available.
@cxa
cxa force-pushed the fix/editor-viewport-window-placement branch from c1787ad to ffac959 Compare July 21, 2026 00:12

@tninja tninja left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for the contribution and the thoughtful follow-up fixes. Looks good to me.

@tninja
tninja merged commit 43d721c into tninja:main Jul 21, 2026
2 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