Gui: introduce ShortcutManager for shortcut management and conflict resolving #6506

realthunder · 2022-03-04T03:46:33Z

Forum post https://forum.freecadweb.org/viewtopic.php?f=17&t=66821

Add new class ShortcutManager to unify shortcut management. Implement through global event filter to support longest key sequence match with user defined delay (configurable through 'Customize -> Keyboard -> Key sequence delay'). Also supports user defined priority to resolve shortcut conflict through 'Customize -> Keyboard'.

Refactored 'Customize -> Keyboard' page for easier command search and better synchronization.

shortcut-manager.mp4

Support longest key sequence match with user defined delay (configurable through 'Customize -> Keyboard -> Key sequence delay'). Support user defined priority to resolve shortcut conflict through 'Customize -> Keyboard') Add 'All' category in 'Customize -> Keyboard' to list all command and showing their shortcuts Unify macro command shortcut setting (BaseApp/Preferences/Shortcut).

By reusing code in DlgCustomKeyboardImp

luzpaz · 2022-03-04T19:25:09Z

This looks awesome!

freecadci · 2022-03-05T09:03:33Z

for feature branch PR_6506. Pipeline 485159467 was triggered at 62fce1c. All CI branches and pipelines.

chennes · 2022-03-05T16:42:15Z

src/Gui/Action.cpp

+                              Action *act)
+{
+    QString text = title;
+    text.remove(QLatin1Char('&'));;


This will remove even "escaped" ampersands (&&).

chennes · 2022-03-05T16:46:16Z

src/Gui/Action.cpp

+public:
+


Suggested change

public:

chennes · 2022-03-05T17:01:00Z

src/Gui/Action.cpp

+        case Qt::DisplayRole:
+        case Qt::EditRole:
+            if (info.text.isEmpty()) {
+#if QT_VERSION>=QT_VERSION_CHECK(5,2,0)


We don't support older versions of Qt anymore, so this should be unnecessary.

chennes · 2022-03-05T17:02:35Z

src/Gui/Action.cpp

+            }
+            return info.tooltip;
+
+        case Qt::UserRole:


I think it's usually clearer to make a constant for the Qt::UserRole with a more descriptive name.

chennes · 2022-03-05T17:04:04Z

src/Gui/Action.cpp

+#if QT_VERSION>=QT_VERSION_CHECK(5,2,0)
+    this->setFilterMode(Qt::MatchContains);
+#endif


Suggested change

#if QT_VERSION>=QT_VERSION_CHECK(5,2,0)

this->setFilterMode(Qt::MatchContains);

#endif

this->setFilterMode(Qt::MatchContains);

chennes · 2022-03-05T17:04:52Z

src/Gui/Action.cpp

+    connect(lineedit, SIGNAL(textEdited(QString)), this, SLOT(onTextChanged(QString)));
+    connect(this, SIGNAL(activated(QModelIndex)), this, SLOT(onCommandActivated(QModelIndex)));


Prefer new-style connections.

Noted. I do use new connection now. These code were added before we fully switch to Qt5.

chennes · 2022-03-05T17:07:19Z

src/Gui/Action.cpp

+                popup()->hide();
+                return true;


Can you refactor to avoid repeating yourself here?

There is subtle difference in handling of these two conditions. The ESC is supposed to clear the line edit text regardless of popup visibility. And if the edit is empty and popup is hidden, the ESC key shouldn't be filtered.

src/Gui/Action.cpp

src/Gui/Command.cpp

chennes · 2022-03-05T17:13:03Z

src/Gui/Command.cpp

+#ifdef FC_DEBUG
+        // Accelerator conflict can now be dynamically resolved in ShortcutManager
+        //
+        // printConflictingAccelerators();
+#endif


Suggested change

#ifdef FC_DEBUG

// Accelerator conflict can now be dynamically resolved in ShortcutManager

//

// printConflictingAccelerators();

#endif

chennes · 2022-03-05T17:23:34Z

src/Gui/CommandView.cpp

+    virtual void activated(int iMsg) override;
+    virtual bool isActive(void) override;
+    virtual Action * createAction(void) override;
+    virtual void languageChange() override;


http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final

Suggested change

virtual void activated(int iMsg) override;

virtual bool isActive(void) override;

virtual Action * createAction(void) override;

virtual void languageChange() override;

void activated(int iMsg) override;

bool isActive(void) override;

Action * createAction(void) override;

void languageChange() override;

chennes · 2022-03-05T17:53:14Z

src/Gui/DlgKeyboardImp.cpp

+    priorityList->header()->setSectionResizeMode(0, QHeaderView::ResizeToContents);
+    priorityList->header()->setSectionResizeMode(1, QHeaderView::ResizeToContents);
+
+    auto updatePriorityList = [priorityList](bool up) {


Using a boolean here feels pretty kludgy. I can live with it here since it's in such an isolated use-case, but I think it would be better to use a more explicit UP/DOWN specifier.

chennes · 2022-03-05T17:55:24Z

src/Gui/DlgKeyboardImp.cpp

+        auto timer = new QTimer(priorityList);
+        timer->setSingleShot(true);
+        if (currentShortcut)
+            QObject::connect(currentShortcut, &QLineEdit::textChanged, timer, [timer](){timer->start(200);});


How did you arrive at the 200ms for these?

Just to prevent refreshing priority list too often, since it gets triggered in a few signals.

chennes · 2022-03-05T18:29:44Z

src/Gui/ShortcutManager.cpp

+    // Qt's shortcut state machine favors shortest match (which is ridiculous,
+    // unless I'm mistaken?). We'll do longest match. We've disabled all


I don't know if you're mistaken, but I agree with you that longest match should be the correct behavior.

chennes · 2022-03-05T18:47:43Z

src/Gui/ShortcutManager.cpp

+        return;
+    // Keep the same top priority of the given action, and adjust the rest. Can
+    // go negative if necessary
+    int current = 0;


If negative priorities are allowed, does it make sense to initialize current to zero?

Suggested change

int current = 0;

int current = std::numeric_limits<int>::lowest();

I suppose it can work. It's just that the priority needs to rise from some base level. If the base is the lowest, then we'd have bunch of huge negative integers in the parameters, which would be confusing.

The reason the priority is allow to go negative is because I try to preserve the top priority (among actions with the same shortcuts) while reordering. This is a partial solution to a corner case where the user may have set priority in the past for some action in a workbench that hasn't been loaded yet. Keep the top priority unchanged can provide some degree of stability in this case.

chennes · 2022-03-05T18:53:50Z

src/Gui/ShortcutManager.cpp

+            }
+        }
+    }
+    timer.stop();


It seems like this would make more sense at the top of the function, rather than the bottom.

chennes · 2022-03-05T19:00:03Z

src/Gui/ShortcutManager.h

+
+        ActionData(QAction *action, const char *name = "")
+            : key(action->shortcut(), name)
+            , pointer(reinterpret_cast<int64_t>(action))


Type name mismatch, you are using intptr_t above.

chennes · 2022-03-05T19:01:40Z

src/Gui/Workbench.cpp

@@ -311,6 +311,8 @@ void Workbench::setupCustomToolbars(ToolBarItem* root, const Base::Reference<Par

 void Workbench::setupCustomShortcuts() const
 {
+    // Now managed by ShortcutManager
+#if 0


I don't see any reason to leave more than just the comment here.

chennes

Overall I really like the feature, and the code looks good. Just a few comments:

The user interface for setting the multi-key delay makes it look like we are storing a delay per-keystroke. I don't have a great suggestion for improving it, but I don't know if we can include it as-is.
I think there needs to be a timeout, after which an in-progress keystroke sequence is cancelled even if the user presses no more keys.
I'm not familiar enough with the Boost multi-index containers to really evaluate, I assume the reinterpret casts are related to the usage there? Obviously those set off red flags and warning sirens when reviewing code.

realthunder · 2022-03-06T13:14:11Z

I think there needs to be a timeout, after which an in-progress keystroke sequence is cancelled even if the user presses no more keys.

The timer in ShortcutManager is used for this purpose. In fact, the Multi-key delay specifies the time to wait for more keystorke, after which the pending action will be triggered.

I'm not familiar enough with the Boost multi-index containers to really evaluate, I assume the reinterpret casts are related to the usage there? Obviously those set off red flags and warning sirens when reviewing code.

I intend to use QAction* as map key for easy lookup, but the action may be deleted somewhere else. I use QPointer for safe accessing, and because QPointer is not immutable, it can't be used for key. I deliberately change the key type to intptr_t to prevent accidentally using it to access the action.

Because long description may cause undesired dialog layout changes. The description is available through tool tip of the command tree widget.

Add API Command::initAction() to force create action for all commands with shortcut in order to register with ShortcutManager to obtain a complete list of actions with the same shortcut.

realthunder · 2022-03-12T14:06:54Z

@wwmayer New commits added for suggested changes and bug fix.

freecadci · 2022-03-12T21:50:06Z

for feature branch PR_6506. Pipeline 490728511 was triggered at 9f40b81. All CI branches and pipelines.

Related FreeCAD#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.

freecadci · 2022-03-16T04:48:39Z

for feature branch PR_6506. Pipeline 492097097 was triggered at 1a8d6fe. All CI branches and pipelines.

donovaly · 2022-04-02T19:05:24Z

We will have to postpone this to 0.21 to make 0.20 ready.

@realthunder maybe you have a fix or could review pending PRs for the 0.20 milestone?: https://github.com/FreeCAD/FreeCAD/milestone/2

luzpaz · 2022-06-17T13:12:17Z

This would be an incredible upgrade to the UI/UX. Hopefully this gets priority love

chennes · 2022-09-14T03:32:35Z

@realthunder if you are bored with working on the TNP mega-merge and want a break 😆, my impression of this PR is that it's ready for a final review once its conflicts are resolved.

realthunder · 2022-09-14T10:56:52Z

Great. I'll work on it soon.

wwmayer · 2022-11-10T13:35:28Z

@chennes I guess we all still agree in merging this PR? If yes, I will start to rebase it and fix the merge conflicts because this PR offers some functions that are needed in another PR.

chennes · 2022-11-10T13:37:15Z

Yes, I think this is good functionality--go for it!

realthunder · 2022-11-10T13:49:03Z

Sorry, forgot about this. I can rebase this tomorrow. But if @wwmayer have time, go for it sure. BTW, there is a follow up fix at here.

wwmayer · 2022-11-10T14:04:35Z

Thanks, I will start with it now...

wwmayer · 2022-11-10T20:11:13Z

#7771 replaces this PR.

realthunder added 3 commits March 4, 2022 10:20

Gui: refactor 'Customize -> Toolbars' command list handling

ae91008

By reusing code in DlgCustomKeyboardImp

Gui: fix shortcut editor

4c10bb3

github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Mar 4, 2022

Fix comment typo

62fce1c

chennes reviewed Mar 5, 2022

View reviewed changes

src/Gui/Action.cpp Outdated

Comment on lines 1367 to 1368

public:

Copy link

Member

chennes Mar 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

public:

chennes reviewed Mar 5, 2022

View reviewed changes

src/Gui/Action.cpp Outdated Show resolved Hide resolved

chennes reviewed Mar 5, 2022

View reviewed changes

src/Gui/Command.cpp Show resolved Hide resolved

chennes reviewed Mar 5, 2022

View reviewed changes

realthunder added 3 commits March 6, 2022 20:43

Gui: fix action title processing

2ed5f65

Minor code change according to suggestions

12008ed

Fix build warning

b56a169

realthunder added 6 commits March 12, 2022 21:53

Gui: move CommandCompleter into its own source file

1d6020e

Gui: fix macro command sync in Customize -> Keyboard/Toolbar

f99b031

Gui: fix missing separator item in Customize -> Toolbar

02982d5

Gui: change Command::getRevision() to const

e89453c

Gui: remove command description label in Customize -> Keyboard

866873a

Because long description may cause undesired dialog layout changes. The description is available through tool tip of the command tree widget.

Gui: fix Customize -> Keyboard shortcut priroity list

9f40b81

Add API Command::initAction() to force create action for all commands with shortcut in order to register with ShortcutManager to obtain a complete list of actions with the same shortcut.

luzpaz mentioned this pull request Mar 13, 2022

Toolbar-only command must be visible for custom shortcut to work #6097

Closed

realthunder added 4 commits March 14, 2022 10:24

Gui: fix command completer selection on item activate

755cd93

Gu: fix readonly AccelLineEdit

1c3719c

Gui: do not remove ending puncuation when clean action title

1a8d6fe

realthunder force-pushed the ShortcutManager branch from fdb7e8b to 1a8d6fe Compare March 14, 2022 10:33

donovaly added the For 0.21 label Apr 2, 2022

chennes removed the For 0.21 label Jun 24, 2022

luzpaz added UI/UX Shortcut labels Jun 28, 2022

wwmayer closed this Nov 10, 2022

luzpaz mentioned this pull request Nov 10, 2022

Gui: introduce ShortcutManager for shortcut management and conflict resolving #7771

Merged

1 task

		connect(lineedit, SIGNAL(textEdited(QString)), this, SLOT(onTextChanged(QString)));
		connect(this, SIGNAL(activated(QModelIndex)), this, SLOT(onCommandActivated(QModelIndex)));

		// Qt's shortcut state machine favors shortest match (which is ridiculous,
		// unless I'm mistaken?). We'll do longest match. We've disabled all

	int current = 0;
	int current = std::numeric_limits<int>::lowest();

Gui: introduce ShortcutManager for shortcut management and conflict resolving #6506

Gui: introduce ShortcutManager for shortcut management and conflict resolving #6506

Conversation

realthunder commented Mar 4, 2022 • edited

luzpaz commented Mar 4, 2022

freecadci commented Mar 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chennes Mar 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chennes left a comment

Choose a reason for hiding this comment

realthunder commented Mar 6, 2022

realthunder commented Mar 12, 2022

freecadci commented Mar 12, 2022

freecadci commented Mar 16, 2022

donovaly commented Apr 2, 2022

luzpaz commented Jun 17, 2022

chennes commented Sep 14, 2022

realthunder commented Sep 14, 2022

wwmayer commented Nov 10, 2022

chennes commented Nov 10, 2022

realthunder commented Nov 10, 2022

wwmayer commented Nov 10, 2022

wwmayer commented Nov 10, 2022

realthunder commented Mar 4, 2022 •

edited

chennes Mar 5, 2022 •

edited