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
