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

[gregormi]

This revision was automatically updated to reflect the committed changes.
Closed by commit R102:c0f28efd280f: Add Copy Info button to the 
About System KCM (authored by gregormi).

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=40853=40854

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 40853.
gregormi added a comment.


  - Use QVector instead of QList
  - Use the term "Operating System:" instead of "Distro"

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=39321=40853

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> dhaumann wrote in Module.cpp:162
> Is "Distro" a proper English term? Should it meaybe read "Distribution"?

"Distro" is slang, but very common. I'm not sure "Distribution" is much better 
if were considering average people. They would consider the software on their 
computer to be an "Operating System."

For that matter, distros themselves might appreciate being referred to as 
operating systems here. Perhaps we should use that term instead.

REPOSITORY
  R102 KInfoCenter

BRANCH
  arcpatch-D7087_1

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Dominik Haumann]

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


  Ok from my side, although it still *feels* a bit messy :-)
  
  - please consider using QVector before committing
  - possibly fix the "Distro" thing, if applicable

INLINE COMMENTS

> Module.cpp:162
>  
> +const auto dummyDistroDescriptionLabel = new QLabel(i18nc("@title:row", 
> "Distro:"), this);
> +dummyDistroDescriptionLabel->hide();

Is "Distro" a proper English term? Should it meaybe read "Distribution"?

> Module.h:73
> +
> +QList > labelsForClipboard;
> +

For future: Please pretty much always prefer QVector over QList.

REPOSITORY
  R102 KInfoCenter

BRANCH
  arcpatch-D7087_1

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 39321.
gregormi added a comment.


  Rebase on master

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=37787=39321

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Henrik Fehlauer]

rkflx added a comment.


  Happy birthday D7087 , you are now 1 year 
old.
  
  @dhaumann Could you either accept, resign or tell us what still needs 
changing? `arc land` will currently block because your reviewer status is still 
set to "requesting changes".
  
  @gregormi Could you rebase your patch on master?
  
  Thanks!

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.


  @gregormi It's been a long time coming, but now the patch LGTM  (BTW, the 
patch does not apply cleanly after recent changes in master, so make sure to 
rebase properly…)
  
  @dhaumann As you "requested changes", could you check whether the patch is 
good to go now? (Currently Phabricator blocks landing the patch because of 
that.)
  
  In D7087#292261 , @gregormi wrote:
  
  > What do you think about this minor layout change? It is noticeable when the 
window is resized horizontally. Instead of centering the grid's middle line, it 
makes the left and right padding space to the window border equal which looks 
more pleasing at least to my eye.
  
  
  Yeah, I noticed this too. Much better after your update! It's slightly 
off-topic for the patch, but now that you added it, just keep it…
  
  > For reference: the layout in master:
  
  Yup, your patch will be improving it quite a lot. You could add a small note 
to the summary to indicate you had to redo the layout a bit.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 37787.
gregormi added a comment.


  - Module.ui: add horizontal spacers

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=37786=37787

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi added a comment.


  What do you think about this minor layout change? It is noticeable when the 
window is resized horizontally. Instead of centering the grid's middle line, it 
makes the left and right padding space to the window border equal which looks 
more pleasing at least to my eye.
  
  Before (system installed Info Center):
  
  F6106142: grafik.png 
  
  After (this change):
  
  F6106144: grafik.png 
  
  New layout with spacers that push the grid columns to their minimum width:
  
  F6106150: grafik.png 
  
  For reference: the layout in master:
  
  F6106171: grafik.png 

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 37786.
gregormi marked 4 inline comments as done.
gregormi added a comment.


  - Module.ui: Fix glitch near KernelName
  - Avoid double connection of copyToClipboard
  - Hide dummy label
  - Better naming of loop variable and optimize loop code

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=36893=37786

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi added inline comments.

INLINE COMMENTS

> rkflx wrote in Module.cpp:263
> Coming back to this after a month, I now wonder what `p` stands for, which 
> might indicate that variable could get a better name…

p stands for labelPair: renaming is a good idea. Done.

> rkflx wrote in Module.cpp:260
> I'm afraid you missed the "hidden" part, so it shows up right in front of the 
> distro logo ;)
> 
> Adding
> 
>   dummyDistroDescriptionLabel->hide();
> 
> where you are creating the label solves the issue for me.

dummy hide: Oh sorry, I should have seen that myself. Fixed it.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Henrik Fehlauer]

rkflx added a comment.


  In D7087#284846 , @gregormi wrote:
  
  > I fixed the layout and it looks now like this in Qt Designer
  
  
  Awesome, I guess you got the overall gist ;)
  
  > The blue arrow shows a difference to your proposal. Do you think the grid 
layout there is ok?
  
  I probably used a vertical layout for this (IIRC), which should work the same 
but is a bit simpler. Same thing for the purely horizontal layout at the 
bottom, where you also used a grid. Again, this does not really matter that 
much.
  
  Nevertheless, somehow there seems to be a glitch near `KernelName`, so the 
values for both kernel and architecture are displaced a bit to the bottom. When 
I break the layout and then redo it, it works fine for me. Not sure how you got 
into that situation ;) Let me know if I should upload my version if you cannot 
get it fixed with your Qt Designer.
  
  ---
  
  In D7087#284855 , @gregormi wrote:
  
  > I added some qDebug code in Module::copyToClipboard()
  >  […]
  >  Any idea why the slot method is called twice?
  
  
  `Module::load()` is called twice (see comment in constructor or D2300 
), so you have two connections. In general 
you could use a `Qt::UniqueConnection` in your `connect` to avoid this, but I 
think here it's actually better to move your setup code (i.e. everything except 
for `labelsForClipboard.clear()`) to the constructor instead.

INLINE COMMENTS

> Module.cpp:161
>  
> +auto dummyDistroDescriptionLabel = new QLabel(i18nc("@title:row", 
> "Distro:"), this);
> +labelsForClipboard << qMakePair(dummyDistroDescriptionLabel, 
> ui->nameVersionLabel);

You could add a `const`.

> Module.cpp:263
> +// note that this loop does not necessarily represent the same order as 
> in the GUI
> +for (auto p : qAsConst(labelsForClipboard)) {
> +auto descriptionLabelText = p.first->text();

Coming back to this after a month, I now wonder what `p` stands for, which 
might indicate that variable could get a better name…

> gregormi wrote in Module.cpp:260
> Good idea. DONE.

I'm afraid you missed the "hidden" part, so it shows up right in front of the 
distro logo ;)

Adding

  dummyDistroDescriptionLabel->hide();

where you are creating the label solves the issue for me.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi marked 6 inline comments as done.
gregormi added inline comments.

INLINE COMMENTS

> rkflx wrote in Module.cpp:260
> Maybe you could avoid this special case if you added a hidden label in 
> `loadSoftware`?
> 
> Also, "label in … the button" sounds a bit odd. Perhaps "label" would be 
> enough, or use `@title:row`? (See 
> https://api.kde.org/frameworks/ki18n/html/prg_guide.html)

Good idea. DONE.

> rkflx wrote in Module.ui:349-351
> Works perfectly fine for me. I think there might be something off with your 
> local shortcuts setup.

Ok. Then I leave it like that.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 36893.
gregormi added a comment.


  - Rework the Module.ui layout to fix the spacing
  - Add dummy label to remove special case for "Distro"
  - Use C++11 for loop and add qAsConst
  - Make RTL aware by not removing the colon
  - Improve translation context message

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=35167=36893

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi added a comment.


  I added some qDebug code in Module::copyToClipboard() and noticed that the 
method is always called twice when the Copy to clipboard button is clicked.
  
  When I remove the line 125 in Module.cpp: connect(ui->pushButtonCopyInfo, 
::clicked, this, ::copyToClipboard);
  then the nothing happens when the button is clicked.
  
  With a web search I only found this 
https://stackoverflow.com/questions/34416103/function-connected-to-the-button-is-called-twice-after-one-click
 which is not the case here. Any idea why the slot method is called twice?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi added a comment.


  Thanks Henrik for the review. I fixed the layout and it looks now like this 
in Qt Designer:
  
  F5972316: grafik.png 
  
  The blue arrow shows a difference to your proposal. Do you think the grid 
layout there is ok? I push an update when I have the other things ready.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Henrik Fehlauer]

rkflx added a comment.


  Thanks for the update and sorry it took me a while to look at it.
  
  > If I remove that part, the button gets wider; I would prefer to keep it as 
small as possible.
  
  Ah, now I see where the problem is: You are inserting the button to a grid 
layout, which results in adding the button to the left of the logo (when 
looking at the left "columns" of the grid) instead of leaving the padding on 
the left alone and only adding it below the informational content and aligned 
to the left. This then causes the button to become wider.
  
  By fixing the overall layout and adding appropriate spacers, you should be 
able to go without setting a `Fixed` size policy for the button. Fixing the 
layout is required anyway, not sure why I didn't notice the huge gap on the 
left before. Try changing the window's width and compare to how it behaved 
before your patch to see what I mean.
  
  I'd arrange the UI like this:
  
  F5885212: about-distro-layout.png 
  
  (Simply draw a selection rectangle around the labels, click on "Grid", align 
button with new spacer in horizontal layout, remove 2 spacers, click on 
"Vertical Layout", adjust size.)

INLINE COMMENTS

> Module.cpp:260
> +if (!ui->nameVersionLabel->isHidden()) {
> +text += i18nc("one line in the information that goes to the 
> clipboard", "%1: %2", i18nc("label in the Copy to Clipboard button", 
> "Distro"), ui->nameVersionLabel->text()) + QStringLiteral("\n");
> +}

Maybe you could avoid this special case if you added a hidden label in 
`loadSoftware`?

Also, "label in … the button" sounds a bit odd. Perhaps "label" would be 
enough, or use `@title:row`? (See 
https://api.kde.org/frameworks/ki18n/html/prg_guide.html)

> Module.cpp:263
> +
> +foreach (auto p, labelsForClipboard) { // note that does not necessarily 
> represent the same order as in the GUI
> +auto descriptionLabelText = p.first->text();

I'd use the C++11 `for ( : )` here, and add `qAsConst` to the second argument 
since we can depend on Qt 5.7.

> Module.cpp:268
> +if (!valueLabel->isHidden()) {
> +if (descriptionLabelText.endsWith(":")) { // strip colon from 
> the end
> +descriptionLabelText = 
> descriptionLabelText.left(descriptionLabelText.length() - 1);

Is `endsWith` RTL aware? If not, it might be better to also check `startsWith`, 
however that assumes in most languages `:` is kept intact, which I'm not sure 
about.

Thinking about this again, why not

- keep the colon in the label
- don't add a colon in `%1 %2`
- change the context to `%1 is a label already including a colon, …`

> Module.cpp:271
> +}
> +text += i18nc("one line in the information that goes to the 
> clipboard", "%1: %2", descriptionLabelText, valueLabelText) + 
> QStringLiteral("\n");
> +}

> explain what the placeholders are meant to be. Leave it to the speakers of 
> the language the decision about the order.

I suspect @ltoscano meant something like this:

  %1 is a label, %2 is a corresponding value

"One line" is meaningless unless the translator also knows the other lines, and 
whether it is saved to the clipboard or a text file is probably also more 
confusing than helping.

> gregormi wrote in Module.ui:333-338
> If I remove that part, the button gets wider; I would prefer to keep it as 
> small as possible.

See reply in main comment.

> gregormi wrote in Module.ui:349-351
> The KStandardAction method seems only easily applicable with QToolButtons but 
> not with QPushButton. Furthermore, I got the "ambiguous key binding" when I 
> tried it. I now settled for QKeySequence::Copy which does not work for me but 
> at least results in no error message.

Works perfectly fine for me. I think there might be something off with your 
local shortcuts setup.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 35167.
gregormi added a comment.


  - use auto, add comment

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=35165=35167

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann, rkflx
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi updated this revision to Diff 35165.
gregormi marked 4 inline comments as done.
gregormi added a comment.


  - Handle colon (:) problem for translating and fix one label bug

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=35163=35165

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi marked an inline comment as done.
gregormi added inline comments.

INLINE COMMENTS

> rkflx wrote in Module.ui:333-338
> Do you need this? Pressing the Reset button for that property in Qt Designer 
> removes this for me.

If I remove that part, the button gets wider; I would prefer to keep it as 
small as possible.

> rkflx wrote in Module.ui:349-351
> 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.

The KStandardAction method seems only easily applicable with QToolButtons but 
not with QPushButton. Furthermore, I got the "ambiguous key binding" when I 
tried it. I now settled for QKeySequence::Copy which does not work for me but 
at least results in no error message.

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

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

[gregormi]

gregormi updated this revision to Diff 35163.
gregormi marked 3 inline comments as done.
gregormi added a comment.


  - Fix comment
  - Clean ui file
  - Shortcut handling

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=35158=35163

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi marked 5 inline comments as done.
gregormi added inline comments.

INLINE COMMENTS

> gregormi wrote in Module.cpp:246
> Yes, I also had the list idea in an earlier stage of the patch< but without 
> reusing the translated label text. I don't expect the information labels to 
> change often, so I think such a refactoring can be done at a later point in 
> time.

I changed my mind. With all the i18nc comments, I decided it is better to do 
the refactoring now.

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

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

[gregormi]

gregormi updated this revision to Diff 35158.
gregormi marked 2 inline comments as done.
gregormi added a comment.


  - Translate the Distro string
  - Rename button to 'Copy to Clipboard' for more clarity; and consistency with 
other applications
  - Use a QList of QPairs to collect labels and then use a loop

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=34111=35158

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[gregormi]

gregormi marked an inline comment as done.
gregormi added inline comments.

INLINE COMMENTS

> rkflx wrote in Module.cpp:246
> 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.

Yes, I also had the list idea in an earlier stage of the patch< but without 
reusing the translated label text. I don't expect the information labels to 
change often, so I think such a refactoring can be done at a later point in 
time.

> ltoscano wrote in Module.cpp:253
> Please don't write (just) "do this in this case", but explain what the 
> placeholders are meant to be. Leave it to the speakers of the language the 
> decision about the order.

I will do it like this:

i18nc("one line in the information that goes to the clipboard", "%1 %2", ...

I see one problem: %1 contains a trailing colon (:). So just reversing to "%2 
%1" would result in "openSUSE Tumbleweed Distro:". One solution would be to 
strip the colon from the %1 string and put it here: "%1: %2".

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

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

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> rkflx wrote in Module.cpp:253
> > `i18nc("reverse order in rtl languages", ...)`. Does that make sense?
> 
> It does, and it's probably easier for @gregormi to implement.

Please don't write (just) "do this in this case", but explain what the 
placeholders are meant to be. Leave it to the speakers of the language the 
decision about the order.

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

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

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> dhaumann wrote in Module.cpp:253
> The reason for this is that in a right-to-left language, you would translated 
> "%1 %2" by using the reverse, e.g. "%2 %1". The problem here is that a 
> translator will not know what to do with this. So what would help is to give 
> translators context, i.g. i18nc("reverse order in rtl languages", ...). Does 
> that make sense?

> `i18nc("reverse order in rtl languages", ...)`. Does that make sense?

It does, and it's probably easier for @gregormi to implement.

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

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

[Dominik Haumann]

dhaumann added a comment.


  Looks pretty good to me already. Is there anything that needs to be 
addressed? What do you think about i18nc?

INLINE COMMENTS

> rkflx wrote in Module.cpp:253
> @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?

The reason for this is that in a right-to-left language, you would translated 
"%1 %2" by using the reverse, e.g. "%2 %1". The problem here is that a 
translator will not know what to do with this. So what would help is to give 
translators context, i.g. i18nc("reverse order in rtl languages", ...). Does 
that make sense?

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

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
> + 
> +  
> +   0
> +   0
> +  
> + 

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

> Module.ui:347
> +  
> +   ..
> + 

This looks odd.

> Module.ui:349-351
> + 
> +  Ctrl+C
> + 

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

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

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

Is "Distro" not a translated string?

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

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

[gregormi]

gregormi added a comment.


  @ngraham , @rkflx, @dhaumann: Do you have any more suggestions to this? I 
think it is now ready to land.

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

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

[gregormi]

gregormi updated this revision to Diff 34111.
gregormi added a comment.


  - Apply comments by reviewers:
- move declaration near to usage
- Reuse label texts
  
  - Rename label to more speaking name

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=26765=34111

BRANCH
  arcpatch-D7087_1

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

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

[Henrik Fehlauer]

rkflx added a comment.


  In D7087#220623 , @gregormi wrote:
  
  > By the way, on Netrunner's Plasma the About System dialog looks like this: 
http://i1-news.softpedia-static.com/images/news2/netrunner-desktop-16-09-avalon-linux-os-is-out-with-kernel-4-7-kde-plasma-5-7-510007-3.jpg:
 it adds Graphics and Network information.
  
  
  Not only that, also "KDE Apps Version", just like I suggested. I guess it's 
open source, so try to find the patch or convince them to upstream it ;)
  
  > **And how **to go forward with this particular patch: If there are no 
objections I will proceed with Dominik's label suggestion as it is "simple, 
easy to understand and extend, and all the copy-to-clipboard code is in one 
place" and it looks good to solve the i18n issue.
  
  Sounds like a plan.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added a comment.


  In D7087#207306 , @rkflx wrote:
  
  > In D7087#207286 , @dhaumann wrote:
  >
  > > In fact, I wonder whether there already is a script or similar helper 
tools that give you this kind of system information. I currently do not 
remember, though... Anyone else?
  >
  >
  > Do you mean `inxi -Fxxxz`? Surely that's material for a different patch, 
but might be a nice idea to integrate. It's more hardware related, though, less 
about software versions.
  >
  > I guess for a useful https://bugs.kde.org report the only thing missing is 
the KDE Applications version and some info about the graphics card + driver.
  
  
  Oh, that's a nice tool. Having that in the menu would help not to forget its 
existence.
  
  In D7087#220251 , @rkflx wrote:
  
  > In D7087#207286 , @dhaumann wrote:
  >
  > > In fact, I wonder whether there already is a script or similar helper 
tools that give you this kind of system information. I currently do not 
remember, though... Anyone else?
  >
  >
  > Try #2: Did you mean this?:
  >
  >   qdbus org.kde.KWin /KWin supportInformation
  
  
  This output contains much KWin specific information. But why not.
  
  By the way, on Netrunner's Plasma the About System dialog looks like this: 
http://i1-news.softpedia-static.com/images/news2/netrunner-desktop-16-09-avalon-linux-os-is-out-with-kernel-4-7-kde-plasma-5-7-510007-3.jpg:
 it adds Graphics and Network information.
  
  **And how **to go forward with this particular patch: If there are no 
objections I will proceed with Dominik's label suggestion as it is "simple, 
easy to understand and extend, and all the copy-to-clipboard code is in one 
place" and it looks good to solve the i18n issue.

INLINE COMMENTS

> dhaumann wrote in Module.cpp:256
> Nitpicking: Why do you declare the clipboard here, when you use it only 10 
> lines later? :-) Better would be:
> 
>   QGuiApplication::clipboard()->setText(text);
> 
> So just one line at the end.

Good question :). I'll fix that.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Henrik Fehlauer]

rkflx added a comment.


  In D7087#207286 , @dhaumann wrote:
  
  > In fact, I wonder whether there already is a script or similar helper tools 
that give you this kind of system information. I currently do not remember, 
though... Anyone else?
  
  
  Try #2: Did you mean this?:
  
qdbus org.kde.KWin /KWin supportInformation

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Dominik Haumann]

dhaumann added a comment.


  One small suggestion to improve the clipboard code.

INLINE COMMENTS

> Module.cpp:256
> +{
> +auto clipboard = QGuiApplication::clipboard();
> +QMapIterator i(collectedData);

Nitpicking: Why do you declare the clipboard here, when you use it only 10 
lines later? :-) Better would be:

  QGuiApplication::clipboard()->setText(text);

So just one line at the end.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Dominik Haumann]

dhaumann added a comment.


  Thinking about it, what about this: The dialog already has all the QLabels. 
What you could do is something along the lines:
  
QString text;
if (!ui->plasma.text().isEmpty()) {
text += i18n("%1: %2", ui->plasmaLabel, ui->plasma.text()); 
}
if (!ui->bla.text.isEmpty()) {
text += i18n("%1: %2", ui->blaLabel, ui->bla.text());
}
QGuiApplication::clipboard()->setText(text);
  
  What you would gain is that the code - albeit maybe a bit verbose - is 
simple, easy to understand and extend, and all the copy-to-clipboard code is in 
one place. And it reuses what's already in the labels. The i18n() thing may 
still be problematic, but given the dialog itself already uses two different 
labels for the text, this mostly should be fine.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Henrik Fehlauer]

rkflx added a comment.


  In D7087#207286 , @dhaumann wrote:
  
  > In fact, I wonder whether there already is a script or similar helper tools 
that give you this kind of system information. I currently do not remember, 
though... Anyone else?
  
  
  Do you mean `inxi -Fxxxz`? Surely that's material for a different patch, but 
might be a nice idea to integrate. It's more hardware related, though, less 
about software versions.
  
  I guess for a useful https://bugs.kde.org report the only thing missing is 
the KDE Applications version and some info about the graphics card + driver.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Dominik Haumann]

dhaumann added a comment.


  I can see the reason for English text only. Then again, I think that @rkflx 
has a very valid argument as well. I do not know any other place in KDE that 
intentionally uses non-translatable text. That's why I agree with @rkflx to 
better translate this.
  
  In fact, I wonder whether there already is a script or similar helper tools 
that give you this kind of system information. I currently do not remember, 
though... Anyone else?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Henrik Fehlauer]

rkflx added a comment.


  And what if someone is asked in a forum or chat operating in a language 
different than English to provide those information? I'd say we should just go 
with the flow, i.e. what you see is what gets copied. It's already translated, 
just like what you can copy in Help > About Kate > Libraries, and everybody 
will understand "Qt" and recognize the version strings anyway.
  
  Therefore, my vote is on #2. Could you just get to the translation with 
`text()` dynamically?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: rkflx, dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi edited the summary of this revision.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added a comment.


  In D7087#207081 , @ngraham wrote:
  
  > Fine from my side. We still need to figure out the translation 
issue--either work to make it translatable, or accept that this output will be 
English-only. I'll let the other reviewers have their say on that matter.
  
  
  Ok.
  
  About the translation: for me the question was not whether or not we have to 
accept that the output is English-only; but rather if it is desirable to have 
bug reports where someone pastes non-English system info. I see three options 
here:
  
  1. Keep it English (which might be strange for non-English users) but will 
lead to pure English bug reports
  2. Make it translatable and if the order of the lines are the same for each 
language, someone who reads bug reports with that info pasted in, can figure 
out what each line means, even if it is not English
  3. Make two items: one is translated ("Copy Info") and the other one is not 
("Copy Info for Bug Reports")

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Fine from my side. We still need to figure out the translation issue--either 
work to make it translatable, or accept that this output will be English-only. 
I'll let the other reviewers have their say on that matter.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added a comment.


  Is there anything else I can do for this patch?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi updated this revision to Diff 26765.
gregormi added a comment.


  Rebase to master. Tested again successfully. Ready from my side.

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=26764=26765

BRANCH
  arcpatch-D7087

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi marked 2 inline comments as done.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi marked an inline comment as done.
gregormi added inline comments.

INLINE COMMENTS

> gregormi wrote in Module.cpp:29
> This was a suggestion from the discussion on ReviewBoard. Is there an obvious 
> way to tell arc to do this in a separate commit? Otherwise I'll try to 
> separate this somehow manually.

I just saw that this change was already done with 
https://phabricator.kde.org/R102:8193488408011c4971711b81b782bb003ffd1c3d

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi marked an inline comment as done.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi updated this revision to Diff 26764.
gregormi added a comment.


  - remove unrelated change

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7087?vs=26493=26764

BRANCH
  arcpatch-D7087

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

AFFECTED FILES
  Modules/about-distro/src/Module.cpp
  Modules/about-distro/src/Module.h
  Modules/about-distro/src/Module.ui

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added a comment.


  In https://phabricator.kde.org/D7087#201065, @gregormi wrote:
  
  > In https://phabricator.kde.org/D7087#200541, @gregormi wrote:
  >
  > > - Add tooltip. Add keyboard shortcut Ctrl+C in the .ui file but it does 
not work (I will remove it if that's not easily fixable)
  >
  >
  > @dhaumann: any idea why the shortcut does not work here? Maybe a general 
thing how Plasma handles keyboard shortcuts?
  
  
  I also asked Kai who has no quick idea. So, should I remove the corresponding 
code and add a comment to it?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added a comment.


  In https://phabricator.kde.org/D7087#200541, @gregormi wrote:
  
  > - Add tooltip. Add keyboard shortcut Ctrl+C in the .ui file but it does not 
work (I will remove it if that's not easily fixable)
  
  
  @dhaumann: any idea why the shortcut does not work here? Maybe a general 
thing how Plasma handles keyboard shortcuts?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added inline comments.

INLINE COMMENTS

> ngraham wrote in Module.cpp:29
> Is this necessary for this patch? If not, let's address it in a different 
> patch or commit.

This was a suggestion from the discussion on ReviewBoard. Is there an obvious 
way to tell arc to do this in a separate commit? Otherwise I'll try to separate 
this somehow manually.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi added a comment.


  In https://phabricator.kde.org/D7087#201047, @dhaumann wrote:
  
  > Translation is not yet working correctly, see above comment.
  
  
  In the comment of the function copyToClipboard(); I wrote the following: 
"Copies the software and hardware information to clipboard. The label language 
is currently English only because the Plasma development language is English."
  
  So, my idea was not to translate the string that goes to the clipboard. 
Should it?

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Dominik Haumann]

dhaumann requested changes to this revision.
dhaumann added a comment.
This revision now requires changes to proceed.


  Translation is not yet working correctly, see above comment.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham, dhaumann
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Dominik Haumann]

dhaumann added a comment.


  All the keys in collectedData are not properly translated. As such, creating 
a final string out of this is not translation friendly.
  
  Given you seem to know exactly what data is going to be exported, you could 
also create a small helper class that gets filled.
  This way, you'd have all data at hand when you want to copy, and you have 
100% control about how you create your strings, e.g.:
  i18n("Processors: %1", processorCount);
  
  If you use i18n("Processors") + ":" + QString::number(processorCount), then 
this cannot be properly translated into all languages.
  
  Rule of thumb: One entire sentence must ALWAYS be in exactly ONE and the SAME 
i18n() call. Otherwise, translation is pretty much ALWAYS broken :-)

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham
Cc: dhaumann, ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  The UI is much better now! Looks great and works fine. If you can't easily 
figure out how to make ctrl+c work, that's fine for now, and we can investigate 
that in a later patch.

INLINE COMMENTS

> Module.cpp:29
>  #include 
> -#if KCOREADDONS_VERSION >= QT_VERSION_CHECK(5,20,0)
>  #include 

Is this necessary for this patch? If not, let's address it in a different patch 
or commit.

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham
Cc: ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

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

[gregormi]

gregormi retitled this revision from "WIP: Add menu with "Copy to Clipboard" to 
the About System module" to "Add "Copy Info" button to the About System KCM".

REPOSITORY
  R102 KInfoCenter

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

To: gregormi, ngraham
Cc: ltoscano, sebas, elvisangelaccio, cfeck, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart