From 4cc1d2ff9a9820ee7bc8dde44fb614e7e279a5d9 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sun, 24 May 2026 14:09:08 +0200 Subject: [PATCH] Fixed a potential crash This bug was triggered during development and happened because the tool bar build code was invalidating QAction objects indirectly while they are used for building the main menu. Solution is to separate building of menu bar and tool bar / context menus. --- src/laybasic/laybasic/layAbstractMenu.cc | 224 ++++++++++++----------- 1 file changed, 121 insertions(+), 103 deletions(-) diff --git a/src/laybasic/laybasic/layAbstractMenu.cc b/src/laybasic/laybasic/layAbstractMenu.cc index 637f9ab8f..3ab6d1c06 100644 --- a/src/laybasic/laybasic/layAbstractMenu.cc +++ b/src/laybasic/laybasic/layAbstractMenu.cc @@ -1280,32 +1280,139 @@ AbstractMenu::extra_menu_name () void AbstractMenu::build (QMenuBar *mbar, QToolBar *tbar) { - if (tbar) { - tbar->clear (); - } - - std::set > present_actions; + // Build the menu bar if (mbar) { + std::set > present_actions; + QList a = mbar->actions (); for (QList::const_iterator i = a.begin (); i != a.end (); ++i) { present_actions.insert (std::make_pair (id_from_action (*i), *i)); } + // NOTE: on MacOS, once the top level menu is created, we should not + // modify it and place menu items into the "Extras" menu instead. + bool menu_frozen = ! s_can_move_menu && ! present_actions.empty (); + + // NOTE: the "extras" menu is relevant on MacOS only + QMenu *extras_menu = find_extras_menu (mbar); + if (extras_menu) { + extras_menu->clear (); + } + + QAction *prev_action = 0; + + for (std::list::iterator c = m_root.children.begin (); c != m_root.children.end (); ++c) { + + if (c->has_submenu ()) { + + if (c->name ().find ("@") == 0) { + + // done later. + + } else { + + if (c->menu () == 0) { + + // NOTE: we intentionally do not make the item owner of the menu action + // as implicitly deleting it might cause trouble on MacOS. Instead we + // explicitly delete below or keep the action. + QMenu *menu = new QMenu (mp_dispatcher->menu_parent_widget ()); + menu->setTitle (tl::to_qstring (c->action ()->get_title ())); + c->set_action (new Action (menu, false), true); + + // This case happens when we dynamically create menus. + // MacOS does not like generating top-level menus dynamically, so + // we put them into the "_extra" top level one. + if (menu_frozen) { + if (extras_menu) { + extras_menu->addMenu (menu); + } + } else { + prev_action = insert_action_after (mbar, prev_action, menu->menuAction ()); + } + + } else { + + // Move the action to the end if present in the menu already + std::set >::iterator a = present_actions.find (std::make_pair (id_from_action (c->menu ()->menuAction ()), c->menu ()->menuAction ())); + if (a != present_actions.end ()) { + if (s_can_move_menu) { + mbar->removeAction (a->second); + insert_action_after (mbar, prev_action, a->second); + } + prev_action = a->second; + present_actions.erase (*a); + } else if (menu_frozen) { + if (extras_menu) { + extras_menu->addMenu (c->menu ()); + } + } else { + prev_action = insert_action_after (mbar, prev_action, c->menu ()->menuAction ()); + } + + } + + // NOTE: the "extras" menu is built implicitly. You cannot put anything there. + if (c->menu () != extras_menu) { + build (c->menu (), c->children); + } + + } + + } else { + + // Move the action to the end if present in the menu already + std::set >::iterator a = present_actions.find (std::make_pair (id_from_action (c->action ()->qaction ()), c->action ()->qaction ())); + if (a != present_actions.end ()) { + if (s_can_move_menu) { + mbar->removeAction (a->second); + insert_action_after (mbar, prev_action, a->second); + } + prev_action = a->second; + present_actions.erase (*a); + } else if (menu_frozen) { + QMenu *extras_menu = find_extras_menu (mbar); + if (extras_menu) { + extras_menu->addAction (c->action ()->qaction ()); + } + } else { + prev_action = insert_action_after (mbar, prev_action, c->action ()->qaction ()); + } + + } + + } + + // Disable the (maybe new) "extras" menu if empty + extras_menu = find_extras_menu (mbar); + if (extras_menu) { + extras_menu->setEnabled (! extras_menu->isEmpty ()); + } + + // Remove all actions that have vanished + for (std::set >::iterator a = present_actions.begin (); a != present_actions.end (); ++a) { + if (s_can_move_menu) { + mbar->removeAction (a->second); + delete a->second; + } else { + if (a->second->menu ()) { + a->second->menu ()->clear (); + } + a->second->setEnabled (false); + } + } + } - // NOTE: on MacOS, once the top level menu is created, we should not - // modify it and place menu items into the "Extras" menu instead. - bool menu_frozen = ! s_can_move_menu && ! present_actions.empty (); + // Finally build the context menus and toolbar. + // NOTE: this is done afterwards, as generating the submenus, specifically for the toolbar menu buttons + // messes with the QActions stored in "present_actions". - // NOTE: the "extras" menu is relevant on MacOS only - QMenu *extras_menu = find_extras_menu (mbar); - if (extras_menu) { - extras_menu->clear (); + if (tbar) { + tbar->clear (); } - QAction *prev_action = 0; - for (std::list::iterator c = m_root.children.begin (); c != m_root.children.end (); ++c) { if (c->has_submenu ()) { @@ -1338,100 +1445,11 @@ AbstractMenu::build (QMenuBar *mbar, QToolBar *tbar) // prepare a detached menu which can be used as context menus build (c->menu (), c->children); - } else if (mbar) { - - if (c->menu () == 0) { - - // NOTE: we intentionally do not make the item owner of the menu action - // as implicitly deleting it might cause trouble on MacOS. Instead we - // explicitly delete below or keep the action. - QMenu *menu = new QMenu (mp_dispatcher->menu_parent_widget ()); - menu->setTitle (tl::to_qstring (c->action ()->get_title ())); - c->set_action (new Action (menu, false), true); - - // This case happens when we dynamically create menus. - // MacOS does not like generating top-level menus dynamically, so - // we put them into the "_extra" top level one. - if (menu_frozen) { - if (extras_menu) { - extras_menu->addMenu (menu); - } - } else { - prev_action = insert_action_after (mbar, prev_action, menu->menuAction ()); - } - - } else { - - // Move the action to the end if present in the menu already - std::set >::iterator a = present_actions.find (std::make_pair (id_from_action (c->menu ()->menuAction ()), c->menu ()->menuAction ())); - if (a != present_actions.end ()) { - if (s_can_move_menu) { - mbar->removeAction (a->second); - insert_action_after (mbar, prev_action, a->second); - } - prev_action = a->second; - present_actions.erase (*a); - } else if (menu_frozen) { - if (extras_menu) { - extras_menu->addMenu (c->menu ()); - } - } else { - prev_action = insert_action_after (mbar, prev_action, c->menu ()->menuAction ()); - } - - } - - // NOTE: the "extras" menu is built implicitly. You cannot put anything there. - if (c->menu () != extras_menu) { - build (c->menu (), c->children); - } - - } - - } else if (mbar) { - - // Move the action to the end if present in the menu already - std::set >::iterator a = present_actions.find (std::make_pair (id_from_action (c->action ()->qaction ()), c->action ()->qaction ())); - if (a != present_actions.end ()) { - if (s_can_move_menu) { - mbar->removeAction (a->second); - insert_action_after (mbar, prev_action, a->second); - } - prev_action = a->second; - present_actions.erase (*a); - } else if (menu_frozen) { - QMenu *extras_menu = find_extras_menu (mbar); - if (extras_menu) { - extras_menu->addAction (c->action ()->qaction ()); - } - } else { - prev_action = insert_action_after (mbar, prev_action, c->action ()->qaction ()); } } } - - // Disable the (maybe new) "extras" menu if empty - extras_menu = find_extras_menu (mbar); - if (extras_menu) { - extras_menu->setEnabled (! extras_menu->isEmpty ()); - } - - // Remove all actions that have vanished - if (mbar) { - for (std::set >::iterator a = present_actions.begin (); a != present_actions.end (); ++a) { - if (s_can_move_menu) { - mbar->removeAction (a->second); - delete a->second; - } else { - if (a->second->menu ()) { - a->second->menu ()->clear (); - } - a->second->setEnabled (false); - } - } - } } void