Skip to content

stocks: fix blank item list after collapsing while scrolled down - #5905

Open
Alistair-Afton wants to merge 4 commits into
DFHack:developfrom
Alistair-Afton:stocks-scroll-bounds
Open

Alistair-Afton wants to merge 4 commits into
DFHack:developfrom
Alistair-Afton:stocks-scroll-bounds

Conversation

@Alistair-Afton

@Alistair-Afton Alistair-Afton commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

collapse_all shrank i_height but left scroll_position_item stale, so collapsing a list while scrolled down left the viewport past the end of the list and rendered a blank pane (fixes #5772).

Applied the same scroll clamp to remove_empty, which had the identical hazard on the type list, and fixed a leaked global (num_items in expand_all was missing local).

Testing

  • Live fort: collapse resets scroll to 0; flags continue toggling correctly
  • ci/lint.py clean

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

@SilasD SilasD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am very uncomfortable making collapse_all, expand_all, and remove_empty global.

these functions directly manipulate 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.

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.

@Alistair-Afton

Copy link
Copy Markdown
Contributor Author

Done both: the functions are local again, and each now starts with if not stocks.open then return end so a stray activation as the page closes can't touch the interface state.

Also added test/plugins/stocks.lua, which drives the overlay hotkeys through the widget's real onInput path in a running game and verifies:

  • collapse resets scroll_position_item
  • expand restores all sections
  • remove-empties keeps scroll_position_type in bounds
  • all three hotkeys are no-ops while the stocks page is closed (exercises the stocks.open guard directly)

Verified in-game: 4/4 tests pass, full suite green (271/271).

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: stocks.overlay doesn't enforce scroll bounds when collapsing categories

2 participants