<table><tr><td style="">rkflx 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/D7087">View Revision</a></tr></table><br /><div><div><p><a href="https://phabricator.kde.org/p/dhaumann/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@dhaumann</a> Your status is still set to "Requesting changes" and thus blocks the patch from landing, but as far as I can see the general concern has been resolved. See inline comment for a question concerning the implementation.</p>
<p><a href="https://phabricator.kde.org/p/gregormi/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@gregormi</a> After re-reading all comments, I think the approach of using <tt style="background: #ebebeb; font-size: 13px;">text()</tt> to get to the translated string is great. As for adding more/other information, let's save that for a future patch.</p>
<p><span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Copy Info</span></span></span> still looks a bit odd, and I wonder how that will be translated. Perhaps <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Copy Information</span></span></span> would be better, but actually I'd rather see <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Copy to Clipboard</span></span></span> here. I don't think it's too long contrary to what you mentioned earlier, and the same terminology is used in Spectacle. This has also been suggested by other commenters multiple times, and is the wording of choice in Firefox's <tt style="background: #ebebeb; font-size: 13px;">about:support</tt>, too.</p>
<hr class="remarkup-hr" />
<p>I'm not set as a reviewer, but given you want to land this now and asked for feedback, I had a more detailed look:</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/D7087#inline-67931">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Module.cpp:246</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: #aa4000">void</span> <span class="n">Module</span><span style="color: #aa2211">::</span><span class="n">copyToClipboard</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Probably not strictly required for this patch, but it would be nicer to refactor this in such a way that adding another string to the UI does not require adding it here too.</p>
<p style="padding: 0; margin: 8px;">Perhaps this can be achieved by creating a list of label/version pairs, and then iterating through that list both when creating the UI and when generating the text to copy.</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/D7087#inline-67932">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Module.cpp:253</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: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">ui</span><span style="color: #aa2211">-></span><span class="n">plasmaLabel</span><span style="color: #aa2211">-></span><span class="n">isHidden</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">text</span> <span style="color: #aa2211">+=</span> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"%1 %2"</span><span class="p">,</span> <span class="n">ui</span><span style="color: #aa2211">-></span><span class="n">plasma</span><span style="color: #aa2211">-></span><span class="n">text</span><span class="p">(),</span> <span class="n">ui</span><span style="color: #aa2211">-></span><span class="n">plasmaLabel</span><span style="color: #aa2211">-></span><span class="n">text</span><span class="p">())</span> <span style="color: #aa2211">+</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"</span><span style="color: #bb6622">\n</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="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><a href="https://phabricator.kde.org/p/dhaumann/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@dhaumann</a> I wonder why you suggested to use <tt style="background: #ebebeb; font-size: 13px;">i18n</tt> here too, as <tt style="background: #ebebeb; font-size: 13px;">text()</tt> should already give you the translated text. How do you expect translators to translate <tt style="background: #ebebeb; font-size: 13px;">"%1 %2"</tt>?</p>
<p style="padding: 0; margin: 8px;">The only challenge is to account for RTL languages, but this could be detected to swap the order (and possibly add an RTL BOM?). Do we have experts who could advice on that?</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/D7087#inline-67929">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Module.h:68-69</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"> * Copies the software and hardware information to clipboard.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * The label language is currently English only</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * because the Plasma development language is English.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Is this comment still accurate?</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/D7087#inline-67938">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Module.ui:333-338</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);"> <property name="sizePolicy">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <sizepolicy hsizetype="Fixed" vsizetype="Fixed">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <horstretch>0</horstretch>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <verstretch>0</verstretch>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> </sizepolicy>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> </property>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Do you need this? Pressing the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Reset</span></span></span> button for that property in Qt Designer removes this for me.</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/D7087#inline-67939">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Module.ui:347</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);"> <iconset theme="edit-copy">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <normaloff>.</normaloff>.</iconset>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> </property>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This looks odd.</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/D7087#inline-67940">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Module.ui:349-351</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);"> <property name="shortcut">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <string>Ctrl+C</string>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> </property>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">For me the shortcut actually works, but ideally this should use the standard shortcut for copying, which the user might have set to something different than <kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Ctrl</kbd><span class="kbd-join" style="padding: 0 4px; color: #92969D;">+</span><kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">C</kbd>.</p>
<p style="padding: 0; margin: 8px;">You could try to look at how it works in Spectacle. I suspect you'd need to add the appropriate action or standard shortcut for that, e.g. <tt style="background: #ebebeb; font-size: 13px;">KStandardAction::Copy</tt> or <tt style="background: #ebebeb; font-size: 13px;">QKeySequence::Copy</tt>, instead of setting shortcut and icon manually.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R102 KInfoCenter</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7087">https://phabricator.kde.org/D7087</a></div></div><br /><div><strong>To: </strong>gregormi, ngraham, dhaumann<br /><strong>Cc: </strong>rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart<br /></div>