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

Fix confirm hover instructions #1333

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Conversation

robob27
Copy link
Contributor

@robob27 robob27 commented Nov 11, 2024

The df.main_hover_instruction enum was updated here. Updating references to this enum in confirm to fix some confirm dialogs not working due to referencing the old enum values.

@robob27 robob27 force-pushed the robob27/confirm-hover-fix branch from 1e79dda to ca2090e Compare November 11, 2024 01:18
@robob27 robob27 force-pushed the robob27/confirm-hover-fix branch from ca2090e to d62447a Compare November 11, 2024 01:19
@myk002
Copy link
Member

myk002 commented Nov 11, 2024

hrm... another option would be to revert the name changes for the enum. @lethosor what do you think?

@ab9rf
Copy link
Member

ab9rf commented Nov 11, 2024

I support staying with the canonical names going forward. Given that we've already made all the effort to move to canonical names, sticking with them going forward seems more reasonable than going back to the noncanonical ones we were using before. And Bay12 will no doubt make further changes, for which having our usage match theirs will make updating that much easier.

Reverting also removes a lot of information; previously we had only 33 of the 569 entries named; now all 601 of them are named, which is incontrovertibly more useful. Seems like an obvious choice to me.

@myk002
Copy link
Member

myk002 commented Nov 11, 2024

fair enough. there is an issue that the names used in df-structures master are now different from those in df-structures adv-beta, (so merging this PR as-is will break DFHack/develop) but we can get around that with code like

    predicate=function() return mi.current_hover == (df.main_hover_instruction.RouteRemove or df.main_hover_instruction.HAULING_REMOVE_ROUTE) end,

or by merging the canonical names to master

@ab9rf
Copy link
Member

ab9rf commented Nov 11, 2024

We should probably just canonicalize df-structures for 50.14 as well. Shouldn't take long. I'll do it in the morning if nobody gets to it before me.

@ab9rf
Copy link
Member

ab9rf commented Nov 11, 2024

I pulled the diff between 50.14 and 51.01-beta24 and the only difference is the absence of the CONTEXT_ADVENTURE entries, so porting across is cut and paste. PR will issue shortly.

ab9rf added a commit to ab9rf/df-structures that referenced this pull request Nov 11, 2024
updates hover instructions to canonical names

based on 04e2b18 by @quietust

this will allow DFHack/scripts#1333 to be merged to 50.14 develop

Co-Authored-By: Quietust <[email protected]>
changelog.txt Outdated
@@ -38,6 +38,7 @@ Template for new versions:
- `makeown`: halt any hostile jobs the unit may be engaged in, like kidnapping
- `fix/loyaltycascade`: allow the fix to work on non-dwarven citizens
- `control-panel`: fix setting numeric preferences from the commandline
- `gui/confirm`: fix some confirm prompts not working
Copy link
Member

Choose a reason for hiding this comment

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

no need for a changelog since there hasn't been a stable release with this broken yet

changelog.txt Outdated Show resolved Hide resolved
@myk002 myk002 merged commit 3472126 into DFHack:master Nov 11, 2024
10 checks passed
@lethosor
Copy link
Member

Yeah, I agree with ab9rf. I wouldn't have renamed them personally, but now that they have been renamed, it should make updates easier going forward.

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.

4 participants