caravan: fix doubled pending value and lost marks across filter views

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
 

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

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