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

Authored by bschiffner on Jun 11 2017, 2:16 PM.

Details

Summary

Fixes https://bugs.kde.org/show_bug.cgi?id=355190

BUG: 355190
FIXED-IN: 5.13.0

It supersedes https://phabricator.kde.org/D1350 which was accepted but didn't make it into plasma-workspace.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bschiffner created this revision.Jun 11 2017, 2:16 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 11 2017, 2:16 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bschiffner updated this revision to Diff 15348.Jun 11 2017, 2:26 PM
  • [Digital Clock] Allow copying current date and time to clipboard (polishing)

KDE 4.11 was this way

diff 2 structures the original patch from Kai Uwe by a clear arrangement of date/time/datetime and size.

It separates data shown from data to be copied.

This prevents automagically added access codes to be copied.
This enables comments in the entries not to be copied too.

The patch adds a submenu intended for other calendars as they get doable with a more advanced QLocale.

As a POC it shows now UNIX Time and Julian Date.

(Thanks Konrad Rosenbaum for nurturing.)

broulik edited edge metadata.Jun 12 2017, 8:29 AM

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.

applets/digital-clock/plugin/clipboardmenu.cpp
84

Prefer QString() over an empty literal ""

117

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

121

That optimism ;)

133

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)

141

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

144

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

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

It's a bit of a shame that we're mixing two data sources in the same applet, but I guess we're limited by whatever works.

applets/digital-clock/plugin/clipboardmenu.cpp
84

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

Does every locale use : as a separator?

117

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

Also, by your own comment later, this leaks.

144

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.

bschiffner updated this revision to Diff 15381.Jun 12 2017, 2:51 PM

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

Thanks for the feedback.
It was integrated to my best understanding and works for me.

Why "^[0-9:]"? Sometimes it is just fun using the biggest sledgehammer. But in all respect: it is even gender neutral to be smashed this way!
(I tried something to do it more politely but think of rigt to left languages, unknown literals etc. So I did this bet.)

The QML / once a minute update problem (seconds all time 0) I didn't notice until now (4 PC/Notebook).

So, the question is what to do regarding the seconds :/ I couldn't figure out a way to cause a refresh of the data source. (A hack could be to disconnect and connect sources before showing the menu but urgh).

In doubt we can just get rid of the seconds altogether... :/

applets/digital-clock/package/contents/ui/DigitalClock.qml
74

I just figured since ClipboardMenu is a singleton-type, it's shared between all digital clock applets. You can have different timezones in each. So what we instead should do is:

Connections {
    target: plasmoid
    onContextualActionsAboutToShow: {
        ClipboardMenu.currentDate = main.currentTime;
    }
}

This way we always only update when the respective context menu is opened.

Sorry for not understanding the (perhaps your?) problem with the seconds.
Do you like to express that you:
1.) do not get seconds at all (erverytime 00 or 59) or
2.) do not get an actualized value of seconds (or time) in very the moment of clicking the intended format or
3.) can have a platform related problem or
4.) someting else?

On all PCs (x86/AMD64) I use the patch on i get the time of opening the context menu with a "correct" value of seconds. This value can be copied from the clipboard anywhere.

And you are right: there is no urgent need for a display of seconds and if not displayed it would remove some "visual bloat".
(Nevertheless I like this "geekiness" for my personal use: excel sheets, exif correction, ...)

If you do not have seconds enabled in digital clock, the data source is only updated once every minute. You might not see this if you try it right after startup but eventually the seconds will always be :00 making it pointless to copy seconds. Ideally, we find a way to update the data source in the instance the menu is opened but I didn't find one.

Ah, I got it now and will try it.

But if the user decides not to see seconds displayed, it can be interpreted plausibly he doesn't want to see them in the clipboard too.
Pseudocode: if config.seconds() then ...

But if the user decides not to see seconds displayed, it can be interpreted plausibly he doesn't want to see them in the clipboard too.

Good idea! Why didn't I think of this. I think this is a perfectly valid assumption. Can you take a look at this (and possibly my other comment)? Otherwise I can also take on and finish your patch from here, we had you waiting long enough :/ and would really nice to finally have this in the next release.

I think you get it much faster done. Please do it.
I'am sitting in a hotel now without tools do to something software related :-(

There's still two weeks left until feature freeze for 5.11 on 14 September ;)

bschiffner updated this revision to Diff 30975.Mar 30 2018, 9:56 PM

It supressed the seconds to be copied into the clipboard in case of the per minute update of the display.

Missing: I don't know how to make the result of the action available to the MMB (middle mouse button).

ping ...

Kai Uwe, David any ideas how to proceed with the change last week?

Bernhard

I read this (unfortunately unpleasant) bug report the other day and was just asking on IRC how to put data the clipboard. I'll be studying this closely - nice work!

Looks good

I don't know how to make the result of the action available to the MMB (middle mouse button).

QClipboard::setText() has a mode argument that specifies which buffer it should use.
Default is QClipboard::Clipboard (the Ctrl+V one), just call it again with QClipboard::Selection for middle mouse paste

applets/digital-clock/package/contents/ui/DigitalClock.qml
74

You didn't address this comment in your updated patch

bschiffner updated this revision to Diff 31690.Apr 8 2018, 5:02 PM

works for me now / ready to ship

Thanks Kai-Uwe for your advices with the qml problems.
I'am far from understanding this topic. Mostly cargo cult, but nevertheless working.

broulik accepted this revision.Apr 8 2018, 8:46 PM

Let's go for it now! Thanks for patience and apologies it took so long :/
Do you have commit access? else I need your email address and can push it on your behalf.

applets/digital-clock/package/contents/ui/DigitalClock.qml
77 ↗(On Diff #31690)

You can merge the two :)

onContextualActionsAboutToShow: {
    ClipboardMenu.secondsIncluded = main.showSeconds;
    ClipboardMenu.currentDate = main.currentTime;
}
This revision is now accepted and ready to land.Apr 8 2018, 8:46 PM

No, I don't have commit access.
"Bernhard Schiffner" <bernhard.schiffner@gmx.net>

But why my email, Kai-Uwe did 98,7% of the work ...

@Kai-Uwe: Can you do the merge (DigitalClock.qml#77) before pushing, please?

ngraham edited the summary of this revision. (Show Details)Apr 8 2018, 9:30 PM
This revision was automatically updated to reflect the committed changes.