before emitting finishedLoading, go thru the list of all
actions and change them as needed to show the tooltip.
Signed-off-by: Tomaz Canabrava <tcanabrava@kde.org>
mwolff | |
apol | |
antonanikin | |
kfunk |
KDevelop |
before emitting finishedLoading, go thru the list of all
actions and change them as needed to show the tooltip.
Signed-off-by: Tomaz Canabrava <tcanabrava@kde.org>
manual
No Linters Available |
No Unit Test Coverage |
This works, thanks.
But I think you should add some comments to code about current restrictions - for example, tooltips are not updated after shortcut changing - we need to restart kdevelop for this.
And also maybe it better to use monospace font for shortcut displaying? Something like this:
foreach(QAction *action, actionCollection()->actions()) { QString shortCut; if (!action->shortcut().isEmpty()) shortCut = action->shortcut().toString(); else if (!actionCollection()->defaultShortcut(action).isEmpty()) shortCut = actionCollection()->defaultShortcut(action).toString(); if (!shortCut.isEmpty()) action->setToolTip(QString("<html>%1 (<b><tt>%2</tt></b>)</html>").arg(action->toolTip()).arg(shortCut)); }
Tomaz, please also add link to the related bug: https://bugs.kde.org/show_bug.cgi?id=364963.
shell/mainwindow.cpp | ||
---|---|---|
353 ↗ | (On Diff #10388) | this is pretty brittle code. I'd rather we use a dynamic property to store the original tooltip string, i.e. before overwriting it, call action->setProperty("__kdev_original_tooltip", tooltip); Then use that later on to append the shortcut string to. |
358 ↗ | (On Diff #10388) | auto shortcut = action->shortcut(); if (shortcut.isEmpty()) { shortcut = actionCollection()->defaultShortcut(action); } if (!shortcut.isEmpty()) { action->setTooltip(...); } |
still not good to go
shell/mainwindow.cpp | ||
---|---|---|
319 ↗ | (On Diff #10843) | this should guard the loops below now, otherwise you do useless work here or could you simply move the updateActionTooltips up and then keep the old code as-is? |
390 | const auto ORIGINAL_TOOLTIP = QByteArrayLiteral("__kdev_original_tooltip"); | |
392 | simplify and fix some bugs: QString tooltip = action->property(ORIGINAL_TOOLTIP).toString(); if (tooltip.isEmpty()) { tooltip = action->toolTip(); } auto shortcut = action->sohrtcut(); if (shortcut.isEmpty()) { // NOTE: your patch uses !isEmpty - that sounds wrong? shortcut = actionCollection()->defaultShortcut(action); } // make sure to always set the tooltip, e.g. when the shortcut gets unset if (!shortcut.isEmpty()) { tooltip = i18nc("%1: original tooltip, %2: shortcut", "%1 <i>(%2)</i>", shortcut.toString()); // NOTE: use i18n to make this localizable } action->setToolTip(tooltip); | |
397 | this is the wrong way around, no? | |
401 | you have to reset the tooltip if the shortcut was unset | |
403 | this is not localization aware, use i18nc (imagine RTL languages) |
if possible, remove the foreach. otherwise lgtm
shell/mainwindow.cpp | ||
---|---|---|
352 ↗ | (On Diff #10388) | don't use foreach in new code please (use range-based for) |
shell/mainwindow.cpp | ||
---|---|---|
350 ↗ | (On Diff #10388) | Maybe also add static ? |
shell/mainwindow.cpp | ||
---|---|---|
350 ↗ | (On Diff #10388) | not needed for Q{ByteArray,String}Literal |
Please check out the alternative approach in https://phabricator.kde.org/D4697 -- this version works for both actions added from the main window + actions from plugins.