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

2018-09-02 Thread 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

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

2018-09-02 Thread 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

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

2018-08-14 Thread 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

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

2018-08-14 Thread 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 >

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

2018-08-08 Thread 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

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

2018-08-03 Thread 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".

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

2018-07-15 Thread 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

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

2018-07-15 Thread 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

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

2018-07-15 Thread 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

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

2018-07-15 Thread 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

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

2018-07-15 Thread 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

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

2018-06-30 Thread 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

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

2018-06-29 Thread 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 >

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

2018-06-29 Thread 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

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

2018-06-29 Thread 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);

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

2018-06-29 Thread 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

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

2018-06-02 Thread 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

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

2018-05-30 Thread 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

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

2018-05-30 Thread 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

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

2018-05-30 Thread 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

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

2018-05-30 Thread 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

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

2018-05-30 Thread 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

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

2018-05-30 Thread 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

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

2018-05-30 Thread 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

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

2018-05-28 Thread 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

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

2018-05-27 Thread 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

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

2018-05-27 Thread 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

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

2018-05-26 Thread 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

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

2018-05-23 Thread 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

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

2018-05-23 Thread 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,

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

2018-05-14 Thread 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

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

2018-03-07 Thread 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:

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

2018-03-07 Thread 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

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

2018-03-06 Thread 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?

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

2018-02-15 Thread 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

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

2018-02-15 Thread 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

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

2018-02-15 Thread 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

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

2018-02-15 Thread 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,

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

2018-02-15 Thread 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 >

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

2018-02-15 Thread 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,

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

2018-02-15 Thread 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

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

2018-02-15 Thread 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

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

2018-02-15 Thread 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,

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

2018-02-08 Thread 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

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

2018-02-08 Thread 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,

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

2018-02-08 Thread 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

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

2018-02-08 Thread 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,

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

2018-02-08 Thread 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

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

2018-02-08 Thread 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

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

2018-02-04 Thread 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?

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

2018-02-04 Thread 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

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

2018-02-04 Thread 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

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

2018-02-04 Thread 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,

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

2018-02-04 Thread 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.

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

2018-02-04 Thread 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 >=

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

2018-02-04 Thread 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,