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

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:ffa23acf440b: [Digital Clock] Allow copying current date 
and time to clipboard (authored by bschiffner, committed by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D6183?vs=31690&id=31855#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6183?vs=31690&id=31855

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

AFFECTED FILES
  applets/digital-clock/package/contents/ui/DigitalClock.qml
  applets/digital-clock/package/contents/ui/main.qml
  applets/digital-clock/plugin/CMakeLists.txt
  applets/digital-clock/plugin/clipboardmenu.cpp
  applets/digital-clock/plugin/clipboardmenu.h
  applets/digital-clock/plugin/digitalclockplugin.cpp

To: bschiffner, #plasma, broulik
Cc: sharvey, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Bernhard Schiffner]

bschiffner added a comment.


  No, I don't have commit access.
  "Bernhard Schiffner" 
  
  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?

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

To: bschiffner, #plasma, broulik
Cc: sharvey, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.
This revision is now accepted and ready to land.


  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.

INLINE COMMENTS

> DigitalClock.qml:77
> +onContextualActionsAboutToShow: {
> +ClipboardMenu.secondsIncluded =main.showSeconds;
> +}

You can merge the two :)

  onContextualActionsAboutToShow: {
  ClipboardMenu.secondsIncluded = main.showSeconds;
  ClipboardMenu.currentDate = main.currentTime;
  }

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

To: bschiffner, #plasma, broulik
Cc: sharvey, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Bernhard Schiffner]

bschiffner updated this revision to Diff 31690.
bschiffner added a comment.


  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.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6183?vs=30975&id=31690

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

AFFECTED FILES
  applets/digital-clock/package/contents/ui/DigitalClock.qml
  applets/digital-clock/package/contents/ui/main.qml
  applets/digital-clock/plugin/CMakeLists.txt
  applets/digital-clock/plugin/clipboardmenu.cpp
  applets/digital-clock/plugin/clipboardmenu.h
  applets/digital-clock/plugin/digitalclockplugin.cpp

To: bschiffner, #plasma, broulik
Cc: sharvey, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Kai Uwe Broulik]

broulik added a comment.


  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

INLINE COMMENTS

> broulik wrote in 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.

You didn't address this comment in your updated patch

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

To: bschiffner, #plasma, broulik
Cc: sharvey, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  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!

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

To: bschiffner, #plasma, broulik
Cc: sharvey, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Bernhard Schiffner]

bschiffner added a comment.


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

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Bernhard Schiffner]

bschiffner updated this revision to Diff 30975.
bschiffner added a comment.


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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6183?vs=15381&id=30975

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

AFFECTED FILES
  applets/digital-clock/package/contents/ui/DigitalClock.qml
  applets/digital-clock/package/contents/ui/main.qml
  applets/digital-clock/plugin/CMakeLists.txt
  applets/digital-clock/plugin/clipboardmenu.cpp
  applets/digital-clock/plugin/clipboardmenu.h
  applets/digital-clock/plugin/digitalclockplugin.cpp

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Bernhard Schiffner]

bschiffner added a comment.


  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 :-(

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Kai Uwe Broulik]

broulik added a comment.


  > 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.

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Bernhard Schiffner]

bschiffner added a comment.


  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 ...

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Kai Uwe Broulik]

broulik added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Bernhard Schiffner]

bschiffner added a comment.


  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, ...)

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Kai Uwe Broulik]

broulik added a comment.


  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... :/

INLINE COMMENTS

> DigitalClock.qml:74
>  
> +Binding {
> +target: ClipboardMenu

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.

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Bernhard Schiffner]

bschiffner added a comment.


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

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Bernhard Schiffner]

bschiffner updated this revision to Diff 15381.
bschiffner added a comment.


  Upating https://phabricator.kde.org/D6183: [Digital Clock] Allow copying 
current date and time to clipboard

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6183?vs=15348&id=15381

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

AFFECTED FILES
  applets/digital-clock/package/contents/ui/DigitalClock.qml
  applets/digital-clock/package/contents/ui/main.qml
  applets/digital-clock/plugin/CMakeLists.txt
  applets/digital-clock/plugin/clipboardmenu.cpp
  applets/digital-clock/plugin/clipboardmenu.h
  applets/digital-clock/plugin/digitalclockplugin.cpp

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

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

[David Edmundson]

davidedmundson added a comment.


  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.

INLINE COMMENTS

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

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

Does every locale use : as a separator?

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

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

Also, by your own comment later, this leaks.

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

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.

REPOSITORY
  R120 Plasma Workspace

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

To: bschiffner, #plasma, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

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

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

[Bernhard Schiffner]

bschiffner added a comment.


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

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

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

[Bernhard Schiffner]

bschiffner added a comment.


  KDE 4.11 was this way
  
  F3781189: copy_datetime_to_clipboard_old.png 


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

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

[Bernhard Schiffner]

bschiffner updated this revision to Diff 15348.
bschiffner added a comment.


  - [Digital Clock] Allow copying current date and time to clipboard (polishing)

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6183?vs=15347&id=15348

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

AFFECTED FILES
  applets/digital-clock/package/contents/ui/DigitalClock.qml
  applets/digital-clock/package/contents/ui/main.qml
  applets/digital-clock/plugin/CMakeLists.txt
  applets/digital-clock/plugin/clipboardmenu.cpp
  applets/digital-clock/plugin/clipboardmenu.h
  applets/digital-clock/plugin/digitalclockplugin.cpp

To: bschiffner, #plasma, broulik
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

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

[Bernhard Schiffner]

bschiffner created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Fixes https://bugs.kde.org/show_bug.cgi?id=355190
  
  BUG: 355190
  FIXED-IN: 5.11.0
  
  It supersedes https://phabricator.kde.org/D1350 which was accepted but didn't 
make it into plasma-workspace.

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/digital-clock/package/contents/ui/DigitalClock.qml
  applets/digital-clock/package/contents/ui/main.qml
  applets/digital-clock/plugin/CMakeLists.txt
  applets/digital-clock/plugin/clipboardmenu.cpp
  applets/digital-clock/plugin/clipboardmenu.h
  applets/digital-clock/plugin/digitalclockplugin.cpp

To: bschiffner, #plasma, broulik
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas