Show shortcuts on the tooltips.
AbandonedPublic

Authored by tcanabrava on Jan 13 2017, 4:05 PM.

Details

Reviewers
mwolff
apol
antonanikin
kfunk
Group Reviewers
KDevelop
Summary

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>

Test Plan

manual

Diff Detail

Repository
R33 KDevPlatform
Branch
show_shortcuts
Lint
No Linters Available
Unit
No Unit Test Coverage
tcanabrava updated this revision to Diff 10148.Jan 13 2017, 4:05 PM
tcanabrava retitled this revision from to Show shortcuts on the tooltips..
tcanabrava updated this object.
tcanabrava edited the test plan for this revision. (Show Details)
tcanabrava added reviewers: kfunk, apol.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 13 2017, 4:05 PM
antonanikin requested changes to this revision.Jan 16 2017, 4:22 AM
antonanikin added a reviewer: antonanikin.
antonanikin added a subscriber: antonanikin.

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));
}
This revision now requires changes to proceed.Jan 16 2017, 4:22 AM

Tomaz, please also add link to the related bug: https://bugs.kde.org/show_bug.cgi?id=364963.

tcanabrava updated this revision to Diff 10232.Jan 16 2017, 1:04 PM
tcanabrava edited edge metadata.
  • React on when a shortcut changes
kfunk edited edge metadata.EditedJan 16 2017, 1:23 PM

Note (not relevant to this particular patch): This is something we could/should probably make more generic and use in other places as well.

shell/mainwindow.cpp
354 ↗(On Diff #10232)

curr -> toolTip

362 ↗(On Diff #10232)

+1 on Anton's idea, maybe a different font format for the shortcut string would make sense.

/me proposes italics

kfunk requested changes to this revision.Jan 16 2017, 1:23 PM
kfunk edited edge metadata.
This revision now requires changes to proceed.Jan 16 2017, 1:23 PM
tcanabrava updated this revision to Diff 10388.Jan 20 2017, 3:14 PM
tcanabrava edited edge metadata.
tcanabrava marked 2 inline comments as done.
  • React on when a shortcut changes
  • Rename curr->toolTip, and show text in italics.
mwolff requested changes to this revision.Jan 22 2017, 8:13 PM
mwolff added a reviewer: mwolff.
mwolff added a subscriber: mwolff.
mwolff added inline comments.
shell/mainwindow.cpp
353

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
auto shortcut = action->shortcut();
if (shortcut.isEmpty()) {
    shortcut = actionCollection()->defaultShortcut(action);
}
if (!shortcut.isEmpty()) {
    action->setTooltip(...);
}
This revision now requires changes to proceed.Jan 22 2017, 8:13 PM
tcanabrava updated this revision to Diff 10843.Feb 2 2017, 11:04 AM
tcanabrava edited edge metadata.
  • React on when a shortcut changes
  • Rename curr->toolTip, and show text in italics.
  • Save the original tooltip in a property / retrieve later.
mwolff requested changes to this revision.Feb 2 2017, 11:20 AM
mwolff edited edge metadata.

still not good to go

shell/mainwindow.cpp
319

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?

350 ↗(On Diff #10232)
const auto ORIGINAL_TOOLTIP = QByteArrayLiteral("__kdev_original_tooltip");
352 ↗(On Diff #10232)

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);
357 ↗(On Diff #10232)

this is the wrong way around, no?

361 ↗(On Diff #10232)

you have to reset the tooltip if the shortcut was unset

363 ↗(On Diff #10232)

this is not localization aware, use i18nc (imagine RTL languages)

This revision now requires changes to proceed.Feb 2 2017, 11:20 AM
tcanabrava updated this revision to Diff 11287.Feb 13 2017, 1:10 PM
tcanabrava edited edge metadata.
  • React on when a shortcut changes
  • Rename curr->toolTip, and show text in italics.
  • Save the original tooltip in a property / retrieve later.
  • Fix displaying empty shortcuts and code cleanup
tcanabrava marked an inline comment as done.Feb 13 2017, 1:11 PM

Seems to work perfectly now.

mwolff accepted this revision.Feb 15 2017, 10:11 PM
mwolff edited edge metadata.

if possible, remove the foreach. otherwise lgtm

shell/mainwindow.cpp
352 ↗(On Diff #10232)

don't use foreach in new code please (use range-based for)

antonanikin added inline comments.Feb 17 2017, 2:45 AM
shell/mainwindow.cpp
350 ↗(On Diff #10232)

Maybe also add static ?

mwolff added inline comments.Feb 17 2017, 7:56 AM
shell/mainwindow.cpp
350 ↗(On Diff #10232)

not needed for Q{ByteArray,String}Literal

tomaz, please commit

kfunk accepted this revision.Feb 20 2017, 8:17 AM
kfunk requested changes to this revision.Feb 20 2017, 11:31 PM

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.

This revision now requires changes to proceed.Feb 20 2017, 11:31 PM
tcanabrava abandoned this revision.Feb 21 2017, 7:46 AM

Closing becauss KFunk found a better solution.