Replace activity ACT_ADV_INVENTORY with uistate.open_menu, remove ACT_VIEW_RECIPE

[Brambor]

Summary

None

Purpose of change

• Fix view recipe works only once per opened Advanced Inventory #62996
• Using "View recipe" in AIM no longer takes an in-game second. It takes no in-game time, as opening a menu should.

Describe the solution

1. Stare at the code for two whole days.
2. Remove ACT_VIEW_RECIPE, just run the menu code directly, as it is just opening a menu over AIM, which is fine.
3. Replace activity ACT_ADV_INVENTORY with uistate.open_menu = create_advanced_inv.
4. Remove legacy code and migrate activity to null.

Describe alternatives you've considered

They were all stupid.

I could also make this as two PRs. There would be a conflict on the save_legacy though.

Side effect

AIM doesn't open when it should after the save/load cycle if autosave is triggered during the "return to AIM" activity - see testing. This is fine.

Testing

Test fix:

• See the reproduction steps in view recipe works only once per opened Advanced Inventory #62996

Test temp_hide_advanced_inv

1. have a lighter
2. in AIM examine > activate
3. The AIM hid, as expected

Test migration
What I did:

1. Load the save in the old version.
2. Set saving interval to 0 min and save after 10 actions.
3. / Open AIM.
4. examine machete.
5. View recipe. (repeat 10 times)
   • The autosave will save during the activity.
6. alt+F4

There is the save:
TEST migration AIM & VIEW.zip

Note it has ACT_ADV_INVENTORY and ACT_VIEW_RECIPE in it in the .sav file

On master:

1. Load save.
2. It opens AIM after loading.

In the new version:

1. Load save.
2. AIM doesn't open and the game doesn't complain about missing ACT_ADV_INVENTORY or ACT_VIEW_RECIPE.

Additional context

[mqrause]

Are you interested in giving ACT_PICKUP_MENU the same treatment (if it's possible)?

[Brambor]

Are you interested in giving ACT_PICKUP_MENU the same treatment (if it's possible)?

Yup, I will go through them and see what I can do!

It is more time-consuming than it should be, as I try to ensure I didn't break anything...

[Brambor]

I realized I didn't test temp_hide, so I will do that tomorrow.

Edit: Yeah, it works as expected.

[PatrikLundell]

I'm all in favor of spending time to ensure things aren't broken...

[Brambor]

@andrei8l Do you know if this will still be needed?

Cataclysm-DDA/src/do_turn.cpp

Lines 534 to 537 in d5e57fa

	// FIXME: hack needed due to the legacy code in advanced_inventory::move_all_items()
	if( !u.activity ) {
	kill_advanced_inv();
	}

My quick testing at removing it didn't break anything obvious...

[andrei8l]

@andrei8l Do you know if this will still be needed?

Cataclysm-DDA/src/do_turn.cpp

Lines 534 to 537 in d5e57fa

	// FIXME: hack needed due to the legacy code in advanced_inventory::move_all_items()
	if( !u.activity ) {
	kill_advanced_inv();
	}

My quick testing at removing it didn't break anything obvious...

Sorry, I don't remember. Kamayana has fixed a lot of bugs in move_all_items(() since then and it's possible that this hack isn't needed anymore.

[Brambor]

I don't plan to work on it then. However, there is the patch:

commit 68af3324a0e5c074039f661a4ab3a0fd0e037ac7
Author: Brambor <[email protected]>
Date:   Tue Sep 17 14:35:25 2024 +0200

    TODO remove unneded ? kill_advanced_inv

diff --git a/src/advanced_inv.cpp b/src/advanced_inv.cpp
index 44fa35ae8a..235c4d2418 100644
--- a/src/advanced_inv.cpp
+++ b/src/advanced_inv.cpp
@@ -117,6 +117,12 @@ namespace
 std::unique_ptr<advanced_inventory> advinv;
 } // namespace
 
+void kill_advanced_inv()
+{
+    advinv.reset();
+    cancel_aim_processing();
+}
+
 void create_advanced_inv()
 {
     if( !advinv ) {
@@ -129,12 +135,6 @@ void create_advanced_inv()
     }
 }
 
-void kill_advanced_inv()
-{
-    advinv.reset();
-    cancel_aim_processing();
-}
-
 void temp_hide_advanced_inv()
 {
     if( advinv ) {
diff --git a/src/do_turn.cpp b/src/do_turn.cpp
index 21ff4c985a..64fde0e095 100644
--- a/src/do_turn.cpp
+++ b/src/do_turn.cpp
@@ -531,10 +531,6 @@ bool do_turn()
     while( u.get_moves() > 0 && u.activity ) {
         u.activity.do_turn( u );
     }
-    // FIXME: hack needed due to the legacy code in advanced_inventory::move_all_items()
-    if( !u.activity ) {
-        kill_advanced_inv();
-    }
 
     // Process NPC sound events before they move or they hear themselves talking
     for( npc &guy : g->all_npcs() ) {
diff --git a/src/ui.h b/src/ui.h
index ce63b0b826..780935b6c6 100644
--- a/src/ui.h
+++ b/src/ui.h
@@ -553,7 +553,6 @@ class pointmenu_cb : public uilist_callback
         void select( uilist *menu ) override;
 };
 
-void kill_advanced_inv();
 void temp_hide_advanced_inv();
 
 /**