D7087: Add "Copy Info" button to the About System KCM

[Henrik Fehlauer]

rkflx added a comment.


  @dhaumann 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.
  
  @gregormi After re-reading all comments, I think the approach of using `text()` to get to the translated string is great. As for adding more/other information, let's save that for a future patch.
  
  Copy Info still looks a bit odd, and I wonder how that will be translated. Perhaps Copy Information would be better, but actually I'd rather see Copy to Clipboard 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 `about:support`, too.
  
  ---
  
  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:

INLINE COMMENTS

> Module.cpp:246
> +
> +void Module::copyToClipboard()
> +{

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.

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.

> Module.cpp:253
> +    if (!ui->plasmaLabel->isHidden()) {
> +        text += i18n("%1 %2", ui->plasma->text(), ui->plasmaLabel->text()) + QStringLiteral("\n");
> +    }

@dhaumann I wonder why you suggested to use `i18n` here too, as `text()` should already give you the translated text. How do you expect translators to translate `"%1 %2"`?

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?

> Module.h:68-69
> +     * Copies the software and hardware information to clipboard.
> +     * The label language is currently English only
> +     * because the Plasma development language is English.
> +     */

Is this comment still accurate?

> Module.ui:333-338
> +     <property name="sizePolicy">
> +      <sizepolicy hsizetype="Fixed" vsizetype="Fixed">
> +       <horstretch>0</horstretch>
> +       <verstretch>0</verstretch>
> +      </sizepolicy>
> +     </property>

Do you need this? Pressing the Reset button for that property in Qt Designer removes this for me.

> Module.ui:347
> +      <iconset theme="edit-copy">
> +       <normaloff>.</normaloff>.</iconset>
> +     </property>

This looks odd.

> Module.ui:349-351
> +     <property name="shortcut">
> +      <string>Ctrl+C</string>
> +     </property>

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 [Ctrl] + [C].

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. `KStandardAction::Copy` or `QKeySequence::Copy`, instead of setting shortcut and icon manually.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180526/a7c93dde/attachment.html>