Add "Copy Info" button to the About System KCM
ClosedPublic

Authored by gregormi on Aug 3 2017, 9:11 AM.

Details

Summary

Adds a new button with a "Copy to Clipboard" action to the "About" System module. The action copies the visible information to the clipboard. It can be used for quickly retrieving information for a bug report.

This is an example of how the copied text looks like:

Distro: openSUSE Tumbleweed 20180404
KDE Frameworks Version: 5.46.0
Qt Version: 5.10.0
Kernel Version: 4.15.13-2-default
OS Type: 64-bit
Processors: 4 × Intel® Core™ i3 CPU M 370 @ 2.40GHz
Memory: 7,7 GiB of RAM

(Note that in this example 'KDE Plasma Version' is missing here because it is hidden in the development version)

Screenshot:

BUG: 366266

Also note, that the layout in Modules/about-distro/src/Module.ui had to be reworked in order to properly integrate the new button (see screenshots below). As a nice side effect is that the behavior of horizontal resizing is now better than before. The content is now always centered instead of shifted to the right.

Diff Detail

Repository
R102 KInfoCenter
Branch
arcpatch-D7087
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
gregormi edited the summary of this revision. (Show Details)Feb 15 2018, 3:19 PM
rkflx added a subscriber: rkflx.EditedFeb 15 2018, 3:46 PM

And what if someone is asked in a forum or chat operating in a language different than English to provide this 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 HelpAbout KateLibraries, 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?

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?

rkflx added a comment.Feb 15 2018, 8:53 PM

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.

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.

One small suggestion to improve the clipboard code.

Modules/about-distro/src/Module.cpp
270 ↗(On Diff #17613)

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.

rkflx added a comment.Mar 6 2018, 8:38 PM

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
In D7087#207306, @rkflx 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 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.

Modules/about-distro/src/Module.cpp
270 ↗(On Diff #17613)

Good question :). I'll fix that.

rkflx added a comment.Mar 7 2018, 11:30 AM

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.

gregormi updated this revision to Diff 34111.May 14 2018, 8:30 AM
  • Apply comments by reviewers:
    • move declaration near to usage
    • Reuse label texts
  • Rename label to more speaking name
gregormi marked 2 inline comments as done.May 14 2018, 8:32 AM
gregormi edited the summary of this revision. (Show Details)May 14 2018, 8:35 AM

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

ngraham added inline comments.May 23 2018, 4:56 PM
Modules/about-distro/src/Module.cpp
271 ↗(On Diff #17613)

Is "Distro" not a translated string?

rkflx added a comment.May 26 2018, 9:20 AM

@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:

Modules/about-distro/src/Module.cpp
267 ↗(On Diff #17613)

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.

274 ↗(On Diff #17613)

@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?

Modules/about-distro/src/Module.h
69–70 ↗(On Diff #17613)

Is this comment still accurate?

Modules/about-distro/src/Module.ui
363–368 ↗(On Diff #17613)

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

377 ↗(On Diff #17613)

This looks odd.

379–381 ↗(On Diff #17613)

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.

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

Modules/about-distro/src/Module.cpp
274 ↗(On Diff #17613)

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?

rkflx added inline comments.May 27 2018, 5:30 PM
Modules/about-distro/src/Module.cpp
274 ↗(On Diff #17613)

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

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

ltoscano added inline comments.May 28 2018, 12:54 PM
Modules/about-distro/src/Module.cpp
274 ↗(On Diff #17613)

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.

gregormi marked an inline comment as done.May 30 2018, 6:00 AM
gregormi added inline comments.
Modules/about-distro/src/Module.cpp
267 ↗(On Diff #17613)

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.

274 ↗(On Diff #17613)

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".

gregormi updated this revision to Diff 35158.May 30 2018, 6:39 AM
gregormi marked 2 inline comments as done.
  • 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
gregormi marked 5 inline comments as done.May 30 2018, 6:40 AM
gregormi added inline comments.
Modules/about-distro/src/Module.cpp
267 ↗(On Diff #17613)

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

gregormi updated this revision to Diff 35163.May 30 2018, 7:11 AM
gregormi marked 3 inline comments as done.
  • Fix comment
  • Clean ui file
  • Shortcut handling
gregormi marked an inline comment as done.May 30 2018, 7:12 AM
gregormi added inline comments.
Modules/about-distro/src/Module.ui
363–368 ↗(On Diff #17613)

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

379–381 ↗(On Diff #17613)

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.

gregormi updated this revision to Diff 35165.May 30 2018, 7:28 AM
gregormi marked 4 inline comments as done.
  • Handle colon (:) problem for translating and fix one label bug
gregormi updated this revision to Diff 35167.May 30 2018, 8:05 AM
  • use auto, add comment
rkflx added a comment.Jun 2 2018, 6:55 PM

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:

(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.)

Modules/about-distro/src/Module.cpp
272 ↗(On Diff #17613)

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)

275 ↗(On Diff #17613)

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

280 ↗(On Diff #17613)

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, …
283 ↗(On Diff #17613)

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.

Modules/about-distro/src/Module.ui
363–368 ↗(On Diff #17613)

See reply in main comment.

379–381 ↗(On Diff #17613)

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

Thanks Henrik for the review. I fixed the layout and it looks now like this in Qt Designer:

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.

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, &QPushButton::clicked, this, &Module::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?

gregormi updated this revision to Diff 36893.Jun 29 2018, 3:53 PM
  • 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
gregormi marked 6 inline comments as done.Jun 29 2018, 3:57 PM
gregormi added inline comments.
Modules/about-distro/src/Module.cpp
272 ↗(On Diff #17613)

Good idea. DONE.

Modules/about-distro/src/Module.ui
379–381 ↗(On Diff #17613)

Ok. Then I leave it like that.

rkflx added a comment.Jun 30 2018, 9:50 PM

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.


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.

Modules/about-distro/src/Module.cpp
175 ↗(On Diff #17613)

You could add a const.

272 ↗(On Diff #17613)

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.

272 ↗(On Diff #17613)

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 updated this revision to Diff 37786.Jul 15 2018, 8:27 AM
gregormi marked 4 inline comments as done.
  • Module.ui: Fix glitch near KernelName
  • Avoid double connection of copyToClipboard
  • Hide dummy label
  • Better naming of loop variable and optimize loop code
gregormi added inline comments.Jul 15 2018, 8:28 AM
Modules/about-distro/src/Module.cpp
272 ↗(On Diff #17613)

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

272 ↗(On Diff #17613)

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

gregormi added a comment.EditedJul 15 2018, 8:55 AM

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.

You can also notice the difference when gradually increasing the width of the window. In the master version you can see that first space is added between the left border and the logo and only then space is added on the left and right border. With the change equal space is added on the left and right side also at the beginning of the resizing.

Before (system installed Info Center):

After (this change):

New layout with spacers that push the grid columns to their minimum widths:

For reference: the layout in master:

gregormi updated this revision to Diff 37787.Jul 15 2018, 8:58 AM
  • Module.ui: add horizontal spacers
rkflx accepted this revision.Jul 15 2018, 10:14 PM

@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.)

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.

gregormi edited the summary of this revision. (Show Details)Jul 16 2018, 6:39 PM
gregormi edited the summary of this revision. (Show Details)Jul 16 2018, 6:41 PM
rkflx added a comment.Aug 3 2018, 9:54 PM

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!

gregormi updated this revision to Diff 39321.Aug 8 2018, 5:51 PM

Rebase on master

gregormi marked 4 inline comments as done.Aug 8 2018, 5:53 PM
dhaumann accepted this revision.Aug 14 2018, 8:10 PM

Ok from my side, although it still *feels* a bit messy :-)

  • please consider using QVector before committing
  • possibly fix the "Distro" thing, if applicable
Modules/about-distro/src/Module.cpp
175 ↗(On Diff #17613)

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

Modules/about-distro/src/Module.h
72 ↗(On Diff #17613)

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

This revision is now accepted and ready to land.Aug 14 2018, 8:10 PM
ngraham added inline comments.Aug 14 2018, 8:15 PM
Modules/about-distro/src/Module.cpp
175 ↗(On Diff #17613)

"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.

rkflx resigned from this revision.Aug 24 2018, 10:49 PM
gregormi updated this revision to Diff 40853.Sep 2 2018, 12:31 PM
  • Use QVector instead of QList
  • Use the term "Operating System:" instead of "Distro"
This revision was automatically updated to reflect the committed changes.