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 <https://phabricator.kde.org/F5885212>
  
  (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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180602/20a93025/attachment-0001.html>