[Differential] [Requested Changes To] D4123: Show shortcuts on the tooltips.

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  still not good to go

INLINE COMMENTS

> mainwindow.cpp:319
>      KTextEditor::View *activeClient = Core::self()->documentController()->activeTextDocumentView();
> -    if(!activeClient)
> -        return;

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?

> mainwindow.cpp:390
> +{
> +    QByteArray original_tooltip = "__kdev_original_tooltip";
> +    foreach(QAction *action, actionCollection()->actions()) {

const auto ORIGINAL_TOOLTIP = QByteArrayLiteral("__kdev_original_tooltip");

> mainwindow.cpp:392
> +    foreach(QAction *action, actionCollection()->actions()) {
> +        if (action->dynamicPropertyNames().indexOf(original_tooltip) == -1) {
> +            action->setProperty(original_tooltip, action->toolTip());

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);

> mainwindow.cpp:397
> +        auto shortcut = action->shortcut();
> +        if (!shortcut.isEmpty()) {
> +            shortcut = actionCollection()->defaultShortcut(action);

this is the wrong way around, no?

> mainwindow.cpp:401
> +        if (shortcut.isEmpty()) {
> +            continue;
> +        }

you have to reset the tooltip if the shortcut was unset

> mainwindow.cpp:403
> +        }
> +        action->setToolTip(action->property(original_tooltip).toString() + " <i>(" + shortcut.toString() + ")</i>");
> +    }

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

REPOSITORY
  R33 KDevPlatform

REVISION DETAIL
  https://phabricator.kde.org/D4123

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: tcanabrava, apol, antonanikin, kfunk, #kdevelop, mwolff
Cc: mwolff, antonanikin, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170202/3e8fa1fb/attachment-0001.html>