Interface Kit: BWindow owns (and deletes) menu sem

_Uninstall() before _Hide() in BMenu because the window must
be available when we _Uninstall() especially for shortcuts.

Remove #define and always assume USE_CACHED_MENUWINDOW.

_Uninstall() whenever we _Hide() in BMenu.

Change-Id: I5dae85f6edf1f0b4ccf67a6d9d77470576671cee
Reviewed-on: https://review.haiku-os.org/c/haiku/+/9012
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>

headers/os/interface/MenuBar.h

@@ -106,10 +106,9 @@ private:
 			menu_bar_border		fBorder;
 			thread_id			fTrackingPID;
 			int32				fPrevFocusToken;
-			sem_id				fMenuSem;
 			BRect*				fLastBounds;
 			uint32				fBorders;
-			uint32				_reserved[1];
+			uint32				_reserved[2];
 
 			bool				fTracking;
 };

src/kits/interface/Menu.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2018 Haiku, Inc. All rights reserved.
+ * Copyright 2001-2025 Haiku, Inc. All rights reserved.
  * Distributed under the terms of the MIT license.
  *
  * Authors:
@@ -51,10 +51,9 @@
 #include "utf8_functions.h"
 
 
-#define USE_CACHED_MENUWINDOW 1
-
 using BPrivate::gSystemCatalog;
 
+
 #undef B_TRANSLATION_CONTEXT
 #define B_TRANSLATION_CONTEXT "Menu"
 
@@ -1392,8 +1391,8 @@ BMenu::Show(bool selectFirst)
 void
 BMenu::Hide()
 {
-	_Hide();
 	_Uninstall();
+	_Hide();
 }
 
 
@@ -1676,11 +1675,9 @@ BMenu::_Hide()
 	window->DetachMenu();
 		// we don't want to be deleted when the window is removed
 
-#if USE_CACHED_MENUWINDOW
 	if (fSuper != NULL)
 		window->Unlock();
 	else
-#endif
 		window->Quit();
 			// it's our window, quit it
 
@@ -2965,13 +2962,12 @@ BMenu::_OverSubmenu(BMenuItem* item, BPoint loc)
 BMenuWindow*
 BMenu::_MenuWindow()
 {
-#if USE_CACHED_MENUWINDOW
 	if (fCachedMenuWindow == NULL) {
 		char windowName[64];
 		snprintf(windowName, 64, "%s cached menu", Name());
 		fCachedMenuWindow = new (nothrow) BMenuWindow(windowName);
 	}
-#endif
+
 	return fCachedMenuWindow;
 }
 
@@ -3060,17 +3056,15 @@ BMenu::_Uninstall()
 
 
 void
-BMenu::_SelectItem(BMenuItem* item, bool showSubmenu, bool selectFirstItem,
+BMenu::_SelectItem(BMenuItem* item, bool showSubmenu, bool selectFirstItem, bool keyDown)
-	bool keyDown)
 {
-	// Avoid deselecting and then reselecting the same item
+	// Avoid deselecting and reselecting the same item which would cause flickering.
-	// which would cause flickering
 	if (item != fSelected) {
 		if (fSelected != NULL) {
 			fSelected->Select(false);
 			BMenu* subMenu = fSelected->Submenu();
 			if (subMenu != NULL && subMenu->Window() != NULL)
-				subMenu->_Hide();
+				subMenu->Hide();
 		}
 
 		fSelected = item;
@@ -3458,7 +3452,7 @@ BMenu::_QuitTracking(bool onlyThis)
 		}
 	}
 
-	_Hide();
+	Hide();
 }
 
 

src/kits/interface/MenuBar.cpp

@@ -52,7 +52,6 @@ BMenuBar::BMenuBar(BRect frame, const char* name, uint32 resizingMode,
 	fBorder(B_BORDER_FRAME),
 	fTrackingPID(-1),
 	fPrevFocusToken(-1),
-	fMenuSem(-1),
 	fLastBounds(NULL),
 	fTracking(false)
 {
@@ -68,7 +67,6 @@ BMenuBar::BMenuBar(const char* name, menu_layout layout, uint32 flags)
 	fBorder(B_BORDER_FRAME),
 	fTrackingPID(-1),
 	fPrevFocusToken(-1),
-	fMenuSem(-1),
 	fLastBounds(NULL),
 	fTracking(false)
 {
@@ -82,7 +80,6 @@ BMenuBar::BMenuBar(BMessage* archive)
 	fBorder(B_BORDER_FRAME),
 	fTrackingPID(-1),
 	fPrevFocusToken(-1),
-	fMenuSem(-1),
 	fLastBounds(NULL),
 	fTracking(false)
 {
@@ -492,11 +489,9 @@ BMenuBar::StartMenuBar(int32 menuIndex, bool sticky, bool showMenu,
 	// so let's call MenusBeginning() directly
 	window->MenusBeginning();
 
-	fMenuSem = create_sem(0, "window close sem");
+	sem_id sem = create_sem(0, "window close sem");
-	_set_menu_sem_(window, fMenuSem);
+	_set_menu_sem_(window, sem);
-
+	fTrackingPID = spawn_thread(_TrackTask, "menu_tracking", B_DISPLAY_PRIORITY, NULL);
-	fTrackingPID = spawn_thread(_TrackTask, "menu_tracking",
-		B_DISPLAY_PRIORITY, NULL);
 	if (fTrackingPID >= 0) {
 		menubar_data data;
 		data.menuBar = this;
@@ -512,7 +507,6 @@ BMenuBar::StartMenuBar(int32 menuIndex, bool sticky, bool showMenu,
 	} else {
 		fTracking = false;
 		_set_menu_sem_(window, B_NO_MORE_SEMS);
-		delete_sem(fMenuSem);
 	}
 }
 
@@ -540,8 +534,6 @@ BMenuBar::_TrackTask(void* arg)
 	window->PostMessage(_MENUS_DONE_);
 
 	_set_menu_sem_(window, B_BAD_SEM_ID);
-	delete_sem(menuBar->fMenuSem);
-	menuBar->fMenuSem = B_BAD_SEM_ID;
 
 	return 0;
 }

src/kits/interface/PopUpMenu.cpp

@@ -375,7 +375,6 @@ BPopUpMenu::_Go(BPoint where, bool autoInvoke, bool startOpened,
 	fTrackThread = spawn_thread(_thread_entry, "popup", B_DISPLAY_PRIORITY, data);
 	if (fTrackThread < B_OK) {
 		// Something went wrong. Cleanup and return NULL
-		delete_sem(sem);
 		if (async && window != NULL)
 			_set_menu_sem_(window, B_BAD_SEM_ID);
 		delete data;
@@ -409,8 +408,6 @@ BPopUpMenu::_thread_entry(void* menuData)
 	if (data->async && data->window)
 		_set_menu_sem_(data->window, B_BAD_SEM_ID);
 
-	delete_sem(data->lock);
-
 	// Commit suicide if needed
 	if (data->async && menu->fAutoDestruct) {
 		menu->fTrackThread = -1;

src/kits/interface/Window.cpp

@@ -217,8 +217,23 @@ static value_info sWindowValueInfo[] = {
 void
 _set_menu_sem_(BWindow* window, sem_id sem)
 {
-	if (window != NULL)
+	if (window == NULL)
-		window->fMenuSem = sem;
+		return;
+
+	// delete semaphore when set to invalid
+	switch (sem) {
+		case B_BAD_SEM_ID:
+		case B_NO_MORE_SEMS:
+		case -1:
+			if (window->fMenuSem > 0)
+				delete_sem(window->fMenuSem);
+			break;
+
+		default:
+			break;
+	}
+
+	window->fMenuSem = sem;
 }