Differential D6183 Diff 31690 applets/digital-clock/plugin/clipboardmenu.cpp

Changeset View

Standalone View

View Options

applets/digital-clock/plugin/clipboardmenu.cpp

  • This file was added.
1​/*
2​ * Copyright 2016 Kai Uwe Broulik <kde@privat.broulik.de>
3​ *
4​ * This program is free software; you can redistribute it and/or
5​ * modify it under the terms of the GNU General Public License as
6​ * published by the Free Software Foundation; either version 2 of
7​ * the License or (at your option) version 3 or any later version
8​ * accepted by the membership of KDE e.V. (or its successor approved
9​ * by the membership of KDE e.V.), which shall act as a proxy
10​ * defined in Section 14 of version 3 of the license.
11​ *
12​ * This program is distributed in the hope that it will be useful,
13​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15​ * GNU General Public License for more details.
16​ *
17​ * You should have received a copy of the GNU General Public License
18​ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19​ *
20​ */
21​
22​#include "clipboardmenu.h"
23​#include "klocalizedstring.h"
24​
25​#include <QClipboard>
26​#include <QGuiApplication>
27​#include <QMenu>
28​
29​ClipboardMenu::ClipboardMenu(QObject *parent) : QObject(parent)
30​{
31​
32​}
33​
34​ClipboardMenu::~ClipboardMenu() = default;
35​
36​QDateTime ClipboardMenu::currentDate() const
37​{
38​ return m_currentDate;
39​}
40​
41​void ClipboardMenu::setCurrentDate(const QDateTime &currentDate)
42​{
43​ if (m_currentDate != currentDate) {
44​ m_currentDate = currentDate;
45​ emit currentDateChanged();
46​ }
47​}
48​
49​bool ClipboardMenu::secondsIncluded() const
50​{
51​ return m_secondsIncluded;
52​}
53​
54​void ClipboardMenu::setSecondsIncluded(const bool &secondsIncluded)
55​{
56​ if (m_secondsIncluded != secondsIncluded) {
57​ m_secondsIncluded = secondsIncluded;
58​ emit secondsIncludedChanged();
59​ }
60​}
61​
62​void ClipboardMenu::setupMenu(QAction *action)
63​{
64​ QMenu *menu = new QMenu;
65​
66​/*
67​ * The refresh rate of m_currentDate depends of what is shown in the plasmoid.
68​ * If only minutes are shown, the value is updated at the full minute and
69​ * seconds are always 0 or 59 and thus useless/confusing to offer for copy.
70​ * Use a reference to the config's showSeconds to decide if seconds are sent
71​ * to the clipboard. There was no workaround found ...
72​ */
73​ connect(menu, &QMenu::aboutToShow, this, [this, menu] {
74​ menu->clear();
75​
76​ const QDate date = m_currentDate.date();
77​ const QTime time = m_currentDate.time();
78​ const QRegExp rx ("[^0-9:]");
79​ const QChar ws = QLatin1Char(' ');
80​ QString s;
81​ QAction *a;
82​
83​ s = date.toString(Qt::SystemLocaleShortDate);
84​ a = menu->addAction(s);
davidedmundsonUnsubmitted
Not Done

you're removing everything that isn't ':' or a number ? Why?

Does every locale use : as a separator?

davidedmundson: you're removing everything that isn't ':' or a number ? Why? Does every locale use : as a…
broulikUnsubmitted
Not Done

Prefer QString() over an empty literal ""

broulik: Prefer `QString()` over an empty literal `""`
85​ a->setData(s);
86​ s = date.toString(Qt::ISODate);
87​ a = menu->addAction(s);
88​ a->setData(s);
89​ s = date.toString(Qt::SystemLocaleLongDate);
90​ a = menu->addAction(s);
91​ a->setData(s);
92​
93​ menu->addSeparator();
94​
95​ s = time.toString(Qt::SystemLocaleShortDate);
96​ a = menu->addAction(s);
97​ a->setData(s);
98​ if (m_secondsIncluded) {
99​ s = time.toString(Qt::SystemLocaleLongDate);
100​ s.replace(rx, QString());
101​ a = menu->addAction(s);
102​ a->setData(s);
103​ s = time.toString(Qt::SystemLocaleLongDate);
104​ a = menu->addAction(s);
105​ a->setData(s);
106​ }
107​
108​ menu->addSeparator();
109​
110​ s = date.toString(Qt::SystemLocaleShortDate) + ws + time.toString(Qt::SystemLocaleShortDate);
111​ a = menu->addAction(s);
112​ a->setData(s);
113​ if (m_secondsIncluded) {
114​ s = date.toString(Qt::SystemLocaleShortDate) + ws + time.toString(Qt::SystemLocaleLongDate).replace(rx, "");
115​ a = menu->addAction(s);
116​ a->setData(s);
117​ s = date.toString(Qt::SystemLocaleShortDate) + ws + time.toString(Qt::SystemLocaleLongDate);
davidedmundsonUnsubmitted
Not Done

lets not put POC in code that we want to merge.

Also, by your own comment later, this leaks.

davidedmundson: lets not put POC in code that we want to merge. Also, by your own comment later, this leaks.
broulikUnsubmitted
Not Done

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

broulik: You don't need to create this menu here, `QMenu::addMenu(QString)` that you're using below…
118​ a = menu->addAction(s);
119​ a->setData(s);
120​ }
121​ s = date.toString(Qt::ISODate) + ws + time.toString(Qt::SystemLocaleShortDate);
broulikUnsubmitted
Not Done

That optimism ;)

broulik: That optimism ;)
122​ a = menu->addAction(s);
123​ a->setData(s);
124​ if (m_secondsIncluded) {
125​ s = date.toString(Qt::ISODate) + ws + time.toString(Qt::SystemLocaleLongDate).replace(rx, "");
126​ a = menu->addAction(s);
127​ a->setData(s);
128​ s = date.toString(Qt::ISODate) + ws + time.toString(Qt::SystemLocaleLongDate);
129​ a = menu->addAction(s);
130​ a->setData(s);
131​ }
132​ s = date.toString(Qt::SystemLocaleLongDate) + ws + time.toString(Qt::SystemLocaleShortDate);
133​ a = menu->addAction(s);
broulikUnsubmitted
Not Done

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)

broulik: I would prefer having proper `i18n` with placeholders here (I think the word puzzles above are…
134​ a->setData(s);
135​
136​ menu->addSeparator();
137​
138​ QMenu *otherCalendarsMenu = menu->addMenu(i18n("Other Calendars"));
139​
140​/* Add ICU Calendars if QLocale is ready for
141​ Chinese, Coptic, Ethiopic, (Gregorian), Hebrew, Indian, Islamic, Persian
broulikUnsubmitted
Not Done

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

broulik: You could just do the lambda inline here, I don't see `l` being used elsewhere
142​
143​ otherCalendarsMenu->addSeparator();
144​*/
davidedmundsonUnsubmitted
Not Done

We want "menu" not "this" as the second context.

Otherwise if you delete the clipboardmenu class first, this leaks
If you delete the menu first, this crashes.

Neither should happen, but better to be safer.

davidedmundson: We want "menu" not "this" as the second context. Otherwise if you delete the clipboardmenu…
broulikUnsubmitted
Not Done

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

connect(action, &QObject::destroyed, menu, &QObject::deleteLater);
broulik: I know this is from the original code but I think you could optimize it to ``` connect(action…
145​ s = QString::number(m_currentDate.toMSecsSinceEpoch() / 1000);
146​ a = otherCalendarsMenu->addAction(i18nc("unix timestamp (seconds since 1.1.1970)", "%1 (UNIX Time)", s));
147​ a->setData(s);
148​ s = QString::number(qreal(2440587.5) + qreal(m_currentDate.toMSecsSinceEpoch()) / qreal(86400000), 'f', 5);
149​ a = otherCalendarsMenu->addAction(i18nc("for astronomers (days and decimals since ~7000 years ago)", "%1 (Julian Date)", s));
150​ a->setData(s);
151​ });
152​
153​ connect(menu, &QMenu::triggered, menu, [](QAction *action) {
154​ qApp->clipboard()->setText(action->data().toString());
155​ qApp->clipboard()->setText(action->data().toString(), QClipboard::Selection);
156​ });
157​
158​ // QMenu cannot have QAction as parent and setMenu doesn't take ownership
159​ connect(action, &QObject::destroyed, menu, &QObject::deleteLater);
160​
161​ action->setMenu(menu);
162​}