D14449: Modify device usage information

[Henrik Fehlauer]

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


  Thanks, looking better than before now. There are still some improvements you could make:
  
  - There is a superfluous space before the comma in the second line.
  - I'd prefer the bar to be a bit wider by default. However, it turns out there is a problem with my original suggestion (see inline comment).
  - The vertical spacing between the first and the second line is too big, it should be the same as for Size:. However, there should still be enough spacing so it also looks good with the Oxygen style. As far as I can see this is an issue with how Breeze renders the `KCapacityBar`, in particular the bounding rect contains unnecessary margins ([⇧] + [Ctrl]-click on it in GammaRay and compare Breeze and Oxygen). Of course that's material for a patch in a different repo, but the "hole" in your current screenshot does not look good (the second line is closer to the bottom than to the first line, which is bad!), and it would be better to fix the problem there before landing the KIO patch.

INLINE COMMENTS

> kpropertiesdialog.cpp:1184-1186
> +        QStorageInfo *storage = new QStorageInfo(QLatin1String("/"));
> +        const quint64 size = storage->bytesTotal();
> +        const quint64 free = storage->bytesAvailable();

That's a bit odd: Why would you have to query that information here again, if in the original implementation it is already available?

Perhaps all calls to `setText` should take place only in one part of the code, e.g. `slotFreeSpaceResult`.

> kpropertiesdialog.cpp:1188
> +        l = new QLabel(d->m_frame);
> +        grid->addWidget(l, curRow, 2);
> +        l->setText(i18nc("Device usage information","%1 used , %2 free", KIO::convertSize(size - free), KIO::convertSize(free)));

Do you need `curRow++` here, in case additional rows are added later on?

> kpropertiesdialog.cpp:1189
> +        grid->addWidget(l, curRow, 2);
> +        l->setText(i18nc("Device usage information","%1 used , %2 free", KIO::convertSize(size - free), KIO::convertSize(free)));
> +

Translators will only see the string and the context, so "Device usage information" is not enough. It would be better to also describe what the parameters are for, see `slotFreeSpaceResult` for an example.

> kpropertiesdialog.cpp:1279
>  
> +        d->m_capacityBar->setMaximumWidth(175);
>          d->m_capacityBar->setText(

I don't think we can set a maximum width for the complete bar, because it can conflict with languages with longer translations.

More importantly, for the Oxygen widget style the text in a `KCapacityBar` might be drawn inline and the bar should span the complete width of the dialog, so I don't think we should fiddle too much with the sizes of `m_capacityBar` or its sub-components.

I guess we have to live with the longer bar for Breeze, which could already become quite long without your patch if you resized the dialog. For other languages it is also less of an issue.

> kpropertiesdialog.cpp:1281
>          d->m_capacityBar->setText(
> -        i18nc("Available space out of total partition size (percent used)", "%1 free of %2 (%3% used)",
> -              KIO::convertSize(available),
> -              KIO::convertSize(size),
> -              percentUsed));
> +        i18nc("Available space out of total partition size (percent used)", "%1% used of %2",
> +              percentUsed,

Please also update the translation context when you change the string to translate.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180805/5a83b8a9/attachment-0001.html>