Keyboard: automatically reload mapping file and update tooltips #13082

ronso0 · 2024-04-10T13:34:22Z

Yeeiij, finally!
Tooltips and the mapping are rebuilt completely if the mapping file has been changed (or removed).
As before, we look for the user's Custom.kbd.cfg first, then use the built-in mapping based on the selected locale.

~~This 'only' covers the GUI mapping.~~ Extending this to WMainMenuBar, as done in #1746, is ~~certainly possibly.~~ done ✔️

Inspired by #1746, but implemented more simply.
Replaces #13074

Fixes #9814

acolombier · 2024-04-10T13:55:19Z

src/controllers/keyboard/keyboardeventfilter.cpp

+    connect(&m_fileWatcher,
+            &QFileSystemWatcher::fileChanged,
+            this,
+            &KeyboardEventFilter::reloadKeyboardConfig);


Just a note that out of the box, the watcher may have a double reload due to the policy used by certain IDE edit files while ensuring resiliency to I/O error (Qt doc about that). It might be worth doing something similar to QML. Perhaps this might be worth introducing a util class to file watcher efficiently?

Thank you for this hint!
Do you mind filing a bug report so we can track that?

For now I'd stick with the implementation we also use for controller scripts.

Will do!
Note that I fixed it in controllers in my PR for screen controller

I switched to AutoFileReloader now that #13607 has been merged.

ronso0 · 2024-04-10T21:01:52Z

oh, need to consider skin reload...

edit: Done.

ronso0 · 2024-04-15T18:55:03Z

Extending this to WMainMenuBar, as done in #1746, is certainly possibly.

Done.

ronso0 · 2024-04-16T09:06:15Z

We could add dummy shortcuts (empty) to all currently unmapped menubar actions so users can add shortcuts themselves.
(i.e. add ConfigKeys to menubar and all *.kbd.cfg files)

Nothing that holds up this PR though.

cr7pt0gr4ph7

Not too important, but the upper/lowercasing of KeyboardEventFIlter got a bit mixed up the commit message Keyboard: handle mapping file and config setting in KeyboardEventFIlter`.

cr7pt0gr4ph7 · 2024-04-26T11:40:52Z

src/widget/wmainmenubar.cpp

@@ -162,9 +169,10 @@ void WMainMenuBar::initialize() {
    QString createPlaylistText = tr("Create a new playlist");
    auto* pLibraryCreatePlaylist = new QAction(createPlaylistTitle, this);
    pLibraryCreatePlaylist->setShortcut(


Wouldn't it be cleaner to do the following?

m_pKbd->registerMenuBarActionForShortcut( pLibraryRescan, ConfigKey("[KeyboardShortcuts]", "LibraryMenu_Rescan"), tr("Ctrl+Shift+L"));

instead of:

pLibraryRescan->setShortcut( QKeySequence(m_pKbd->registerMenuBarActionGetKeySeqString( pLibraryRescan, ConfigKey("[KeyboardShortcuts]", "LibraryMenu_Rescan"), tr("Ctrl+Shift+L"))));

This avoids having the setShortcut both here (for the initial setup) and in KeyboardEventFilter (for the reload), which feels a bit like code duplication. It also avoids repetition of pLibraryRescan.

Thanks, done.

daschuer · 2024-05-01T15:18:30Z

This one has a conflict now. Can we remove the 2.5-beta milestone?

ronso0 · 2024-05-01T15:24:48Z

Sure, assigned it to 2.5 for now because I think this is a bugfix, see #9814
It can either slip in during the beta period (it's already working without issues), or later.

Set to draft because I've just rebased this and am about to tweak it considering @cr7pt0gr4ph7's suggestions.

cr7pt0gr4ph7

Small nitpick: Spelling of "chnanged" in the commit message. LGTM otherwise.

ronso0 · 2024-05-03T10:25:49Z

Thanks for taking the time to review!

Spelling of "chnanged" in the commit message.

Yeah, that happens when I work in bright sunlight...

…gled or mapping file changed

…are toggled or mapping file changed

ronso0 · 2024-10-13T14:19:22Z

Oh sorry, I did a rebase instead of merging main.

Swiftb0y

couple codestyle complaints

Swiftb0y · 2024-10-13T14:48:52Z

src/skin/legacy/legacyskinparser.cpp

+                const ConfigKey configKey = ConfigKey::parseCommaSeparated(key);
+                shortcutKeys.append(std::make_pair(configKey, QString()));


take advantage of rvalue move semantics.

Suggested change

const ConfigKey configKey = ConfigKey::parseCommaSeparated(key);

shortcutKeys.append(std::make_pair(configKey, QString()));

shortcutKeys.append({ConfigKey::parseCommaSeparated(key), QString()});

Swiftb0y · 2024-10-13T14:50:10Z

src/skin/legacy/legacyskinparser.h

@@ -140,7 +140,8 @@ class LegacySkinParser : public QObject, public SkinParser {
    void setupWidget(const QDomNode& node, QWidget* pWidget,
                     bool setupPosition=true);
    void setupConnections(const QDomNode& node, WBaseWidget* pWidget);
-    void addShortcutToToolTip(WBaseWidget* pWidget, const QString& shortcut, const QString& cmd);
+    const QString buildShortcutString(const QString& shortcut, const QString& cmd) const;


Suggested change

const QString buildShortcutString(const QString& shortcut, const QString& cmd) const;

QString buildShortcutString(const QString& shortcut, const QString& cmd) const;

Swiftb0y · 2024-10-13T14:51:46Z

src/coreservices.h

-    std::shared_ptr<ConfigObject<ConfigValueKbd>> getKeyboardConfig() const {
-        return m_pKbdConfig;
-    }
+    std::shared_ptr<ConfigObject<ConfigValueKbd>> getKeyboardConfig() const;


why not in the header? Its body is small enough.

In KeyboardEventFilter yes.
Here it was small before, when CoreServices created the ConfigValueKbd.
Now KeyboardEventFilter creates it and we may not have created that yet, so I think I'll add another test, so it'll be slightly larger. Like this

std::shared_ptr<ConfigObject<ConfigValueKbd>> getKeyboardConfig() const { VERIFY_OR_DEBUG_ASSERT(m_pKeyboardEventFilter) { return std::make_shared<ConfigObject<ConfigValueKbd>>(); } return m_pKeyboardEventFilter->getKeyboardConfig(); }

Wdyt?

makes sense. Though you may want to consider returning a nullptr instead.

Hmm, actually we don't need that anymore.
The only place where keys are requested is

mixxx/src/library/autodj/autodjfeature.cpp

Line 180 in 34dde6f

keyboard->getKeyboardConfig()->getValueString(ConfigKey("[AutoDJ]", "enabled")),

and that gets a KeyboardEventFilter pointer from Library which has a CoreServices pointer.
And KeyboardEventFilter is initialized in the CoreServices ctor so this seems safe.

Will remove.

Swiftb0y · 2024-10-13T14:56:37Z

src/widget/wbasewidget.h

+        m_shortcutKeys = cfgKeys;
+    }
+
+    const QList<std::pair<ConfigKey, QString>> getShortcutKeys() {


Suggested change

const QList<std::pair<ConfigKey, QString>> getShortcutKeys() {

const QList<std::pair<ConfigKey, QString>>& getShortcutKeys() {

Swiftb0y · 2024-10-13T14:57:47Z

src/widget/wbasewidget.h

    QString m_baseTooltip;
+    QString m_shortcutTooltip;
+    QString m_baseTooltipOptShortcuts;
+    QList<std::pair<ConfigKey, QString>> m_shortcutKeys;


how big is this QList on average?

For knobs and sliders there'll be 5 4 controls (~~control,~~ _up, _down, _up_small, _down_small).
For other widgets it's 1-2 (left- and right-click).

thanks. makes sense. This is not duplicated between widgets then?

No. It's set by LegacySkinParser and fetched by KeyboardEventFilter which looks up the keys per control, composes the tooltip and pushes that to the widget.

Btw this QList with a pair feels a bit clunky, so I switched to a QMap<ConfigKey, QString> (sorted by key) with convenient cons_key_value_iterator and structured binding.
This also has the benefit that shortcuts are now always sorted by control/sub-control's ConfigKey¹ (same group, so sorted by item) instead of order they've been added (QList) or sorted by the command's tr string (std::map<QString, ConfigKey>.
The slight performance regression shouldn't matter much as we add 4 items max.

¹WPushButton: _activate, _toggle
WKnob/WSlider: _down, _down_small, _up, _up_small

I'm primarily concerned about the memory overhead from many large QLists in many widgets. But I can't make a good judgement call here either.

ScopedTimer says: for 12 calls KeyboardEventFilter::updateWidgetShortcuts() takes around 0.08s on average.
The diff with/without the extra ControlPotmeter/WHotcueButton controls is hardly measurable.

Right, but I was considering memory overhead (likely also not easily measurable because we're wasting so much already in other places), not runtime performance. Moreover 80ms is quite a lot...

Moreover 80ms is quite a lot...

ah okay, without the qdebug lines it's down to ~4ms for ~1600 widgets (LateNight).

I'll push the latest version soonish.

...and now (without the debug stuff) it's obvious that the extra controls do have a noticeable impact:
with: 4-5ms
without: ~2.5ms

Swiftb0y · 2024-10-13T14:59:44Z

src/widget/wmainmenubar.cpp

+                ConfigKey("[KeyboardShortcuts]",
+                        QString("OptionsMenu_EnableVinyl%1").arg(i + 1)),


would be nice if you could do this to the remaining lines if you're touching them anyways.

Suggested change

ConfigKey("[KeyboardShortcuts]",

QString("OptionsMenu_EnableVinyl%1").arg(i + 1)),

ConfigKey(QStringLiteral("[KeyboardShortcuts]"),

QStringLiteral("OptionsMenu_EnableVinyl%1").arg(i + 1)),

Swiftb0y · 2024-10-13T15:01:00Z

src/widget/wmainmenubar.cpp

    pOptionsKeyboard->setCheckable(true);
    pOptionsKeyboard->setChecked(keyboardShortcutsEnabled);
    pOptionsKeyboard->setStatusTip(keyboardShortcutText);
    pOptionsKeyboard->setWhatsThis(buildWhatsThis(keyboardShortcutTitle, keyboardShortcutText));
-    connect(pOptionsKeyboard, &QAction::triggered, this, &WMainMenuBar::toggleKeyboardShortcuts);
+    connect(pOptionsKeyboard, &QAction::triggered, m_pKeyboard, &KeyboardEventFilter::setEnabled);


Suggested change

connect(pOptionsKeyboard, &QAction::triggered, m_pKeyboard, &KeyboardEventFilter::setEnabled);

connect(pOptionsKeyboard, &QAction::triggered, m_pKeyboard.get(), &KeyboardEventFilter::setEnabled);

Swiftb0y · 2024-10-13T15:02:05Z

src/widget/wmainmenubar.cpp

        : QMenuBar(pParent),
          m_pConfig(pConfig),
-          m_pKbdConfig(pKbdConfig) {
+          m_pKeyboard(pKbd) {


to avoid unnecessary ref count changes.

Suggested change

m_pKeyboard(pKbd) {

m_pKeyboard(std::move(pKbd)) {

src/controllers/keyboard/keyboardeventfilter.cpp

Swiftb0y · 2024-10-13T15:06:30Z

src/controllers/keyboard/keyboardeventfilter.cpp

this is mostly copy pasted, right? Do you want to improve it or leave it?

…nubar

ronso0 · 2024-10-23T23:53:44Z

some thoughts on the general tooltip update approach used here and performance:

With the current implementatoin kbd mapppings are (re)loaded

on startup
when kbd shortcuts are toggled on, and
when the mapping file has been edited

so I think a tiny performance hit (GUI thread) in these non-critical situations is an acceptable tradeoff for having the GUI reflect the complete kbd mapping (all possible poti sub-controls) immediatly, even if it's just for double-checking that the mapping has been applied as expected.

The current method is:

for each control widget (button, knob, ..) the skin parser collects ConfigKeys for the from-widget connections, optionally creates the sub-control strings to be displayed in the tooltip
('up', 'down', 'up small', 'down small' for knobs and sliders, 'activate', 'clear' etc. for Hotcue buttons)
this CfgKey/string list is stored in each WBaseWidget
each widget is registered in KeyboardEventFilter (~1600 for LateNight 16 Samplers)
KeyboardEventFilter loads the kbd mapping file
iterate over all registered widgets, check if the controls are in the kbd mapping
- yes: (re)create kbd tooltip strings
- no: clear kbd tooltip string
- push string to widget, (re)construct tooltip
similar method for menubar actions

The downside:
all widgets are updated even if just one shortcut has been changed.

So I tried another approach, hoping that it would it be faster (in total, ie. load mapping file, check shortcuts, update widgets):

reload kbd mapping
compare last/new mapping, collect changed/added/removed shortcuts
collect affected widgets
update affected widgets

I had to implement the compare method and store more data in KeyboardEventFilter, and it turned out that this did not shorten the total time to the extent that it justifies the additional complexity (and potential errors/inconsistency), given that shortcuts are evaluated rarely.
(current method: ~6-7ms in total, alternative: ~5-6ms, measured when changing one shortcut)
I can provide the commit if someone is interested.

…are toggled or mapping file changed

ronso0 · 2024-10-26T11:24:30Z

some thoughts on the general tooltip update approach used here and performance:

With the current implementatoin kbd mapppings are (re)loaded

on startup
when kbd shortcuts are toggled on, and
when the mapping file has been edited

so I think a tiny performance hit (GUI thread) in these non-critical situations is an acceptable tradeoff for having the GUI reflect the complete kbd mapping (all possible poti sub-controls) immediatly, even if it's just for double-checking that the mapping has been applied as expected.

The current method is:

for each control widget (button, knob, ..) the skin parser collects ConfigKeys for the from-widget connections, optionally creates the sub-control strings to be displayed in the tooltip
('up', 'down', 'up small', 'down small' for knobs and sliders, 'activate', 'clear' etc. for Hotcue buttons)
this CfgKey/string list is stored in each WBaseWidget
each widget is registered in KeyboardEventFilter (~1600 for LateNight 16 Samplers)
KeyboardEventFilter loads the kbd mapping file
iterate over all registered widgets, check if the controls are in the kbd mapping
- yes: (re)create kbd tooltip strings
- no: clear kbd tooltip string
- push string to widget, (re)construct tooltip
similar method for menubar actions

The downside:
all widgets are updated even if just one shortcut has been changed.

So I tried another approach, hoping that it would it be faster (in total, ie. load mapping file, check shortcuts, update widgets):

reload kbd mapping
compare last/new mapping, collect changed/added/removed shortcuts
collect affected widgets
update affected widgets

I had to implement the compare method and store more data in KeyboardEventFilter, and it turned out that this did not shorten the total time to the extent that it justifies the additional complexity (and potential errors/inconsistency), given that shortcuts are evaluated rarely.
(current method: ~6-7ms in total, alternative: ~5-6ms, measured when changing one shortcut)
I can provide the commit if someone is interested.

ronso0 · 2024-10-26T11:25:32Z

IMO this is now ready for a final review.
@Swiftb0y suggestions for improving the copied code (eventFilter()) are welcome!

…are toggled or mapping file changed

…meters

github-actions bot added the ui label Apr 10, 2024

ronso0 added keyboard tooltip labels Apr 10, 2024

ronso0 added this to the 2.5-beta milestone Apr 10, 2024

acolombier reviewed Apr 10, 2024

View reviewed changes

acolombier mentioned this pull request Apr 10, 2024

Double reload when editing controller mapping file #13083

Open

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch 2 times, most recently from c41be0a to 6896dc8 Compare April 11, 2024 23:03

ronso0 marked this pull request as ready for review April 14, 2024 22:29

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 6896dc8 to 428caf9 Compare April 15, 2024 18:54

ronso0 mentioned this pull request Apr 15, 2024

Tooltips: immediatly show/hide keyboard hints when option is toggled #13074

Closed

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 428caf9 to 7f055f3 Compare April 15, 2024 19:22

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 7f055f3 to 4583b2d Compare April 19, 2024 08:01

ronso0 mentioned this pull request Apr 26, 2024

Usability/Keyboard: Add a rebindable keyboard shortcut for editing items as a replacement for F2 #13148

Open

cr7pt0gr4ph7 reviewed Apr 30, 2024

View reviewed changes

ronso0 marked this pull request as draft May 1, 2024 15:20

ronso0 modified the milestones: 2.5-beta, 2.5.0 May 1, 2024

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 4583b2d to 61db431 Compare May 1, 2024 16:47

cr7pt0gr4ph7 reviewed May 2, 2024

View reviewed changes

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 61db431 to 0b63ef6 Compare May 3, 2024 10:25

ronso0 marked this pull request as ready for review May 3, 2024 10:26

daschuer modified the milestones: 2.5.0, 2.5-beta May 8, 2024

ronso0 removed the controller backend label Sep 6, 2024

ronso0 added 4 commits October 12, 2024 01:24

move handling of keyboard mapping file to KeyboardEventFilter

4f9207d

Skins: update tooltips with keyboard shortcuts when shortcuts are tog…

cffa779

…gled or mapping file changed

Menubar: update shortcuts when the keyboard mapping file changed

bbe1e2b

KeyboardEventFilter: restore default ON, auto*, disconnect shorthand

ea676fc

ronso0 mentioned this pull request Oct 13, 2024

fix: add explicit virtual destructor in order to fix linker issues #13756

Merged

ronso0 added 4 commits October 13, 2024 16:14

fixup! Skins: update tooltips with keyboard shortcuts when shortcuts …

d715454

…are toggled or mapping file changed

fixup! Skins: update tooltips with keyboard shortcuts when shortcuts …

36c8a49

…are toggled or mapping file changed

fixup! move handling of keyboard mapping file to KeyboardEventFilter

44dc1c8

KeyboardEventFilter: use mixxx::Logger

44f47e9

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 0d83677 to 44f47e9 Compare October 13, 2024 14:18

github-actions bot added the controller backend label Oct 13, 2024

KeyboardEventFilter: move menubar toggle connection to WMainMenubar

19b4434

Swiftb0y reviewed Oct 13, 2024

View reviewed changes

ronso0 added 2 commits October 13, 2024 19:55

fixup! KeyboardEventFilter: move menubar toggle connection to WMainMe…

e3212d9

…nubar

fixup! move handling of keyboard mapping file to KeyboardEventFilter

49afca2

ronso0 added 3 commits October 24, 2024 02:13

fixup! Skins: update tooltips with keyboard shortcuts when shortcuts …

4103f0f

…are toggled or mapping file changed

fixup! Menubar: update shortcuts when the keyboard mapping file changed

4ed1a9c

fixup! KeyboardEventFilter: use mixxx::Logger

465ba9a

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 0a3d391 to 465ba9a Compare October 24, 2024 00:13

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 94f3037 to e9f3ca0 Compare October 26, 2024 12:03

ronso0 added 3 commits October 26, 2024 14:07

fixup! Skins: update tooltips with keyboard shortcuts when shortcuts …

693015b

…are toggled or mapping file changed

Tooltips: create keyboard shortcuts for hotcue buttons and ControlPot…

4eed28c

…meters

KeyboardEventFilter: add file path helper, polish

c85011c

ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from e9f3ca0 to c85011c Compare October 26, 2024 12:07

ronso0 mentioned this pull request Oct 26, 2024

Keyboard controller #1746

Closed

		const ConfigKey configKey = ConfigKey::parseCommaSeparated(key);
		shortcutKeys.append(std::make_pair(configKey, QString()));

	const ConfigKey configKey = ConfigKey::parseCommaSeparated(key);
	shortcutKeys.append(std::make_pair(configKey, QString()));
	shortcutKeys.append({ConfigKey::parseCommaSeparated(key), QString()});

	const QString buildShortcutString(const QString& shortcut, const QString& cmd) const;
	QString buildShortcutString(const QString& shortcut, const QString& cmd) const;

	const QList<std::pair<ConfigKey, QString>> getShortcutKeys() {
	const QList<std::pair<ConfigKey, QString>>& getShortcutKeys() {

		ConfigKey("[KeyboardShortcuts]",
		QString("OptionsMenu_EnableVinyl%1").arg(i + 1)),

	connect(pOptionsKeyboard, &QAction::triggered, m_pKeyboard, &KeyboardEventFilter::setEnabled);
	connect(pOptionsKeyboard, &QAction::triggered, m_pKeyboard.get(), &KeyboardEventFilter::setEnabled);

Keyboard: automatically reload mapping file and update tooltips #13082

Are you sure you want to change the base?

Keyboard: automatically reload mapping file and update tooltips #13082

Conversation

ronso0 commented Apr 10, 2024 • edited Loading

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

ronso0 commented Apr 10, 2024 • edited Loading

ronso0 commented Apr 15, 2024

ronso0 commented Apr 16, 2024

cr7pt0gr4ph7 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 1, 2024

ronso0 commented May 1, 2024

cr7pt0gr4ph7 left a comment • edited Loading

Choose a reason for hiding this comment

ronso0 commented May 3, 2024

ronso0 commented Oct 13, 2024

Swiftb0y left a 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

ronso0 Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Oct 14, 2024 • edited Loading

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

ronso0 commented Oct 23, 2024

The current method is:

ronso0 commented Oct 26, 2024

The current method is:

ronso0 commented Oct 26, 2024

ronso0 commented Apr 10, 2024 •

edited

Loading

ronso0 commented Apr 10, 2024 •

edited

Loading

cr7pt0gr4ph7 left a comment •

edited

Loading

cr7pt0gr4ph7 left a comment •

edited

Loading

ronso0 Oct 13, 2024 •

edited

Loading

ronso0 Oct 14, 2024 •

edited

Loading