Skip to content

fix(view): preserve cycled layout across force_entry_refresh_on_noop - #315

Open
dlyongemallo wants to merge 1 commit into
mainfrom
312-layout_selection
Open

fix(view): preserve cycled layout across force_entry_refresh_on_noop#315
dlyongemallo wants to merge 1 commit into
mainfrom
312-layout_selection

Conversation

@dlyongemallo

Copy link
Copy Markdown
Owner

Fixes #312.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is a minimal, correctly-guarded fix that mirrors the established convert_layout pattern (e.g. file_history_view.lua:405-408), covers all file collections, and is backed by a targeted regression test with valid API usage.

Pull request overview

This PR fixes issue #312, where the diff layout chosen via cycle_layout/set_layout (e.g. g<C-x>) was silently reset to the config default whenever a refresh replaced NOOP entries. This happens on adapters where force_entry_refresh_on_noop returns true — notably the jj adapter on any LOCAL-touching range, which fires on tab_enter, FocusGained, polling, or an explicit R. Because get_updated_files always rebuilds entries with the config default layout, the fix carries the old entry's layout class onto the freshly built entry before the swap, using the existing FileEntry:convert_layout mechanism.

Changes:

  • In the NOOP replace-entry branch of update_files_impl, convert the new entry to the old entry's layout class when they differ, preserving the user's cycled/set layout across refreshes.
  • Add a regression test simulating the force_entry_refresh_on_noop replace path and asserting the recreated entry keeps the user's chosen layout class.
File summaries
File Description
lua/diffview/scene/views/diff/diff_view.lua Preserves the old entry's layout class on the replacement entry in the NOOP force-refresh branch.
lua/diffview/tests/functional/diff_view_spec.lua Adds a regression test verifying the layout class survives the entry swap.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jakubbortlik

Copy link
Copy Markdown

Hi David, thanks for addressing this. When I was trying to figure out what's wrong, Claude Code suggested that maybe the JjAdapter:force_entry_refresh_on_noop is obsolete and should be removed. Supposedly, "the staleness it guards against is now handled by
JjAdapter:on_local_buffer_reused (jj/init.lua:690-700)" - but I don't know enough about diffview's codebase to judge if this is indeed the case.

One thing I can confirm though is that removing the JjAdapter:force_entry_refresh_on_noop override removes one more bug: When on the 312-layout_selection branch and when inspecting a file I do :DiffviewOpen main the diffiview now correctly retains the layout, but the cursor position is lost and more over, switching back and forth to the original tab from which I opened the diffview, the original tab is closed. When I simply remove JjAdapter:force_entry_refresh_on_noop or make it return false, both issues are gone (not sure though, if that introduces other issues, but I haven't noticed any).

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.

[Bug] layout selection is not persistent when using JJ adapter

3 participants