D6183: [Digital Clock] Allow copying current date and time to clipboard

[Kai Uwe Broulik]

broulik added a comment.


  Thanks a lot for picking this up!
  
  I have some minor nitpicks below, otherwise looking very good. You don't need to mention "Fixes https://bugs.kde.org/show_bug.cgi?id=355190" in the commit message, the "BUG: 355190" already does this, also causes Bugzilla to auto-close the bug once committed, etc.
  
  The reason I did not finish/submit my original patch is that the time dataengine is updated only once every minute (unless you have seconds enabled) so when you open the menu the seconds will not be accurate but reflect the last time it was updated (usually being 0).
  
  I will try to figure out a way to force an update explicitly from QML, so we can finally have this in.

INLINE COMMENTS

> clipboardmenu.cpp:83
> +        s = time.toString(Qt::SystemLocaleLongDate);
> +        s.replace(rx, "");
> +        a = menu->addAction(s);

Prefer `QString()` over an empty literal `""`

> clipboardmenu.cpp:116
> +
> +        QMenu *menu_cal = new QMenu;
> +        menu_cal->clear();

You don't need to create this menu here, `QMenu::addMenu(QString)` that you're using below already creates a new menu for you (basically just remove this and the next line).

Also, we typically use camel-case instead of underscores, eg. `otherCalendarsMenu`

> clipboardmenu.cpp:120
> +
> +/* Add ICU Calendars if QLocale is ready for
> +        Chinese

That optimism ;)

> clipboardmenu.cpp:132
> +        s = QString::number(m_currentDate.toMSecsSinceEpoch() / 1000);
> +        a = menu_cal->addAction(s + ws + "(" + i18n("UNIX Time") + ")");
> +        a->setData(s);

I would prefer having proper `i18n` with placeholders here (I think the word puzzles above are fine given the text itself cannot really be influenced by us, but here and below it should be:

  i18nc("placeholder is unix timestamp", "%1 (UNIX Time)", s);

(the `c` means "context", so the first string gives translators a bit of an explanation what this is about)

> clipboardmenu.cpp:140
> +
> +    connect(menu, &QMenu::triggered, menu, l);
> +

You could just do the lambda inline here, I don't see `l` being used elsewhere

> clipboardmenu.cpp:143
> +    // QMenu cannot have QAction as parent and setMenu doesn't take ownership
> +    connect(action, &QObject::destroyed, this, [menu] {
> +        menu->deleteLater();

I know this is from the original code but I think you could optimize it to

  connect(action, &QObject::destroyed, menu, &QObject::deleteLater);

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170612/dce4f0ef/attachment-0001.html>