Skip to content
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

gui/launcher pause handling changes #1128

Merged
merged 5 commits into from
Jun 11, 2024

Conversation

lethosor
Copy link
Member

No description provided.

if ch == ' ' then return text:match('%S$') end
-- if game was not initially paused, then allow double-space to toggle pause
if ch == ' ' and not self.parent_view.parent_view.saved_pause_state then
return text:sub(1, self.subviews.editfield.cursor - 1):match('%S$')
Copy link
Member

@myk002 myk002 May 24, 2024

Choose a reason for hiding this comment

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

this is better than before since not being able to unpause when you want to is less bad than unpausing when you don't want to, but I worry that this is still too opaque. I like the ideas in #665 for raising the visibility of (and reducing potential confusion about) this feature. We can look into that as a followup.

In the meantime, could you update the gui/launcher docs regarding the pause behavior? Also needs a changelog line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand how #665 would work. Would we conditionally hide/show the "Space: pause/unpause" label if space is going to have an effect? When it's hidden, it would be just as opaque as it is now, IMO.

I was hoping to gather some testing feedback before going through the docs updates. Will follow up on Discord.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand how #665 would work.

me neither, which is why I never merged it, but something along the proposed lines of "make the current state and potential state changes more visible and accessible" seems like a good idea.

FWIW I'm fine with this change as written, at least until we can find a way to improve the overall UX

@Bumber64
Copy link
Contributor

Bumber64 commented May 26, 2024

Pausing is handled by keybind D_PAUSE, right? It's not a given that's space, or even a printable character. It can be bound to more than one key.

@lethosor
Copy link
Member Author

That is true, and we should look at making that logic more robust, but I think changing that is outside the scope of this PR.

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

needs doc update for pause behavior

@myk002 myk002 merged commit e2b9432 into DFHack:master Jun 11, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants