-
Notifications
You must be signed in to change notification settings - Fork 211
confirm
fixes: pausing, Escape from uniform, order-remove descriptions
#1446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ChrisJohnsen
wants to merge
8
commits into
DFHack:master
Choose a base branch
from
ChrisJohnsen:cj/confirm
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
435fcd0
confirm: only show pause option for pausable confirmations
ChrisJohnsen 2a73f93
confirm: handle LEAVESCREEN in uniform-discard-changes
ChrisJohnsen fe09c12
confirm: use interface rect for order-remove calculations
ChrisJohnsen ded125a
confirm: order-remove: work around stale scroll position
ChrisJohnsen fc3e300
confirm: rework order-remove description generation
ChrisJohnsen 4bce233
confirm: more specific order-remove descriptions
ChrisJohnsen fb25d4d
confirm: only pause specific confirmations
ChrisJohnsen d73e2dc
confirm: allow multiple confirmations to be paused
ChrisJohnsen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!