diff --git a/lua/diffview/config.lua b/lua/diffview/config.lua index d78bb76c..19fb0a11 100644 --- a/lua/diffview/config.lua +++ b/lua/diffview/config.lua @@ -7,6 +7,9 @@ local lazy = require("diffview.lazy") local Diff1 = lazy.access("diffview.scene.layouts.diff_1", "Diff1") ---@type Diff1|LazyModule local Diff1Inline = lazy.access("diffview.scene.layouts.diff_1_inline", "Diff1Inline") ---@type Diff1Inline|LazyModule +local Diff1InlinePinned = + lazy.access("diffview.scene.layouts.diff_1_inline_pinned", "Diff1InlinePinned") ---@type Diff1InlinePinned|LazyModule +local Diff1Pinned = lazy.access("diffview.scene.layouts.diff_1_pinned", "Diff1Pinned") ---@type Diff1Pinned|LazyModule local Diff2 = lazy.access("diffview.scene.layouts.diff_2", "Diff2") ---@type Diff2|LazyModule local Diff2Hor = lazy.access("diffview.scene.layouts.diff_2_hor", "Diff2Hor") ---@type Diff2Hor|LazyModule local Diff2HorPinned = lazy.access("diffview.scene.layouts.diff_2_hor_pinned", "Diff2HorPinned") ---@type Diff2HorPinned|LazyModule @@ -34,11 +37,11 @@ function M.diffview_callback(cb_name) end -- Layout aliases used across multiple view kinds and cycle_layouts. The --- pinned `Diff2` variants (`diff2_*_pinned`) intentionally aren't listed --- here: they're internal to file-history `pin_local` mode and are --- selected by the view based on whether `--pin-local` is active, not by --- direct user configuration. The `LayoutName` alias below still includes --- them so `name_to_layout` can resolve them internally. +-- pinned variants (`diff1_*_pinned`, `diff2_*_pinned`) intentionally +-- aren't listed here: they're internal to file-history `pin_local` mode +-- and are selected by the view based on whether `--pin-local` is active, +-- not by direct user configuration. The `LayoutName` alias below still +-- includes them so `name_to_layout` can resolve them internally. ---@alias DiffviewStandardLayout "diff1_plain"|"diff1_inline"|"diff2_horizontal"|"diff2_vertical" ---@alias DiffviewMergeLayout "diff1_plain"|"diff3_horizontal"|"diff3_vertical"|"diff3_mixed"|"diff4_mixed" ---@alias DiffviewInferredLayout -1 @@ -879,7 +882,9 @@ function M.get_log_options(single_file, t, vcs) end ---@alias LayoutName "diff1_plain" +--- | "diff1_plain_pinned" --- | "diff1_inline" +--- | "diff1_inline_pinned" --- | "diff2_horizontal" --- | "diff2_horizontal_pinned" --- | "diff2_vertical" @@ -891,7 +896,9 @@ end local layout_map = { diff1_plain = Diff1, + diff1_plain_pinned = Diff1Pinned, diff1_inline = Diff1Inline, + diff1_inline_pinned = Diff1InlinePinned, diff2_horizontal = Diff2Hor, diff2_horizontal_pinned = Diff2HorPinned, diff2_vertical = Diff2Ver, diff --git a/lua/diffview/scene/layouts/diff_1_inline.lua b/lua/diffview/scene/layouts/diff_1_inline.lua index a4f25389..de2a187e 100644 --- a/lua/diffview/scene/layouts/diff_1_inline.lua +++ b/lua/diffview/scene/layouts/diff_1_inline.lua @@ -443,10 +443,12 @@ end ---@override ---Diff1Inline owns `a_file` even though it isn't attached to a window, so ---expose it through `owned_files()` so `FileEntry:destroy()` can tear it ----down alongside the windowed files. +---down alongside the windowed files. Defer to `Layout:owned_files` for the +---windowed slots so `shared_symbols` (e.g. `Diff1InlinePinned`'s borrowed +---b-side) is honoured in subclasses. ---@return vcs.File[] function Diff1Inline:owned_files() - local out = Layout.files(self) + local out = Layout.owned_files(self) if self.a_file and not vim.tbl_contains(out, self.a_file) then out[#out + 1] = self.a_file end diff --git a/lua/diffview/scene/layouts/diff_1_inline_pinned.lua b/lua/diffview/scene/layouts/diff_1_inline_pinned.lua new file mode 100644 index 00000000..2e9d0b5f --- /dev/null +++ b/lua/diffview/scene/layouts/diff_1_inline_pinned.lua @@ -0,0 +1,28 @@ +local Diff1Inline = require("diffview.scene.layouts.diff_1_inline").Diff1Inline +local Diff1Pinned = require("diffview.scene.layouts.diff_1_pinned").Diff1Pinned +local oop = require("diffview.oop") + +local M = {} + +---@class Diff1InlinePinned : Diff1Inline +---Inline variant of `Diff1Pinned`: renders the diff inline against the +---view's shared working-tree b-file. The b-side `vcs.File` is owned by the +---FileHistoryView (its pin_local cache); `shared_symbols = { "b" }` keeps +---`Layout:owned_files()` from returning it, so `FileEntry:destroy` won't +---tear it down. The view destroys the cached b-file once in `close()`. +---Inherits the rest of `Diff1Inline`'s rendering and `Diff1Pinned`'s +---`detach_files_for_swap` semantics. +local Diff1InlinePinned = oop.create_class("Diff1InlinePinned", Diff1Inline) + +Diff1InlinePinned.name = "diff1_inline_pinned" +Diff1InlinePinned.shared_symbols = { "b" } + +---@param opt Diff1Inline.init.Opt +function Diff1InlinePinned:init(opt) + self:super(opt) +end + +Diff1InlinePinned.detach_files_for_swap = Diff1Pinned.detach_files_for_swap + +M.Diff1InlinePinned = Diff1InlinePinned +return M diff --git a/lua/diffview/scene/layouts/diff_1_pinned.lua b/lua/diffview/scene/layouts/diff_1_pinned.lua new file mode 100644 index 00000000..9ddce01e --- /dev/null +++ b/lua/diffview/scene/layouts/diff_1_pinned.lua @@ -0,0 +1,46 @@ +local Diff1 = require("diffview.scene.layouts.diff_1").Diff1 +local oop = require("diffview.oop") + +local M = {} + +---@class Diff1Pinned : Diff1 +---A single-window Diff1 whose b-window pins to a working-tree LOCAL buffer +---across log navigation. The b-side `vcs.File` is owned by the +---FileHistoryView (its pin_local cache), not by individual FileEntries. +---Listing "b" in `shared_symbols` keeps `Layout:owned_files()` from +---returning it, so `FileEntry:destroy` and `FileEntry:set_active` walk past +---the borrowed file. The view tears it down in `close()` once. +local Diff1Pinned = oop.create_class("Diff1Pinned", Diff1) + +Diff1Pinned.name = "diff1_plain_pinned" +Diff1Pinned.shared_symbols = { "b" } + +---@param opt Diff1.init.Opt +function Diff1Pinned:init(opt) + self:super(opt) +end + +-- Mirror `Diff2HorPinned:detach_files_for_swap`: skip detaching window b +-- when the next entry's b is the same `vcs.File` instance so the pinned +-- LOCAL buffer's diffview keymaps and edits survive the swap. In +-- multi-file pinning a row change can swap b to a different working-tree +-- File (each path has its own view-owned File); detach the old one in +-- that case so its buffer doesn't keep stale diffview state attached. +-- The inherited `detach_files()` still runs on tab-leave / view-close. +---@override +---@param next_entry? FileEntry +function Diff1Pinned:detach_files_for_swap(next_entry) + -- Without a next entry we have no comparison; preserve the old + -- "skip detach" behaviour for callers that haven't migrated to the + -- new signature. + if self.b and next_entry then + local next_layout = next_entry.layout --[[@as Diff1Pinned ]] + local next_b = next_layout and next_layout.b and next_layout.b.file + if next_b ~= self.b.file then + self.b:detach_file() + end + end +end + +M.Diff1Pinned = Diff1Pinned +return M diff --git a/lua/diffview/scene/layouts/diff_2_hor_pinned.lua b/lua/diffview/scene/layouts/diff_2_hor_pinned.lua index eaf00654..1590bdf0 100644 --- a/lua/diffview/scene/layouts/diff_2_hor_pinned.lua +++ b/lua/diffview/scene/layouts/diff_2_hor_pinned.lua @@ -71,10 +71,10 @@ function Diff2HorPinned:detach_files_for_swap(next_entry) if self.a then self.a:detach_file() end + -- Without a next entry we have no comparison; preserve the old + -- "skip detach" behaviour for callers that haven't migrated to the + -- new signature. if self.b and next_entry then - -- Without a next entry we have no comparison; preserve the old - -- "skip detach" behaviour for callers that haven't migrated to the - -- new signature. local next_layout = next_entry.layout --[[@as Diff2HorPinned ]] local next_b = next_layout and next_layout.b and next_layout.b.file if next_b ~= self.b.file then diff --git a/lua/diffview/scene/layouts/diff_2_ver_pinned.lua b/lua/diffview/scene/layouts/diff_2_ver_pinned.lua index 8a3a8578..4f156c49 100644 --- a/lua/diffview/scene/layouts/diff_2_ver_pinned.lua +++ b/lua/diffview/scene/layouts/diff_2_ver_pinned.lua @@ -40,9 +40,9 @@ function Diff2VerPinned:detach_files_for_swap(next_entry) if self.a then self.a:detach_file() end + -- See `Diff2HorPinned:detach_files_for_swap` for the no-next-entry + -- carve-out. if self.b and next_entry then - -- See `Diff2HorPinned:detach_files_for_swap` for the no-next-entry - -- carve-out. local next_layout = next_entry.layout --[[@as Diff2VerPinned ]] local next_b = next_layout and next_layout.b and next_layout.b.file if next_b ~= self.b.file then diff --git a/lua/diffview/scene/views/file_history/file_history_view.lua b/lua/diffview/scene/views/file_history/file_history_view.lua index 5318d814..2e826e64 100644 --- a/lua/diffview/scene/views/file_history/file_history_view.lua +++ b/lua/diffview/scene/views/file_history/file_history_view.lua @@ -532,24 +532,35 @@ function FileHistoryView:should_show_panel() return config.get_config().file_history_panel.show end --- Map a non-pinned Diff2 layout name to its pinned counterpart. Pinned --- layouts share window orientation with their unpinned siblings; we just --- re-route the layout class so the b-window keeps its file across entry --- swaps. Names that have no pinned variant fall through unchanged. +-- Map a non-pinned layout name to its pinned counterpart. Pinned variants +-- share window orientation with their unpinned siblings; we re-route the +-- layout class so the b-window keeps its file across entry swaps via +-- `shared_symbols = { "b" }`. Diff1/Diff1Inline have pinned variants too +-- (their b-side is also bound to the view-owned working-tree file in +-- pin_local mode); names without a pinned sibling fall through unchanged. local pinned_variant = { + diff1_plain = "diff1_plain_pinned", + diff1_inline = "diff1_inline_pinned", diff2_horizontal = "diff2_horizontal_pinned", diff2_vertical = "diff2_vertical_pinned", } --- Inverse of `pinned_variant`. A pinned class is only valid when adapters --- inject `revs.a = COMMIT` (which only happens under `pin_local`); applied --- to a parent-vs-commit history the pinned `should_null` mis-classifies --- status "A"/"?" and the adapter then fails to `show :`. The --- user-config path is already gated by `config`'s `standard_layouts` --- validation (pinned names aren't in the schema's allow-list), but we --- still fold pinned → unpinned here as belt-and-suspenders for any other --- caller (tests, future code) that reaches `get_default_layout` with a --- pinned name and `pin_local` off. +-- Inverse of `pinned_variant`. Pinned classes only make sense in +-- `pin_local` mode: they all declare `shared_symbols = { "b" }` and expect +-- the FileHistoryView to own the b-side `vcs.File` via its pin_local cache, +-- so outside `pin_local` there's no shared owner and the b-side would +-- never be torn down. The Diff2 pinned variants are additionally unsafe +-- there because they override `should_null` with parent-vs-commit semantics +-- that assume `revs.a = COMMIT` (only injected under `pin_local`); applied +-- to a parent-vs-commit history they mis-classify status "A"/"?" and the +-- adapter then fails to `show :` (the Diff1 pinned variants +-- inherit `Diff1.should_null` unchanged, so they don't have that specific +-- bug, but the shared-b ownership mismatch still applies). The user-config +-- path is already gated by `config`'s `standard_layouts` validation +-- (pinned names aren't in the schema's allow-list), but we still fold +-- pinned → unpinned here as belt-and-suspenders for any other caller +-- (tests, future code) that reaches `get_default_layout` with a pinned +-- name and `pin_local` off. local unpinned_variant = {} for unpinned, pinned in pairs(pinned_variant) do unpinned_variant[pinned] = unpinned @@ -564,22 +575,24 @@ function FileHistoryView:get_default_layout() name = FileHistoryView.get_default_diff2().name end + local resolved if self.pin_local then - -- pin_local needs a pinned-Diff2 layout: only those declare - -- `shared_symbols = { "b" }`, which is what keeps `FileEntry:destroy` - -- from tearing down the view-owned working-tree file on every refresh. - -- If the user's configured layout doesn't have a pinned variant - -- (e.g. `diff1_inline`), fall back to the default Diff2 so the - -- shared-b-side mechanism actually engages. - if not pinned_variant[name] then - name = FileHistoryView.get_default_diff2().name - end - name = pinned_variant[name] + -- Upgrade standard layout names to their pinned siblings so the + -- shared-b mechanism engages: pinned variants declare + -- `shared_symbols = { "b" }`, which keeps `FileEntry:destroy` from + -- tearing down the view-owned working-tree file on every refresh. + -- All standard layouts (`diff1_*`, `diff2_*`) have pinned siblings, + -- so the `pinned_variant` lookup normally hits. If a non-standard + -- name ever reaches here (e.g. via a future non-config caller that + -- bypasses the `standard_layouts` allow-list), fall back to the + -- default pinned Diff2 -- matching `resolve_pinned_layout` -- so the + -- shared-b contract still holds. + resolved = pinned_variant[name] or pinned_variant[FileHistoryView.get_default_diff2().name] else - name = unpinned_variant[name] or name + resolved = unpinned_variant[name] or name end - return config.name_to_layout(name --[[@as string ]]) + return config.name_to_layout(resolved --[[@as string ]]) end ---Inverse of `resolve_pinned_layout`: map a pinned class to its unpinned @@ -599,17 +612,19 @@ function FileHistoryView:unpinned_layout(layout_class) end ---Map an arbitrary layout class to the right one for this view's pin_local ----state. Used by `cycle_layout` / `set_layout` so neither action can drop a ----pin_local FileHistoryView into an unpinned `Diff2` (which would cause +---state. Used by `cycle_layout` / `set_layout` so neither action drops a +---pin_local FileHistoryView into an unpinned variant (which would cause ---`FileEntry:destroy` to tear down the view-owned working-tree file once ---per entry, and would untie the b-window from its shared LOCAL buffer). ---When `pin_local` is off, returns the input unchanged. When on: --- - already a pinned variant: returns it unchanged (preserves the user's --- chosen orientation). ---- - has a pinned sibling (e.g. `diff2_horizontal`): returns the pinned ---- sibling. ---- - no pinned variant (e.g. `diff1_inline`): returns the configured ---- default Diff2's pinned form, so the shared-b mechanism still engages. +--- - has a pinned sibling (e.g. `diff2_horizontal`, `diff1_inline`): +--- returns the pinned sibling. +--- - no pinned variant (e.g. `diff3_*`/`diff4_*` reaching us via a +--- user-supplied `view.cycle_layouts.default` entry or a direct +--- `actions.set_layout` call): falls back to the default Diff2's pinned +--- form so the shared-b contract still holds. ---@param layout_class Layout (class) ---@return Layout (class) function FileHistoryView:resolve_pinned_layout(layout_class) @@ -623,11 +638,14 @@ function FileHistoryView:resolve_pinned_layout(layout_class) return layout_class end - if not pinned_variant[name] then - name = FileHistoryView.get_default_diff2().name + local sibling = pinned_variant[name] + if sibling then + return config.name_to_layout(sibling --[[@as string ]]) end - return config.name_to_layout(pinned_variant[name] --[[@as string ]]) + return config.name_to_layout( + pinned_variant[FileHistoryView.get_default_diff2().name] --[[@as string ]] + ) end M.FileHistoryView = FileHistoryView diff --git a/lua/diffview/tests/functional/cycle_layouts_spec.lua b/lua/diffview/tests/functional/cycle_layouts_spec.lua index b845ed0a..5f73e5c3 100644 --- a/lua/diffview/tests/functional/cycle_layouts_spec.lua +++ b/lua/diffview/tests/functional/cycle_layouts_spec.lua @@ -4,12 +4,19 @@ local helpers = require("diffview.tests.helpers") local utils = require("diffview.utils") local Diff1 = require("diffview.scene.layouts.diff_1").Diff1 +local Diff1Inline = require("diffview.scene.layouts.diff_1_inline").Diff1Inline +local Diff1InlinePinned = require("diffview.scene.layouts.diff_1_inline_pinned").Diff1InlinePinned +local Diff1Pinned = require("diffview.scene.layouts.diff_1_pinned").Diff1Pinned local Diff2Hor = require("diffview.scene.layouts.diff_2_hor").Diff2Hor +local Diff2HorPinned = require("diffview.scene.layouts.diff_2_hor_pinned").Diff2HorPinned local Diff2Ver = require("diffview.scene.layouts.diff_2_ver").Diff2Ver +local Diff2VerPinned = require("diffview.scene.layouts.diff_2_ver_pinned").Diff2VerPinned local Diff3Hor = require("diffview.scene.layouts.diff_3_hor").Diff3Hor local Diff3Ver = require("diffview.scene.layouts.diff_3_ver").Diff3Ver local Diff3Mixed = require("diffview.scene.layouts.diff_3_mixed").Diff3Mixed local Diff4Mixed = require("diffview.scene.layouts.diff_4_mixed").Diff4Mixed +local FileHistoryView = + require("diffview.scene.views.file_history.file_history_view").FileHistoryView local eq = helpers.eq @@ -595,3 +602,227 @@ describe("diffview.actions.cycle_layout with custom config", function() eq(Diff2Hor, converted_layouts[1]) end) end) + +-- Regression: when `pin_local` is on and the cycle list contains a Diff1 +-- layout (e.g. `diff1_inline` auto-inserted by config validation when +-- `view.file_history.layout` is set to it), `cycle_layout` used to land +-- on that entry, `resolve_pinned_layout` would collapse it to the default +-- Diff2's pinned form, and if the cycle was already on that orientation +-- `convert_layout` was a no-op -- so the user saw cycling stop part-way +-- (e.g. Ver -> Hor -> stuck on Hor). `pinned_variant` now covers Diff1 +-- and Diff1Inline (mapped to their pin_local-safe pinned siblings), so +-- the same set of layout NAMES is reachable whether `pin_local` is on or +-- off; in pin_local mode the active class is the `*_pinned` form, which +-- declares `shared_symbols = { "b" }` and keeps `FileEntry:destroy` from +-- tearing down the view-owned working-tree file. +describe("diffview.actions.cycle_layout pin_local cycling", function() + local lib = require("diffview.lib") + + local stubs = {} + local converted_layouts + + local function stub(tbl, key, val) + stubs[#stubs + 1] = { tbl, key, tbl[key] } + tbl[key] = val + end + + local original_config + + before_each(function() + converted_layouts = {} + original_config = vim.deepcopy(config.get_config()) + end) + + after_each(function() + for i = #stubs, 1, -1 do + local s = stubs[i] + s[1][s[2]] = s[3] + end + stubs = {} + + local old_warn = utils.warn + utils.warn = function() end + config.setup(original_config) + utils.warn = old_warn + end) + + local function mock_file_entry(layout_class) + return { + layout = { + class = layout_class, + emitter = require("diffview.events").EventEmitter(), + }, + kind = "working", + convert_layout = function(self, next_class) + converted_layouts[#converted_layouts + 1] = next_class + self.layout.class = next_class + end, + } + end + + -- Mock a FileHistoryView: stub `instanceof` so the action's dispatch + -- picks the file-history branch, and inherit the helper methods from + -- the real class via `__index` so `unpinned_layout` and + -- `resolve_pinned_layout` reflect actual behaviour. + local function mock_file_history_view(files, cur_entry, pin_local) + return setmetatable({ + pin_local = pin_local, + instanceof = function(self, other) + return other == FileHistoryView + end, + cur_layout = { + get_main_win = function() + return { id = 1 } + end, + is_focused = function() + return false + end, + sync_scroll = function() end, + }, + panel = { + list_files = function() + return files + end, + }, + cur_file = function() + return cur_entry + end, + set_file = function() end, + }, { __index = FileHistoryView }) + end + + it("cycles through all entries when cycle has a trailing Diff1 entry", function() + -- Reproduces the reported bug: with cycle `{ Ver, Hor, Diff1Inline }` + -- and `pin_local`, cycling used to skip Diff1Inline (resolved to the + -- default Diff2's pinned form, so the user got Ver -> Hor -> stuck on + -- Hor). `pinned_variant` now covers Diff1Inline, so cycling lands on + -- `Diff1InlinePinned` (b-side declared shared, view-owned working-tree + -- File survives entry teardown) and the cycle visits all three. + local old_warn = utils.warn + utils.warn = function() end + config.setup({ + view = { + file_history = { layout = "diff2_vertical", pin_local = true }, + cycle_layouts = { + default = { "diff2_vertical", "diff2_horizontal", "diff1_inline" }, + }, + }, + }) + utils.warn = old_warn + + local file = mock_file_entry(Diff2VerPinned) + local view = mock_file_history_view({ file }, file, true) + + stub(lib, "get_current_view", function() + return view + end) + stub(vim.api, "nvim_win_get_cursor", function() + return { 1, 0 } + end) + + actions.cycle_layout() + eq(1, #converted_layouts) + eq(Diff2HorPinned, converted_layouts[1]) + + actions.cycle_layout() + eq(2, #converted_layouts) + eq(Diff1InlinePinned, converted_layouts[2]) + + actions.cycle_layout() + eq(3, #converted_layouts) + eq(Diff2VerPinned, converted_layouts[3]) + end) + + it("cycles through Diff1-only cycles in pin_local mode", function() + -- The cycle list contains only Diff1 variants; both have pinned + -- siblings, so cycling stays in the pin_local-safe class space. + local old_warn = utils.warn + utils.warn = function() end + config.setup({ + view = { + file_history = { layout = "diff1_inline", pin_local = true }, + default = { layout = "diff1_inline" }, + cycle_layouts = { default = { "diff1_inline", "diff1_plain" } }, + }, + }) + utils.warn = old_warn + + local file = mock_file_entry(Diff1InlinePinned) + local view = mock_file_history_view({ file }, file, true) + + stub(lib, "get_current_view", function() + return view + end) + stub(vim.api, "nvim_win_get_cursor", function() + return { 1, 0 } + end) + + actions.cycle_layout() + eq(1, #converted_layouts) + eq(Diff1Pinned, converted_layouts[1]) + end) + + it("reaches the same set of layouts whether pin_local is on or off", function() + -- Symmetry check: with `cycle_layouts.default = { Hor, Ver, Diff1Inline }`, + -- three presses should land on the same sequence of layout NAMES + -- regardless of `pin_local` (the pin_local entries differ only by + -- carrying the `*_pinned` class for the same orientation). + local old_warn = utils.warn + utils.warn = function() end + config.setup({ + view = { + cycle_layouts = { + default = { "diff2_horizontal", "diff2_vertical", "diff1_inline" }, + }, + }, + }) + utils.warn = old_warn + + local file_off = mock_file_entry(Diff2Hor) + local view_off = mock_file_history_view({ file_off }, file_off, false) + stub(lib, "get_current_view", function() + return view_off + end) + stub(vim.api, "nvim_win_get_cursor", function() + return { 1, 0 } + end) + + actions.cycle_layout() + actions.cycle_layout() + actions.cycle_layout() + local off_names = { + converted_layouts[1].name, + converted_layouts[2].name, + converted_layouts[3].name, + } + eq({ "diff2_vertical", "diff1_inline", "diff2_horizontal" }, off_names) + + -- Reset for the pin_local pass. + for i = #stubs, 1, -1 do + local s = stubs[i] + s[1][s[2]] = s[3] + end + stubs = {} + converted_layouts = {} + + local file_on = mock_file_entry(Diff2HorPinned) + local view_on = mock_file_history_view({ file_on }, file_on, true) + stub(lib, "get_current_view", function() + return view_on + end) + stub(vim.api, "nvim_win_get_cursor", function() + return { 1, 0 } + end) + + actions.cycle_layout() + actions.cycle_layout() + actions.cycle_layout() + local on_names = { + converted_layouts[1].name, + converted_layouts[2].name, + converted_layouts[3].name, + } + -- Same orientations, with `*_pinned` variants throughout. + eq({ "diff2_vertical_pinned", "diff1_inline_pinned", "diff2_horizontal_pinned" }, on_names) + end) +end) diff --git a/lua/diffview/tests/functional/layouts_spec.lua b/lua/diffview/tests/functional/layouts_spec.lua index c9f4414f..50595e81 100644 --- a/lua/diffview/tests/functional/layouts_spec.lua +++ b/lua/diffview/tests/functional/layouts_spec.lua @@ -811,6 +811,129 @@ describe("diffview.scene.layouts.diff_2_*_pinned detach_files_for_swap", functio end) end) +describe("diffview.scene.layouts.diff_1_*_pinned ownership", function() + local Diff1Pinned = require("diffview.scene.layouts.diff_1_pinned").Diff1Pinned + local Diff1InlinePinned = require("diffview.scene.layouts.diff_1_inline_pinned").Diff1InlinePinned + + -- The b-side `vcs.File` is owned by the FileHistoryView (its pin_local + -- cache), not by individual FileEntries. `shared_symbols` is the contract + -- that tells `Layout:owned_files()` to exclude that window from the + -- destruction set in `FileEntry:destroy` and `FileEntry:set_active`. + it("declares 'b' as a shared symbol so entry teardown skips it", function() + eq({ "b" }, Diff1Pinned.shared_symbols) + eq({ "b" }, Diff1InlinePinned.shared_symbols) + end) + + -- Concrete check of the filter: `Diff1Pinned:owned_files()` must return + -- nothing (its only window is the shared b). For `Diff1InlinePinned` the + -- `a_file` is per-entry-owned and stays in the destruction set; the + -- shared `b` does not. This is what protects the view-owned b-file from + -- being torn down on entry refresh in pin_local mode. + it("owned_files() excludes the b-side file", function() + do + local b_file = { path = "foo.txt" } + local inst = setmetatable({ + b = { file = b_file }, + windows = {}, + symbols = Diff1Pinned.symbols, + shared_symbols = Diff1Pinned.shared_symbols, + }, { __index = Diff1Pinned }) + + eq({}, inst:owned_files()) + end + + do + local a_file = { path = "old/foo.txt" } + local b_file = { path = "foo.txt" } + local inst = setmetatable({ + b = { file = b_file }, + a_file = a_file, + windows = { { file = b_file } }, + symbols = Diff1InlinePinned.symbols, + shared_symbols = Diff1InlinePinned.shared_symbols, + }, { __index = Diff1InlinePinned }) + + eq({ a_file }, inst:owned_files()) + end + end) +end) + +describe("diffview.scene.layouts.diff_1_*_pinned detach_files_for_swap", function() + local Diff1Pinned = require("diffview.scene.layouts.diff_1_pinned").Diff1Pinned + local Diff1InlinePinned = require("diffview.scene.layouts.diff_1_inline_pinned").Diff1InlinePinned + + -- The swap variant is what `_set_file` calls between log entries; it + -- skips the pinned b so the LOCAL buffer's keymaps/edits survive the + -- swap when the b-file stays the same instance. The full `detach_files()` + -- is left to the base Layout so tab-leave/view-close still tear + -- everything down. + it("does not detach window b when next entry's b matches", function() + for _, cls in ipairs({ Diff1Pinned, Diff1InlinePinned }) do + local detached = {} + local shared_b_file = { path = "live.txt" } + local inst = setmetatable({ + b = { + file = shared_b_file, + detach_file = function() + detached.b = true + end, + }, + }, { __index = cls }) + + local next_entry = { layout = { b = { file = shared_b_file } } } + inst:detach_files_for_swap(next_entry) + + assert.is_nil(detached.b) + end + end) + + -- Multi-file pinning: each path has its own view-owned working-tree File. + -- Crossing rows changes the b-side instance, and the OLD b's buffer + -- must be detached so it doesn't keep diffview keymaps and buffer-local + -- overrides after navigation. + it("detaches window b when the next entry's b is a different File instance", function() + for _, cls in ipairs({ Diff1Pinned, Diff1InlinePinned }) do + local detached = {} + local cur_b_file = { path = "alpha.txt" } + local next_b_file = { path = "beta.txt" } + local inst = setmetatable({ + b = { + file = cur_b_file, + detach_file = function() + detached.b = true + end, + }, + }, { __index = cls }) + + local next_entry = { layout = { b = { file = next_b_file } } } + inst:detach_files_for_swap(next_entry) + + assert.True(detached.b) + end + end) + + -- Defensive: when the caller doesn't pass a next entry, treat it as + -- "nothing to compare against" and skip detaching b, mirroring the + -- Diff2 pinned variants. + it("skips detaching b when no next_entry is passed", function() + for _, cls in ipairs({ Diff1Pinned, Diff1InlinePinned }) do + local detached = {} + local inst = setmetatable({ + b = { + file = { path = "live.txt" }, + detach_file = function() + detached.b = true + end, + }, + }, { __index = cls }) + + inst:detach_files_for_swap() + + assert.is_nil(detached.b) + end + end) +end) + describe("diffview.scene.layouts.diff_2_*_pinned use_entry inheritance", function() local Diff2HorPinned = require("diffview.scene.layouts.diff_2_hor_pinned").Diff2HorPinned local Diff2VerPinned = require("diffview.scene.layouts.diff_2_ver_pinned").Diff2VerPinned diff --git a/lua/diffview/tests/functional/pin_local_spec.lua b/lua/diffview/tests/functional/pin_local_spec.lua index 99cb7bd7..f0f128d7 100644 --- a/lua/diffview/tests/functional/pin_local_spec.lua +++ b/lua/diffview/tests/functional/pin_local_spec.lua @@ -1,3 +1,5 @@ +local Diff1InlinePinned = require("diffview.scene.layouts.diff_1_inline_pinned").Diff1InlinePinned +local Diff1Pinned = require("diffview.scene.layouts.diff_1_pinned").Diff1Pinned local Diff2 = require("diffview.scene.layouts.diff_2").Diff2 local Diff2Hor = require("diffview.scene.layouts.diff_2_hor").Diff2Hor local Diff2HorPinned = require("diffview.scene.layouts.diff_2_hor_pinned").Diff2HorPinned @@ -685,21 +687,22 @@ describe("FileHistoryView pinned-local layout selection (sanity)", function() eq("diff2_vertical", stub_named("diff2_vertical_pinned", nil):get_default_layout().name) end) - -- pin_local + a non-Diff2 layout (e.g. `diff1_inline`): the layout has no - -- pinned variant, so the shared-b-side mechanism would not engage and - -- entry teardown would tear down the view-owned working-tree file. Force - -- the default Diff2 (which IS pinned-capable) so pinning actually works. - -- The exact orientation depends on `prefer_horizontal()` -- assert only - -- that the result is one of the pinned Diff2 variants. - it("falls back to a pinned Diff2 when pin_local + a non-Diff2 layout", function() - local pinned_names = { - diff2_horizontal_pinned = true, - diff2_vertical_pinned = true, - } - for _, name in ipairs({ "diff1_inline", "diff1_plain" }) do - local resolved = stub_named(name, true):get_default_layout().name - assert.is_true(pinned_names[resolved], "expected pinned Diff2, got " .. tostring(resolved)) - end + -- pin_local + a Diff1 layout (e.g. `diff1_inline`): upgrade to the + -- pinned Diff1 sibling so the shared-b mechanism still engages. The + -- pinned variants declare `shared_symbols = { "b" }`, so + -- `FileEntry:destroy` leaves the view-owned working-tree file alone. + it("upgrades a Diff1 layout to its pinned variant when pin_local is on", function() + eq("diff1_inline_pinned", stub_named("diff1_inline", true):get_default_layout().name) + eq("diff1_plain_pinned", stub_named("diff1_plain", true):get_default_layout().name) + end) + + -- Defensive path mirroring the Diff2 downgrade: if a pinned Diff1 name + -- reaches `get_default_layout` with `pin_local` unset, it must be + -- downgraded to its unpinned sibling. Pinned variants borrow the b-side + -- from the view, which only owns one when `pin_local` is live. + it("downgrades a pinned Diff1 name when pin_local is unset", function() + eq("diff1_inline", stub_named("diff1_inline_pinned", nil):get_default_layout().name) + eq("diff1_plain", stub_named("diff1_plain_pinned", nil):get_default_layout().name) end) end) @@ -711,9 +714,14 @@ end) -- breaking the pin and wiping shared diffview state from the user's -- working-tree buffers. describe("FileHistoryView:resolve_pinned_layout", function() + local Diff1 = require("diffview.scene.layouts.diff_1").Diff1 + local Diff1Inline = require("diffview.scene.layouts.diff_1_inline").Diff1Inline local Diff2Hor = require("diffview.scene.layouts.diff_2_hor").Diff2Hor local Diff2Ver = require("diffview.scene.layouts.diff_2_ver").Diff2Ver - local Diff1Inline = require("diffview.scene.layouts.diff_1_inline").Diff1Inline + local Diff3Hor = require("diffview.scene.layouts.diff_3_hor").Diff3Hor + local Diff3Ver = require("diffview.scene.layouts.diff_3_ver").Diff3Ver + local Diff3Mixed = require("diffview.scene.layouts.diff_3_mixed").Diff3Mixed + local Diff4Mixed = require("diffview.scene.layouts.diff_4_mixed").Diff4Mixed local function inst(pin_local) return setmetatable({ pin_local = pin_local }, { __index = FileHistoryView }) @@ -735,16 +743,41 @@ describe("FileHistoryView:resolve_pinned_layout", function() eq(Diff2VerPinned, inst(true):resolve_pinned_layout(Diff2VerPinned)) end) - -- A non-Diff2 layout has no pinned sibling, so the only safe option is - -- to fall back to a pinned Diff2 (the configured default's pinned form), - -- so the shared-b mechanism still engages. The exact orientation - -- depends on `prefer_horizontal()`; only assert it's a pinned Diff2. - it("falls back to a pinned Diff2 when the input has no pinned variant", function() - local resolved = inst(true):resolve_pinned_layout(Diff1Inline) - assert.is_true( - resolved == Diff2HorPinned or resolved == Diff2VerPinned, - "expected a pinned Diff2, got " .. tostring(resolved.name) - ) + -- Diff1 variants gain pinned siblings too so `cycle_layout` / + -- `set_layout` can route to them without `FileEntry:destroy` tearing + -- down the view-owned working-tree file. `Diff1*Pinned` declare + -- `shared_symbols = { "b" }`, mirroring `Diff2*Pinned`. + it("upgrades unpinned Diff1 to its pinned sibling", function() + eq(Diff1Pinned, inst(true):resolve_pinned_layout(Diff1)) + eq(Diff1InlinePinned, inst(true):resolve_pinned_layout(Diff1Inline)) + end) + + it("preserves a pinned Diff1 variant unchanged (idempotent)", function() + eq(Diff1Pinned, inst(true):resolve_pinned_layout(Diff1Pinned)) + eq(Diff1InlinePinned, inst(true):resolve_pinned_layout(Diff1InlinePinned)) + end) + + -- `actions.set_layout("diff3_horizontal")` and user-supplied + -- `view.cycle_layouts.default` entries can reach `resolve_pinned_layout` + -- with merge-only layouts. Those have no pinned sibling, so they would + -- otherwise drop a pin_local FileHistoryView into an unpinned class + -- whose `shared_symbols` is empty, letting `FileEntry:destroy` tear + -- down the view-owned working-tree file. Falling back to the default + -- Diff2's pinned form preserves the shared-b contract. The exact + -- orientation depends on `prefer_horizontal()`, so we only assert + -- that the result is one of the pinned Diff2 variants. + it("falls back to a pinned Diff2 for non-pinnable layouts in pin_local", function() + local pinned_diff2 = { + [Diff2HorPinned] = true, + [Diff2VerPinned] = true, + } + for _, cls in ipairs({ Diff3Hor, Diff3Ver, Diff3Mixed, Diff4Mixed }) do + local resolved = inst(true):resolve_pinned_layout(cls) + assert.is_true( + pinned_diff2[resolved], + "expected pinned Diff2, got " .. tostring(resolved and resolved.name) + ) + end end) end) @@ -756,13 +789,16 @@ end) -- isn't a known pinned variant, so non-pin_local views keep their -- existing behaviour. describe("FileHistoryView:unpinned_layout", function() + local Diff1 = require("diffview.scene.layouts.diff_1").Diff1 + local Diff1Inline = require("diffview.scene.layouts.diff_1_inline").Diff1Inline local Diff2Hor = require("diffview.scene.layouts.diff_2_hor").Diff2Hor local Diff2Ver = require("diffview.scene.layouts.diff_2_ver").Diff2Ver - local Diff1Inline = require("diffview.scene.layouts.diff_1_inline").Diff1Inline local view = setmetatable({}, { __index = FileHistoryView }) it("maps pinned variants back to their unpinned siblings", function() + eq(Diff1, view:unpinned_layout(Diff1Pinned)) + eq(Diff1Inline, view:unpinned_layout(Diff1InlinePinned)) eq(Diff2Hor, view:unpinned_layout(Diff2HorPinned)) eq(Diff2Ver, view:unpinned_layout(Diff2VerPinned)) end)