D14449: Modify device usage information

[Pino Toscano]

pino added a comment.


  In D14449#335788 , @shubham wrote:
  
  > won't the pie chart representation look good, as we have in windows?
  
  
  That would be way too much wasted space for eye candy.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: ngraham, pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added a comment.


  A pie chart could work. But is there something wrong with the current 
representation?

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: ngraham, pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, bruns

D14449: Modify device usage information

[Shubham]

shubham added a subscriber: ngraham.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: ngraham, pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  won't the pie chart representation look good, as we have in windows?

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  You plz proceed on it. Right now working on some other patch.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added a comment.


  Great! Are you gonna do the QFormLayout patch, or should I?

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  In D14449#315524 , @ngraham wrote:
  
  > Do you plan to submit a new version of this after the layout has been 
converted to use `QFormLayout`? I do hope some form of this patch makes it in, 
since IMHO it's a nice little quality-of-life improvement.
  
  
  Sure

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added a comment.


  Do you plan to submit a new version of this after the layout has been 
converted to use `QFormLayout`? I do hope some form of this patch makes it in, 
since IMHO it's a nice little quality-of-life improvement.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham abandoned this revision.
shubham added a comment.


  Actually should be implemented using form layout

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added a task: T9297: Polish file/folder properties dialog.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> 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)));
> +

Also, style for i18n strings: there must be no space before comma.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#303766 , @shubham wrote:
  
  > In D14449#300327 , @ngraham 
wrote:
  >
  > > +1 for using the same label but putting the information on more than one 
line like @rkflx suggests.
  >
  
  
  As far as I can see that comment was part of the discussion about what to 
show, which I think we now concluded. I don't think that was meant as an advice 
regarding implementation.
  
  You are already creating a new label (which is fine, also where you're doing 
it is the perfect spot). I was only wondering whether the call to `setText` 
should better be placed elsewhere.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  In D14449#300327 , @ngraham wrote:
  
  > +1 for using the same label but putting the information on more than one 
line like @rkflx suggests.
  
  
  sorry for causing trouble : (

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  > This comment has been deleted.
  
  You are doing this in every other Diff, which is a bit annoying. Please first 
check what you wrote, then click on Send. Otherwise people will just ignore 
notifications.
  
  > is there any other simpler way to achieve this?
  
  Maybe, maybe not. I added my idea on that topic above, but it's not my job to 
research that in depth.

INLINE COMMENTS

> shubham wrote in kpropertiesdialog.cpp:1184-1186
> If all function calls to setText are to take place inside 
> slotFreeSpaceResult, then we will need to make a new QLabel, so we can't 
> re-use it(which is the main point as @ngraham had earlier said)

Could you point out where @ngraham said something like that? I cannot find it.

> shubham wrote in kpropertiesdialog.cpp:1279
> so we need to remove setMaximumWidth() ?

Yes.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  In D14449#303710 , @rkflx wrote:
  
  > - The vertical spacing between the first and the second line is too big, it 
should be the same as for Size:.
  
  
  is there any other simpler way to achieve this?

INLINE COMMENTS

> rkflx wrote in kpropertiesdialog.cpp:1184-1186
> 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`.

If all function calls to setText are to take place inside slotFreeSpaceResult, 
then we will need to make a new QLabel, so we can't re-use it(which is the main 
point as @ngraham had earlier said)

> rkflx wrote in kpropertiesdialog.cpp:1279
> 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.

so we need to remove setMaximumWidth() ?

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

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

D14449: Modify device usage information

[Shubham]

shubham edited reviewers, added: rkflx; removed: elvisangelaccio.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx, elvisangelaccio
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham updated this revision to Diff 39120.
shubham added a comment.


  F6176187: 2.png 
  @rkflx is this okay now?

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14449?vs=38784=39120

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  > Not if you set a maximum width.
  
  @rkflx  eeh, skipped from my mind
  btw , then it is doable

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#303660 , @shubham wrote:
  
  > > Let me plug my suggestion again:
  > > 
  > >   Device capacity: ==- 24% used of 94.4 Gib
  > >22.5 GiB used, 71.9 GiB free
  > >
  >
  > if this design is used , then the capacity bar would look elongated and 
stretched, which doesn't looks good.
  
  
  Not if you set a maximum width.
  
  > Since used space is not of that much importance, so we can just omit it 
like this
  >  Device capacity: ===> 24% full of 90GiB (70GiB free)
  
  That's more or less what we currently have without your patch, but if we want 
to close the bug as WONTFIX I'd prefer the original order.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  > Let me plug my suggestion again:
  > 
  >   Device capacity: ==- 24% used of 94.4 Gib
  >22.5 GiB used, 71.9 GiB free
  >
  
  if this design is used , then the capacity bar would look elongated and 
stretched, which doesn't looks good. 
  Since used space is not of that much importance, so we can just omit it like 
this
  Device capacity: ===> 24% full of 90GiB (70GiB free)

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added a comment.


  In D14449#300868 , @rkflx wrote:
  
  > Let me plug my suggestion again:
  >
  >   Device capacity: ==- 24% used of 94.4 Gib
  >22.5 GiB used, 71.9 GiB free
  >   
  >
  > @ngraham Any opinions on that?
  
  
  I'm fine with that. A simple way to make sure the alignment is correct is to 
put all the strings that should be on a second line in their own cell in the 
`gridView`.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Dominik Haumann]

dhaumann added a comment.


  @rkflx Would be fine with this, as long as the alignment is correct (i.e. the 
text "Device capacity: ==- 24% used of 94.4 Gib" should all be one one 
line, and the text " 22.5 GiB used, 71.9 GiB free" should be 
below. The screenshot above is an example of wrong alignment.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#300841 , @dhaumann wrote:
  
  >   Device capacity: 94.4 Gib
  >   Device usage:==-- 24% full (71.9 GiB free, 22.5 GiB used)
  >   
  >
  > Could you give this a try with screenshot?
  
  
  I'm afraid the second line will become too long for locales other than 
English, making the capacity bar a tiny dot. Try it in GammaRay ([⇧] + [Ctrl] 
click, then change the text, add some spacing in front which currently is 
missing).
  
  Let me plug my suggestion again:
  
Device capacity: ==- 24% used of 94.4 Gib
 22.5 GiB used, 71.9 GiB free
  
  @ngraham Any opinions on that?

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  no problem

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Dominik Haumann]

dhaumann added a comment.


  Correct, but "visual cleanness" goes over redundancy (imo). In fact, the 
information 100 GiB, 20 GiB used (20%), 80 GiB free is also highly redundant.
  So redundancy is not the argument here. Instead, I think we should follow the 
best visual design. And the progress bar + 2 lines text behind is clearly not 
visually the best solution :-)

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added a comment.


  In D14449#300842 , @shubham wrote:
  
  > dhaumann: but that deviec capacity label seems to be redundant, because 
capacity is simply  used + free
  
  
  Yeah, but you have to do mental math to calculate that. It doesn't make sense 
to make the user do math to find out how big their disk is.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham added a comment.


  In D14449#300841 , @dhaumann wrote:
  
  > I think it looks visually distracting that one progress bar line ends up in 
two lines of text - this should be avoided.
  >
  > To behonest, the suggestion by @ngraham makes most sense to me: Two lines 
with the distinction of "Device capacity" and "Device usage".
  >
  >   Device capacity: 94.4 Gib
  >   Device usage:==-- 24% full (71.9 GiB free, 22.5 GiB used)
  >   
  >
  > Could you give this a try with screenshot?
  
  
  dhaumann: but that deviec capacity label seems to be redundant, because 
capacity is simply  used + free

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Dominik Haumann]

dhaumann added a comment.


  I think it looks visually distracting that one progress bar line ends up in 
two lines of text - this should be avoided.
  
  To behonest, the suggestion by @ngraham makes most sense to me: Two lines 
with the distinction of "Device capacity" and "Device usage".
  
Device capacity: 94.4 Gib
Device usage:==-- 24% full (71.9 GiB free, 22.5 GiB used)
  
  Could you give this a try with screenshot?

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham updated this revision to Diff 38784.
shubham added a comment.


  Remove  unintentional " "(that was silly)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14449?vs=38740=38784

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1270
>  const int percentUsed = qRound(100.0 * qreal(used) / qreal(size));
> -
> +
>  d->m_capacityBar->setText(

Unnecessary/unintentional whitespace change.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Shubham]

shubham updated this revision to Diff 38740.
shubham retitled this revision from "Display used space in GiB also" to "Modify 
device usage information".
shubham added a comment.


  F6162515: Screenshot_20180730_113005.png 
 Looks neat and tidy

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14449?vs=38690=38740

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns