From 7458912b3340a428cd5fcb763859659ebb8b76ce Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Mon, 14 Mar 2022 16:28:18 +0800 Subject: [PATCH] Gui: fix shortcut context handling in ShortcutManager Related #6097 Qt ignores shortcut of actions in invisible toolbar, but not for actions in a hidden menu action of menu bar, which is likely a Qt bug. The desired behavior should be that of toolbar actions, so that actions belong to different workbenches can have the same shortcut without conflict. This commit works around this inconsistency by ensuring only the active actions are added in menu bar. In addition, all active actions will be added to a zero sized child widget of the main window, which ensures the shortcuts of these actions being active regardless whether the action is in toolbar or menu bar, visible or not. --- src/Base/ConsoleObserver.cpp | 2 ++ src/Gui/MenuManager.cpp | 26 ++++++++++++++++++++++ src/Gui/ShortcutManager.cpp | 42 +++++++++++++++++------------------- src/Gui/ToolBarManager.cpp | 13 ++++++++++- src/Gui/ToolBarManager.h | 2 +- 5 files changed, 61 insertions(+), 24 deletions(-) diff --git a/src/Base/ConsoleObserver.cpp b/src/Base/ConsoleObserver.cpp index fdc23f7167..f5a26d9ae8 100644 --- a/src/Base/ConsoleObserver.cpp +++ b/src/Base/ConsoleObserver.cpp @@ -34,6 +34,7 @@ #include #include "ConsoleObserver.h" +#include "Interpreter.h" using namespace Base; @@ -261,6 +262,7 @@ std::stringstream &LogLevel::prefix(std::stringstream &str, const char *src, int } if (print_tag) str << '<' << tag << "> "; if (print_src==2) { + Base::PyGILStateLocker lock; PyFrameObject* frame = PyEval_GetFrame(); if (frame) { line = PyFrame_GetLineNumber(frame); diff --git a/src/Gui/MenuManager.cpp b/src/Gui/MenuManager.cpp index 059b5f949e..6511ddcb41 100644 --- a/src/Gui/MenuManager.cpp +++ b/src/Gui/MenuManager.cpp @@ -209,6 +209,7 @@ void MenuManager::setup(MenuItem* menuItems) const QMenuBar* menuBar = getMainWindow()->menuBar(); +#if 0 #if defined(FC_OS_MACOSX) && QT_VERSION >= 0x050900 // Unknown Qt macOS bug observed with Qt >= 5.9.4 causes random crashes when viewing reused top level menus. menuBar->clear(); @@ -222,6 +223,31 @@ void MenuManager::setup(MenuItem* menuItems) const ("User parameter:BaseApp/Preferences/MainWindow")->GetBool("ClearMenuBar",false)) { menuBar->clear(); } +#else + // In addition to the reason described in the above comments, there is + // another more subtle one that's making clearing menu bar a necessity for + // all platforms. + // + // By right, it should be fine for more than one command action having the + // same shortcut but in different workbench. It should not require manual + // conflict resolving in this case, as the action in an inactive workbench + // is expected to be inactive as well, or else user may experience + // seemingly random shortcut miss firing based on the order he/she + // switches workbenches. In fact, this may be considered as an otherwise + // difficult to implement feature of context aware shortcut, where a + // specific shortcut can activate different actions under different + // workbenches. + // + // This works as expected for action adding to a toolbar. As Qt will ignore + // actions inside an invisible toolbar. However, Qt refuse to do the same + // for actions in a hidden menu action of a menu bar. This is very likely a + // Qt bug, as the behavior does not seem to conform to Qt's documentation + // of Qt::ShortcutContext. + // + // Clearing the menu bar, and recreate it every time when switching + // workbench with only the active actions can solve this problem. + menuBar->clear(); +#endif QList items = menuItems->getItems(); QList actions = menuBar->actions(); diff --git a/src/Gui/ShortcutManager.cpp b/src/Gui/ShortcutManager.cpp index 510570b380..e56ff25b01 100644 --- a/src/Gui/ShortcutManager.cpp +++ b/src/Gui/ShortcutManager.cpp @@ -203,37 +203,35 @@ bool ShortcutManager::checkShortcut(QObject *o, const QKeySequence &key) if (iter == index.end()) return false; - auto it = iter; - // skip to the first not exact matched key - for (; it != index.end() && key == it->key.shortcut; ++it); + // disable and enqueue the action in order to try other alternativeslll + action->setEnabled(false); + pendingActions.emplace_back(action, key.count(), 0); // check for potential partial match, i.e. longer key sequences bool flush = true; - for (; it != index.end(); ++it) { + bool found = false; + for (auto it = iter; it != index.end(); ++it) { if (key.matches(it->key.shortcut) == QKeySequence::NoMatch) break; - if (it->action && it->action->isEnabled()) { + if (action == it->action) { + // There maybe more than one action with the exact same shortcut. + // However, we only disable and enqueue the triggered action. + // Because, QAction::isEnabled() does not check if the action is + // active under its current ShortcutContext. We would have to check + // its parent widgets visibility which may or may not be reliable. + // Instead, we rely on QEvent::Shortcut to be sure to enqueue only + // active shortcuts. We'll fake the current key sequence below, + // which will trigger all possible matches one by one. + pendingActions.back().priority = getPriority(it->key.name); + found = true; + } + else if (it->action && it->action->isEnabled()) { flush = false; - break; + if (found) + break; } } - int count = 0; - for (it = iter; it != index.end() && key == it->key.shortcut; ++it) { - if (it->action && it->action->isEnabled()) { - if (!flush) { - // temporary disable the action so that we can try potential - // match with further keystrokes. - it->action->setEnabled(false); - } - pendingActions.emplace_back(it->action, key.count(), getPriority(it->key.name)); - ++count; - } - } - if (!count) { - // action not found in the map, shouldn't happen! - pendingActions.emplace_back(action, key.count(), 0); - } if (flush) { // We'll flush now because there is no potential match with further // keystrokes, so no need to wait for timer. diff --git a/src/Gui/ToolBarManager.cpp b/src/Gui/ToolBarManager.cpp index f285ef98dd..7709af19ff 100644 --- a/src/Gui/ToolBarManager.cpp +++ b/src/Gui/ToolBarManager.cpp @@ -26,6 +26,7 @@ # include # include # include +# include #endif #include "ToolBarManager.h" @@ -56,7 +57,7 @@ void ToolBarItem::setCommand(const std::string& name) _name = name; } -std::string ToolBarItem::command() const +const std::string & ToolBarItem::command() const { return _name; } @@ -178,6 +179,14 @@ void ToolBarManager::setup(ToolBarItem* toolBarItems) if (!toolBarItems) return; // empty menu bar + static QPointer _ActionWidget; + if (!_ActionWidget) + _ActionWidget = new QWidget(getMainWindow()); + else { + for (auto action : _ActionWidget->actions()) + _ActionWidget->removeAction(action); + } + saveState(); this->toolbarNames.clear(); @@ -222,6 +231,8 @@ void ToolBarManager::setup(ToolBarItem* toolBarItems) // setup the toolbar setup(*it, toolbar); + for (auto action : toolbar->actions()) + _ActionWidget->addAction(action); // try to add some breaks to avoid to have all toolbars in one line if (toolbar_added) { diff --git a/src/Gui/ToolBarManager.h b/src/Gui/ToolBarManager.h index b85eb56c91..d2486e0162 100644 --- a/src/Gui/ToolBarManager.h +++ b/src/Gui/ToolBarManager.h @@ -45,7 +45,7 @@ public: ~ToolBarItem(); void setCommand(const std::string&); - std::string command() const; + const std::string &command() const; bool hasItems() const; ToolBarItem* findItem(const std::string&);