From 0d7627f3a5aa6fd4ace077d93eb02873e9be1f6a Mon Sep 17 00:00:00 2001 From: Alistair-Afton Date: Tue, 15 Sep 2026 19:55:13 +0200 Subject: [PATCH 1/2] `caravan`: fix doubled pending value and lost marks across filter views Each "bring goods to depot" filter combination (grouped x inside containers) cached its own copy of choice state. Rebuilding a combo added the selected values to the pending total again, doubling the displayed value, and marks made in one view were invisible in the others, so dismissing the modal silently dropped them. Keep a shared set of explicit marks on the MoveGoods widget, derive each cached view's pending flags from it, recompute the pending total once per scan instead of accumulating it, and commit every explicit mark on dismiss regardless of which view is active. --- changelog.txt | 1 + internal/caravan/movegoods.lua | 121 +++++++++++++++++++++------------ 2 files changed, 80 insertions(+), 42 deletions(-) diff --git a/changelog.txt b/changelog.txt index b492b6d2ec..a638977a07 100644 --- a/changelog.txt +++ b/changelog.txt @@ -32,6 +32,7 @@ Template for new versions: ## Fixes - `bodyswap`: fix "invalid argument count" when the target unit has no nemesis record +- `caravan`: fix doubled "total value of items marked for trade" after toggling filter options, and keep item marks when switching between filter views in the ``Bring goods to depot`` overlay - `fix/loyaltycascade`: guard against citizens that are not historical figures and emit a warning. - `gui/siegemanager`: fix nil index if there are no siege engines on the map diff --git a/internal/caravan/movegoods.lua b/internal/caravan/movegoods.lua index 77e8222979..1feab69f6f 100644 --- a/internal/caravan/movegoods.lua +++ b/internal/caravan/movegoods.lua @@ -144,6 +144,11 @@ end function MoveGoods:init() self.value_pending = 0 + -- marked state is shared across all cached choice lists so toggles made + -- in one filter view are not lost when switching to another + self.marked = {} + self.item_values = {} + self.items_by_id = {} self.animal_ethics, self.wood_ethics = get_ethics_restrictions() self.banned_items = common.get_banned_items() @@ -499,8 +504,14 @@ function MoveGoods:cache_choices() local item_id = item.id local value = common.get_perceived_value(item) if value <= 0 then goto continue end + self.item_values[item_id] = value + self.items_by_id[item_id] = item + -- explicit user marks win; otherwise fall back to the initial state + local is_pending = self.marked[item_id] + if is_pending == nil then + is_pending = not not pending[item_id] or item.flags.in_building + end local dist = get_distance(self.depot, xyz2pos(dfhack.items.getPosition(item))) - local is_pending = not not pending[item_id] or item.flags.in_building local is_forbidden = item.flags.forbid local is_banned, is_risky = common.scan_banned(item, self.risky_items) local is_requested = dfhack.items.isRequestedTradeGood(item) @@ -540,7 +551,6 @@ function MoveGoods:cache_choices() has_requested=is_requested, has_ethical=has_ethical, ethical_mixed=is_ethical_mixed, - dirty=false, } local search_key if not inside_containers and is_container(item) then @@ -574,7 +584,17 @@ function MoveGoods:cache_choices() group.text = make_choice_text(data.num_at_depot == data.quantity, data.dist, data.total_value, data.quantity, data.desc, cache_threshold) table.insert(group_choices, group) - self.value_pending = self.value_pending + (data.per_item_value * data.selected) + end + + self.value_pending = 0 + for item_id, item in pairs(self.items_by_id) do + local is_marked = self.marked[item_id] + if is_marked == nil then + is_marked = not not pending[item_id] or item.flags.in_building + end + if is_marked then + self.value_pending = self.value_pending + (self.item_values[item_id] or 0) + end end self.choices_cache[get_cache_index(true, inside_containers)] = group_choices @@ -654,29 +674,47 @@ function MoveGoods:get_choices() return choices end +-- keeps the marked flag consistent in every cached choice list that contains +-- this item so other filter views reflect the toggle +function MoveGoods:sync_marked(item_id) + local marked = self.marked[item_id] + for _, choices in pairs(self.choices_cache) do + for _, choice in ipairs(choices) do + local items = choice.data.items + if items[item_id] then + items[item_id].pending = marked + local selected = 0 + for _, item_data in pairs(items) do + if item_data.pending then selected = selected + 1 end + end + choice.data.selected = selected + end + end + end +end + +function MoveGoods:set_marked(item_id, marked) + if self.marked[item_id] == marked then return end + self.marked[item_id] = marked + self.value_pending = self.value_pending + + (self.item_values[item_id] or 0) * (marked and 1 or -1) + self:sync_marked(item_id) +end + function MoveGoods:toggle_item_base(choice, target_value) if choice.item_id then - local item_data = choice.data.items[choice.item_id] - if item_data.pending then - self.value_pending = self.value_pending - choice.data.per_item_value - choice.data.selected = choice.data.selected - 1 - end - if target_value == nil then target_value = not item_data.pending end - item_data.pending = target_value - if item_data.pending then - self.value_pending = self.value_pending + choice.data.per_item_value - choice.data.selected = choice.data.selected + 1 + if target_value == nil then + target_value = not choice.data.items[choice.item_id].pending end + self:set_marked(choice.item_id, target_value) else - self.value_pending = self.value_pending - (choice.data.selected * choice.data.per_item_value) - if target_value == nil then target_value = (choice.data.selected ~= choice.data.quantity) end - for _, item_data in pairs(choice.data.items) do - item_data.pending = target_value + if target_value == nil then + target_value = (choice.data.selected ~= choice.data.quantity) + end + for item_id in pairs(choice.data.items) do + self:set_marked(item_id, target_value) end - choice.data.selected = target_value and choice.data.quantity or 0 - self.value_pending = self.value_pending + (choice.data.selected * choice.data.per_item_value) end - choice.data.dirty = true return target_value end @@ -748,30 +786,29 @@ function MoveGoodsModal:onDismiss() -- mark/unmark selected goods for trade local depot = self.depot if not depot then return end + local move_goods = self.subviews.move_goods + move_goods:cache_choices() -- make sure marked/items_by_id are populated local pending = self.pending_item_ids - for _, choice in ipairs(self.subviews.move_goods:cache_choices()) do - if not choice.data.dirty then goto continue end - for item_id, item_data in pairs(choice.data.items) do - local item = item_data.item - if item_data.pending and not pending[item_id] then - item.flags.forbid = false - if dfhack.items.getHolderBuilding(item) == depot then - item.flags.in_building = true - else - -- TODO: if there is just one (ethical, if filtered) item inside of a bin, mark the item for - -- trade instead of the bin - -- TODO: give containers that have some items inside of them marked for trade a ":" marker in the UI - -- TODO: correlate items inside containers marked for trade across the cached choices so no choices are lost - dfhack.items.markForTrade(item, depot) - end - elseif not item_data.pending and pending[item_id] then - local spec_ref = dfhack.items.getSpecificRef(item, df.specific_ref_type.JOB) - if spec_ref then - dfhack.job.removeJob(spec_ref.data.job) - end - elseif not item_data.pending and item.flags.in_building and dfhack.items.getHolderBuilding(item) == depot then - item.flags.in_building = false + for item_id, marked in pairs(move_goods.marked) do + local item = move_goods.items_by_id[item_id] + if not item then goto continue end + if marked and not pending[item_id] then + item.flags.forbid = false + if dfhack.items.getHolderBuilding(item) == depot then + item.flags.in_building = true + else + -- TODO: if there is just one (ethical, if filtered) item inside of a bin, mark the item for + -- trade instead of the bin + -- TODO: give containers that have some items inside of them marked for trade a ":" marker in the UI + dfhack.items.markForTrade(item, depot) + end + elseif not marked and pending[item_id] then + local spec_ref = dfhack.items.getSpecificRef(item, df.specific_ref_type.JOB) + if spec_ref then + dfhack.job.removeJob(spec_ref.data.job) end + elseif marked == false and item.flags.in_building and dfhack.items.getHolderBuilding(item) == depot then + item.flags.in_building = false end ::continue:: end From 7b1537fd1b5da9d1f76f1476b6bd0d54a7efc50c Mon Sep 17 00:00:00 2001 From: Alistair-Afton Date: Fri, 18 Sep 2026 00:30:26 +0200 Subject: [PATCH 2/2] caravan: add tests for movegoods pending value and mark persistence Regression coverage for the filter-view fixes: the pending total must equal the canonical recomputation after every choices cache build, and marks made in one view must propagate to rebuilt views. --- test/caravan/movegoods.lua | 92 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 test/caravan/movegoods.lua diff --git a/test/caravan/movegoods.lua b/test/caravan/movegoods.lua new file mode 100644 index 0000000000..bc2ac186a0 --- /dev/null +++ b/test/caravan/movegoods.lua @@ -0,0 +1,92 @@ +config = { + mode = 'fortress', + target = 'caravan', +} + +local movegoods = reqscript('internal/caravan/movegoods') +local common = reqscript('internal/caravan/common') +local mock = require('test_util.mock') + +local function get_depot() + for _, b in ipairs(df.global.world.buildings.all) do + if df.building_tradedepotst:is_instance(b) then return b end + end +end + +local function expected_pending(w) + local pending = w.pending_item_ids + local sum = 0 + for item_id, item in pairs(w.items_by_id) do + local marked = w.marked[item_id] + if marked == nil then + marked = not not pending[item_id] or item.flags.in_building + end + if marked then + sum = sum + (w.item_values[item_id] or 0) + end + end + return sum +end + +local function find_choice(w, item_id) + for _, choices in pairs(w.choices_cache) do + for _, choice in ipairs(choices) do + if choice.data.items[item_id] then return choice end + end + end +end + +local function run_check() + local depot = get_depot() or {centerx=0, centery=0, z=0, contained_items={}} + local ids = {} + for _, item in ipairs(df.global.world.items.other.IN_PLAY) do + if common.get_perceived_value(item) > 0 then ids[item.id] = true end + end + if not next(ids) then return end + local w = movegoods.MoveGoods{pending_item_ids=ids, depot=depot} + for _, inside in ipairs{false, true} do + for _, group in ipairs{false, true} do + w.subviews.inside_containers:setOption(inside and 'Yes' or 'No') + w.subviews.group_items:setOption(group and 'Yes' or 'No') + w:cache_choices() + expect.eq(expected_pending(w), w.value_pending) + end + end + return w +end + +function test.value_pending_not_doubled_by_cache_builds() + run_check() +end + +function test.marks_persist_across_filter_views() + local w = run_check() + if not w then return end + -- mark a non-container item (those appear in every filter view) + local marked_id + for _, choice in ipairs(w.choices_cache[1]) do + for item_id, item_data in pairs(choice.data.items) do + if not df.item_binst:is_instance(item_data.item) + and not item_data.item:isFoodStorage() then + marked_id = item_id + break + end + end + if marked_id then break end + end + if not marked_id then return end + local choice = find_choice(w, marked_id) + w:toggle_item_base(choice, true) + expect.true_(w.marked[marked_id]) + -- rebuild every view and verify the mark propagated + for idx in pairs(w.choices_cache) do w.choices_cache[idx] = nil end + for _, inside in ipairs{false, true} do + for _, group in ipairs{false, true} do + w.subviews.inside_containers:setOption(inside and 'Yes' or 'No') + w.subviews.group_items:setOption(group and 'Yes' or 'No') + w:cache_choices() + local c = find_choice(w, marked_id) + if c then expect.true_(c.data.items[marked_id].pending) end + end + end +end