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
