stocks: make the overlay's collapse hotkey a toggle - #5920
Alistair-Afton wants to merge 4 commits into
Conversation
Collapsing all categories shrinks stocks.i_height but left scroll_position_item untouched, so the view pointed past the end of the list with no way to scroll back. Reset it when collapsing, and clamp scroll_position_type when removing empty categories shrinks the type list. Also fix a missing local on num_items in expand_all, and make the three actions module-level so tests can reach them. fixes DFHack#5772
There was no way to restore the item list after collapsing all categories without switching stocks pages or knowing about the separate expand hotkey. Ctrl-X now expands when every category is collapsed, and its label flips between 'collapse all' and 'expand all' to match. fixes DFHack#5773
d1b8035 to
823fbd9
Compare
SilasD
left a comment
There was a problem hiding this comment.
The new functions all_collapsed and toggle_all have the same issue I raised in PR #5905 . to reiterate:
I am very uncomfortable making collapse_all, expand_all, remove_empty, all_collapsed, and toggle_all global.
these functions directly manipulate or read the UI; the UI is generally fragile and tends to leave dangling pointers when a viewsheet is closed.
making these functions global means that any other script can call them at any time. if the stocks viewsheet is not open, I think there is a significant risk of crashing the game.
to emphasize: these functions should (must) not be called if the stocks interface is not open.
this is true even of all_collapsed, which only reads the UI state. even walking a vector of boolean might be problematic if the stocks window has never been opened; I am not at all convinced that the vector would have valid contents instead of garbage.
I would either like these changed back to local or have a safety-test added, something like
if stocks.open == false then return; end
at the start of each function.
or both. both would be good.
Other than that issue, the code generally looks fine, though I didn't test it.
|
Done both here as well: Also added
Verified in-game: 4/4 tests pass, full suite green (271/271). |
Fixes #5773
Summary
collapse allhotkey (Ctrl-X) is now a toggle: when every category is collapsed it expands the list again instead of doing nothing.collapse allandexpand allso it always describes what the key will do.expand allhotkey (Ctrl-Z) is unchanged, so expanding is still a single press when only some categories are collapsed.Testing
toggle_all/all_collapsedin-game against mocked stocks state: collapse -> expand -> collapse cycle flips flags and recomputesi_heightcorrectly each way, and the scroll reset from stocks: fix blank item list after collapsing while scrolled down #5905 still applies on the collapse half.Note: this branch is based on
stocks-scroll-bounds(#5905) since it uses the module-level functions from that change. The diff will shrink to just this change once #5905 merges.