DFHack / dfhack

Memory hacking library for Dwarf Fortress and a set of tools that use it
Other
1.86k stars 465 forks source link

Search prompt doesn't appear the second time you enter a screen #1725

Closed myk002 closed 3 years ago

myk002 commented 3 years ago

This is preventing quickfort blueprints from using search prompts, which makes stockpile configuration in blueprints much more verbose than it would be otherwise.

It appears to be a race condition since I can only reproduce it by programmatically sending the keys. I've never been able to manually type fast enough to trigger the problem.

Here's a blueprint that shows the search prompt working the first time you enter a screen:

#query
{animalsprefix}e{Right}s

Save this text to a file named, say, searchtest.csv in the blueprints/ directory in your DF installation. Run the blueprint via quickfort run searchtest.csv at the DFHack# prompt.

The blueprint will end with the cursor active in the search widget. quickfort will report an error since we didn't end up on the main screen, but that's expected here.

And here's a blueprint that shows the search prompt missing when you enter a screen for a second time:

#query
{animalsprefix}e{Right}^{animalsprefix}{Right}s

The search prompt doesn't even appear.

Looking through search.cpp, it's not immediately obvious why this is happening. My suspicion is that when screens are entered and exited so quickly, the wrong screen is detected at some point.

This is an ancient bug. I've been running into it for years, though this is the first time I've investigated enough to discover the missing search prompt. No need to block release, but I'd appreciate any insights you might have about debugging the issue.

lethosor commented 3 years ago

In the second case, there is an extra (dismissed) viewscreen_layer_stockpilest on the screen stack, presumably the one that was dismissed earlier. DF takes a tick to clean these up, and only cleans up ones at the top of the stack. I believe the search plugin has some logic that ties a search instance ("module") to a specific instance of a screen, and doesn't allow it to appear on more than one screen at a time, so it's probably only appearing on the first instance of the screen.

lethosor commented 3 years ago

It looks to me like changing is_live_screen() to exclude dismissed screens seems to trigger an update elsewhere in the search plugin, and makes your second blueprint work on my end:

--- a/plugins/search.cpp
+++ b/plugins/search.cpp
@@ -97,7 +97,7 @@ void make_text_dim(int x1, int x2, int y)
 static bool is_live_screen(const df::viewscreen *screen)
 {
     for (df::viewscreen *cur = &gview->view; cur; cur = cur->child)
-        if (cur == screen)
+        if (cur == screen && cur->breakdown_level == interface_breakdown_types::NONE)
             return true;
     return false;
 }

Unsure if this has side-effects yet.

myk002 commented 3 years ago

Thank you for the analysis! How can we test for side effects? I can write a blueprint that enters and exits the screen repeatedly, doing a search and changing state each time.

myk002 commented 3 years ago

For example, this blueprint now works with your proposed change

#query
{animalsprefix}eb{Right}stoad&&^{animalsprefix}{Right}scrow&&^{animalsprefix}{Right}skea&&^{animalsprefix}{Right}sraven&&^{animalsprefix}{Right}scassowaries&&^
lethosor commented 3 years ago

Well, there are a couple places where reset_on_change() is called, and my primary concern was that this change increases the number of situations where search state could be reset. However, the closest I've come to identifying behavior that changed is by creating multiple instances of the same viewscreen (e.g. by feeding BUILDJOB_STOCKPILE_SETTINGS twice to viewscreen_dwarfmodest), manually dismissing the first one, triggering a search in the second, and then "un-dismissing" the first, which is unlikely to occur and isn't what your blueprint is doing. There are still limitations preventing searches from working in two non-dismissed instances of the same viewscreen (i.e. the plugin prevents the search UI from displaying in the second), but that's separate from this issue and hasn't caused any reported issues for users yet.

I tested out a couple more complicated searches (e.g. the trade screen search) and they still seem to be behaving normally, so I'll go ahead with the above change.

myk002 commented 3 years ago

Awesome -- thank you! For my reference, though, how do you un-dismiss a viewscreen? I thought it was a one-way operation..

lethosor commented 3 years ago

This particular screen was in between two non-dismissed screens, so DF didn't clean it up, and I just set its breakdown_level back to 0 manually. (It's not safe, particularly for screens implemented by DFHack, since dismiss() does additional things for those, but it was the way I found to trigger the changed behavior that I was concerned about.)