<table><tr><td style="">broulik added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D6183" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Thanks a lot for picking this up!</p>
<p>I have some minor nitpicks below, otherwise looking very good. You don't need to mention "Fixes <a href="https://bugs.kde.org/show_bug.cgi?id=355190"" class="remarkup-link" target="_blank" rel="noreferrer">https://bugs.kde.org/show_bug.cgi?id=355190"</a> in the commit message, the "BUG: 355190" already does this, also causes Bugzilla to auto-close the bug once committed, etc.</p>
<p>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).</p>
<p>I will try to figure out a way to force an update explicitly from QML, so we can finally have this in.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6183#inline-25419" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">clipboardmenu.cpp:83</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">s</span> <span style="color: #aa2211">=</span> <span class="n">time</span><span class="p">.</span><span class="n">toString</span><span class="p">(</span><span class="n">Qt</span><span style="color: #aa2211">::</span><span class="n">SystemLocaleLongDate</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">s</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="n">rx</span><span class="p">,</span> <span style="color: #766510">""</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">a</span> <span style="color: #aa2211">=</span> <span class="n">menu</span><span style="color: #aa2211">-></span><span class="n">addAction</span><span class="p">(</span><span class="n">s</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Prefer <tt style="background: #ebebeb; font-size: 13px;">QString()</tt> over an empty literal <tt style="background: #ebebeb; font-size: 13px;">""</tt></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6183#inline-25423" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">clipboardmenu.cpp:116</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QMenu</span> <span style="color: #aa2211">*</span><span class="n">menu_cal</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QMenu</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">menu_cal</span><span style="color: #aa2211">-></span><span class="n">clear</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You don't need to create this menu here, <tt style="background: #ebebeb; font-size: 13px;">QMenu::addMenu(QString)</tt> that you're using below already creates a new menu for you (basically just remove this and the next line).</p>
<p style="padding: 0; margin: 8px;">Also, we typically use camel-case instead of underscores, eg. <tt style="background: #ebebeb; font-size: 13px;">otherCalendarsMenu</tt></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6183#inline-25422" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">clipboardmenu.cpp:120</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">/* Add ICU Calendars if QLocale is ready for</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> Chinese</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That optimism ;)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6183#inline-25418" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">clipboardmenu.cpp:132</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">s</span> <span style="color: #aa2211">=</span> <span class="n">QString</span><span style="color: #aa2211">::</span><span class="n">number</span><span class="p">(</span><span class="n">m_currentDate</span><span class="p">.</span><span class="n">toMSecsSinceEpoch</span><span class="p">()</span> <span style="color: #aa2211">/</span> <span style="color: #601200">1000</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">a</span> <span style="color: #aa2211">=</span> <span class="n">menu_cal</span><span style="color: #aa2211">-></span><span class="n">addAction</span><span class="p">(</span><span class="n">s</span> <span style="color: #aa2211">+</span> <span class="n">ws</span> <span style="color: #aa2211">+</span> <span style="color: #766510">"("</span> <span style="color: #aa2211">+</span> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"UNIX Time"</span><span class="p">)</span> <span style="color: #aa2211">+</span> <span style="color: #766510">")"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">a</span><span style="color: #aa2211">-></span><span class="n">setData</span><span class="p">(</span><span class="n">s</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would prefer having proper <tt style="background: #ebebeb; font-size: 13px;">i18n</tt> 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:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">i18nc("placeholder is unix timestamp", "%1 (UNIX Time)", s);</pre></div>
<p style="padding: 0; margin: 8px;">(the <tt style="background: #ebebeb; font-size: 13px;">c</tt> means "context", so the first string gives translators a bit of an explanation what this is about)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6183#inline-25415" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">clipboardmenu.cpp:140</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">connect</span><span class="p">(</span><span class="n">menu</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">QMenu</span><span style="color: #aa2211">::</span><span class="n">triggered</span><span class="p">,</span> <span class="n">menu</span><span class="p">,</span> <span class="n">l</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You could just do the lambda inline here, I don't see <tt style="background: #ebebeb; font-size: 13px;">l</tt> being used elsewhere</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6183#inline-25416" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">clipboardmenu.cpp:143</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// QMenu cannot have QAction as parent and setMenu doesn't take ownership</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">connect</span><span class="p">(</span><span class="n">action</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">QObject</span><span style="color: #aa2211">::</span><span class="n">destroyed</span><span class="p">,</span> <span style="color: #aa4000">this</span><span class="p">,</span> <span class="p">[</span><span class="n">menu</span><span class="p">]</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">menu</span><span style="color: #aa2211">-></span><span class="n">deleteLater</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I know this is from the original code but I think you could optimize it to</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">connect(action, &QObject::destroyed, menu, &QObject::deleteLater);</pre></div></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R120 Plasma Workspace</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6183" rel="noreferrer">https://phabricator.kde.org/D6183</a></div></div><br /><div><strong>To: </strong>bschiffner, Plasma, broulik<br /><strong>Cc: </strong>plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas<br /></div>