confirm fixes: pausing, Escape from uniform, order-remove descriptions
#1446
Conversation
Prevent dialogs.showYesNoPrompt from showing the pause option by passing a nil on_pause argument when a confirmation is not pausable (e.g., trade-cancel, depot-remove).
Match the _MOUSE_R handling: prompt if there are uniform changes.
When the DF interface percentage is not 100, the interface width can be smaller than the window width. For certain sizes (depending on the interface percentage), the following condition can hold: - interface width <= 154 < window width In this situation, the info window tab row will not have "unwrapped" (from four to two UI rows), but the previous order index calculation code would assume that it had (due to using the window width). This two UI row discrepancy caused the calculated order index to be one too high (and possibly out of bounds) when clicking on either of the bottom two UI rows of an order remove button. The incorrect index caused the confirmation to display the description of the following order.
The scroll_position_work_orders value is being used here as the index of the first displayed order. DF seems to maintain this when the list view is updated via scrolling, but DF does not update it in at least two important situations: - when an order is removed, and - when the order list view grows enough to display additional orders (e.g., by increasing the height of the DF window). When the order list view is not scrolled all the way to the bottom, DF handles these actions by pushing orders into view at the bottom of the list view. Since the same order is still at the top of the list view, this does not require an update of the reported scroll position value. However, when the order list view is scrolled all the way to the bottom, DF handles these actions by pushing orders into view at the *top* of the list view. Since a new order is now at the top of the list view, we would expect that DF should have updated the reported scroll position, but it does not. The lack of scroll position update breaks our expectation that the reported scroll position should match the index of the first displayed order. If N orders have been removed and M order rows have been added to the order list view (after having scrolled to the bottom of the order list), our calculated order_idx will be N+M too high (causing mismatched descriptions in the confirmation dialogs, and out-of-bounds errors that entirely prevent confirmation when acting on the last N+M orders!). When there are at least as many orders as the height of the orders list view, DF seems to always keep the bottom of the list view populated. Assume this is will be case and adjust our order_idx calculation when the reported scroll position would put the effective index of the last order out of bounds. If DF's order list view ever changes to displaying empty order rows even when there are orders that could be "pushed in" from the top, this will need to be revisited.
Handle possibly out-of-bounds order index. Factor out call to material description generation.
Let order removal confirmation show the specific variety of
- shield (shield, buckler), and
- helm (helm, cap, hood),
- gloves (gauntlets, gloves, mittens),
- shoes (shoes, high boots, low boots, socks),
- ammo (bolt),
- trap component (axe blade, corkscrew, ball, disc, spike), and
- meal (easy, fine, lavish).
Most of these were previously described using the "generic" variety
("Gloves" for gauntlets, "Shoes" for socks, etc.), but meals looked
particularly odd since they were previously described as "Coral", "Green
Glass", and "Clear Glass" "Prepare Meal".
`itemdefs.food` is not used because those list the final item names
(biscuits, stew, roast), not the meal "type" (easy, fine, lavish).
Pausing a confirmation currently pauses all confirmations, not just other instances of the current confirmation. For example, when trading, pausing a Mark All confirmation will also skip confirmation of the Seize action. The prompt in showYesNoPrompt is "Pause this confirmation". To better match that description, only pause new occurrences of the current confirmation.
confirm.lua
Outdated
| return false | ||
| end | ||
| local scr = dfhack.gui.getDFViewscreen(true) | ||
| for id, conf in pairs(specs.REGISTRY) do | ||
| if specs.config.data[id].enabled and self:matches_conf(conf, keys, scr) then | ||
| if conf == self.paused_conf then |
There was a problem hiding this comment.
Wouldn't this now only allow pausing one confirmation per screen instead of all of them?
Incidentally, I agree the pause behavior should not affect all confirmations on the same screen - that's how the original implementation worked too.
There was a problem hiding this comment.
Yes, that change does make it so that only one confirmation can effectively be paused at a time: pausing a confirmation would reenable any previously paused confirmation.
Would you prefer it be changed to allow multiple simultaneous (individually) paused confirmations? For example, during trading, "unmark all fort goods" and "offer as gift" could both be paused, inhibiting those confirmations, without inhibiting confirmations for "seize", "trade", or other trade actions.
Or I could tighten up my description (commit and changelog) to indicate that pauses are mutually exclusive.
There was a problem hiding this comment.
I do not think pauses should be mutually exclusive. If someone wants to stop seeing more than one confirmation temporarily, they should be able to do so. This is how the feature was originally implemented.
There was a problem hiding this comment.
Actually, I might be confusing this with the original "settings" screen, which let you quickly disable the currently-visible confirmation, not pause it. It does appear that the old "pause" logic only paused the confirmation for the lifetime of the screen - this is a little harder to track now that DF doesn't use full screens for much.
I still feel like the behavior where pausing a second confirmation unpauses the first one is unintuitive, though.
There was a problem hiding this comment.
Having multiple, independently paused confirmations does seem like it would be less surprising.
The initial change was a minimal code change to keep one paused confirmation from affecting all confirmations in the same context. I failed to explore better functionality that could be arranged with a bit more code.
Thanks!
Allowing only a single, mutually exclusive, paused confirmation was likely to be confusing. Per review from lethosor.
The individual commits here are mostly independent and the commit messages give descriptions and motivations.
The last "order-remove descriptions" commit (4bce233) depends on its parent (a small refactoring), but could be reworked without it.
The last commit (fb25d4d: only pause specific confirmations) is a bit speculative. The motivating example is that pausing any of
trade-{un,}mark-all-{fort,merchant} will also pausetrade-seizesince they all share the same context. Trading and hauling seem to be the only pausable confirmations (after 435fcd0) with shared contexts. Does this seem reasonable for both of them? There is a large disparity in the ramifications of an unintentionally skipped seize/offer/trade action versus the un/mark-all trade confirmations, so it seems important there. Maybe it is a good idea for hauling too, since the buttons for deleting a stop and a whole route are right next to each other?In the changelog for this "pause specific confirmations" change, I used the words "in the current context", but that probably does not mean much to players (and the underlying focus strings are not easy to observe). Not sure what can be done to better communicate this (maybe some visible widget that indicates when and which confirmations are paused?).
Similar fixes to the "stale scroll position" commit (ded125a) definitely need to be made in some other places, too. Of the other
scroll_positions ininternal/confirm/specs:mi.hotkey.scroll_positiongoes stale when scrolled to the bottom and the window gains height, and does cause similar index confusionmi.assign_uniform.scroll_positionseems immune to this since the displayed list doesn't seem to change size with window height (even when its 9 "uniform rows" don't fit in the available area)There are plenty of other
scroll_positionhits in the type definitions, but there are many fewer in the Lua files (and none in the C++ files?). I'll open a placeholder issue while I investigate these.