[Places] Draw 2px rectangle disk capacity bar underneath mount points
Needs ReviewPublic

Authored by Zren on May 20 2019, 8:51 PM.

Details

Reviewers
None
Group Reviewers
Dolphin
VDG
Summary

There a few more examples (including kvantum's blur) at the 2m40 mark in this video:

Elementary (and other file managers) show the capacity bar in the sidebar as well: https://i.stack.imgur.com/4l7YS.png


  • This patch draws overtop the text that's goes below the baseline like gj_.
    • It's fairly difficult to add extra space below the text as the places view is a "details" view. That means the text is vertically centered in the widget's "row".
    • We can make the PlacesItemWidget 8px taller, adding 4px above and below so there's adequate room for the capacitybar, but that wastes 4px for each mount point. Add padding above/below:
      • We add a PlacesItemListWidgetInformant inheriting KStandardItemListWidgetInformant. We overload, PlacesItemListWidgetInformant::calculateItemSizeHints(...) so we can change the height of each PlacesItemWidget.
      • We then need to check if the current PlacesItemWidget is a mount point, and share that boolean with the KStandardItemListWidgetInformant. I'm not 100% sure how to do this.
    • Ideally we'd use the "compact" view, which displays the text/size/etc in a column beside the icon. However the compact view was not designed for a file to take up the entire width of the viewport. It's also designed to handle vertical overflow with a horizontal scrollbar.

Performance (TODO)

I originally wanted to draw these capacity bars in the main file view (like Windows's "This PC"), but that would probably slow down loading folders with millions of files. So while D10453 was a nice hack, it was dead in the water for merging upstream.

This current patch might affect the startup speed, as the places panel is visible on load. I need to determine if this code is blocking or if it's lazy loaded. We don't want to make the startup slower.

KIO's File Selector only shows the capacity bars on hover?


QT_SCALE_FACTOR=2 build/bin/dolphin

FEATURE: 315405
FIXED-IN: 19.08.0

Test Plan
  • Check if an unmounted drive does not shows a capacity bar at startup.
  • Check if an unmounted drive shows a capacity bar after mounting.

  • Confirm various color schemes look okay with the breeze widget style.
  • Confirm the kvantum widget style looks okay.
  • Confirm it looks okay with various screen scaling.
    • 2x HiDPI QT_SCALE_FACTOR=2 build/bin/dolphin
    • 1.5x QT_SCALE_FACTOR=1.5 build/bin/dolphin

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Zren created this revision.May 20 2019, 8:51 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMay 20 2019, 8:51 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
Zren requested review of this revision.May 20 2019, 8:51 PM
Zren edited the summary of this revision. (Show Details)May 20 2019, 8:57 PM
ngraham edited the summary of this revision. (Show Details)

+1 for restoring the feature, which is very useful! I've wanted this myself many times, so thanks for submitting this! However I'll admit I'm not super thrilled about having the capacity indicator bar always visible. I can't quite put my finger on why, but something about it feels a bit off. Maybe that's why the original only appeared on hover? But that's problematic in an era where we need to be designing for touch-friendliness. Maybe it could be a pie chart to the right of the icon but left of the text? Or maybe it's fine as-is and I'm just being a stick-in-the-mud.

Either way, adding VDG for comment.

ndavis added a subscriber: ndavis.May 20 2019, 11:28 PM

Can you use the NegativeText color from the colorscheme instead of a custom danger color?

abetts added a subscriber: abetts.May 20 2019, 11:31 PM

I love this idea visually. I would even say that if you enable this on the sidebar, that maybe it should not repeat itself on the bottom bar on the same window.

filipf added a subscriber: filipf.May 20 2019, 11:52 PM

Can you use the NegativeText color from the colorscheme instead of a custom danger color?

Although some color schemes have ... odd colors set for NegativeText (e.g pink in Wonton Soup), I agree that it would be better to have it follow the color scheme.

Will have a more detailed look at this, but right now I'd add we need padding above the the lines.

+1 for restoring the feature, which is very useful! I've wanted this myself many times, so thanks for submitting this! However I'll admit I'm not super thrilled about having the capacity indicator bar always visible. I can't quite put my finger on why, but something about it feels a bit off. Maybe that's why the original only appeared on hover? But that's problematic in an era where we need to be designing for touch-friendliness. Maybe it could be a pie chart to the right of the icon but left of the text? Or maybe it's fine as-is and I'm just being a stick-in-the-mud.

Either way, adding VDG for comment.

I agree that something about it seems off, but I think a horizontal bar is easier to read than a pie chart. A simple percentage with a background to make it stand out could work if we need something less wide than the bar.

Can you use the NegativeText color from the colorscheme instead of a custom danger color?

Although some color schemes have ... odd colors set for NegativeText (e.g pink in Wonton Soup), I agree that it would be better to have it follow the color scheme.

We shouldn't hardcode colors to work around problems with specific colorschemes anyway.

+1 for restoring the feature, which is very useful! I've wanted this myself many times, so thanks for submitting this! However I'll admit I'm not super thrilled about having the capacity indicator bar always visible. I can't quite put my finger on why, but something about it feels a bit off. Maybe that's why the original only appeared on hover? But that's problematic in an era where we need to be designing for touch-friendliness. Maybe it could be a pie chart to the right of the icon but left of the text? Or maybe it's fine as-is and I'm just being a stick-in-the-mud.

Either way, adding VDG for comment.

I agree that there's something off about it - to me it is the fact that it's a lot of visual clutter that does not give actionable information. It relies on the user knowing how big the volumes are because for example a 90% filled bar is very different to someone with a 100 GiB volume than someone with a 1TiB volume. I don't have any data on this but I have a feeling most people don't care about how full all their volumes are until it hinders them so how I suggest we do it is for it to stay out of the user's way until the volumes are almost full, when there is a reason to present the information and possible actions the user can take.

Or at least vastly reduce how much attention it grabs because it could just be me but it distracts from reading the volume labels, I imagine it can get draining if you have to scan through a lot to find the one you want.

+1 for restoring the feature, which is very useful! I've wanted this myself many times, so thanks for submitting this! However I'll admit I'm not super thrilled about having the capacity indicator bar always visible. I can't quite put my finger on why, but something about it feels a bit off. Maybe that's why the original only appeared on hover? But that's problematic in an era where we need to be designing for touch-friendliness. Maybe it could be a pie chart to the right of the icon but left of the text? Or maybe it's fine as-is and I'm just being a stick-in-the-mud.

Either way, adding VDG for comment.

I agree that there's something off about it - to me it is the fact that it's a lot of visual clutter that does not give actionable information. It relies on the user knowing how big the volumes are because for example a 90% filled bar is very different to someone with a 100 GiB volume than someone with a 1TiB volume. I don't have any data on this but I have a feeling most people don't care about how full all their volumes are until it hinders them so how I suggest we do it is for it to stay out of the user's way until the volumes are almost full, when there is a reason to present the information and possible actions the user can take.

Or at least vastly reduce how much attention it grabs because it could just be me but it distracts from reading the volume labels, I imagine it can get draining if you have to scan through a lot to find the one you want.

Do you think this and other metrics could just be optional and enabled in settings, for example?

Do you think this and other metrics could just be optional and enabled in settings, for example?

Ehhhhhh, not sure we need another setting for this. IMO let's first try to find a visual appearance that's aesthetically pleasing enough that nobody wants to turn it off in the first place! :)

Zren added a comment.Wed, May 22, 1:56 AM

Can you use the NegativeText color from the colorscheme instead of a custom danger color?

NegativeText is a KDE ColorScheme color, and not part of QPalette. I haven't tried using it to access that QColor before.

DolphinView::updatePallette() uses KColorScheme, and plasma's theme.cpp provides an example.

Examples (Before on left / After using NegativeText on right):

Breeze:

Breeze Dark:

Wantan Soup:

Zren updated this revision to Diff 58452.Wed, May 22, 1:59 AM
Zren edited the summary of this revision. (Show Details)

Use KColorScheme's NegativeText color

I think what bugs me is the fact that the bar is below Root as well. If the only Places panel entries to get capacity bars were actual devices, I think it might feel better. And we're actually planning to remove the Root entry soon anyway: D15739. I just need to get around to fixing the test...

cfeck added a subscriber: cfeck.Wed, May 22, 2:22 AM

We disabled this feature because KDiskFreeSpaceInfo caused hangs for offline network mounts. Has this changed?

Zren added a comment.Wed, May 22, 4:39 AM

We disabled this feature because KDiskFreeSpaceInfo caused hangs for offline network mounts. Has this changed?

Do you know what specific part is slow?

Could we check KMountPoint::probablySlow() and skip those devices?

Ideally, isMountPoint, usedSpace. and totalSpace should be lazy loaded and not block.

I think what bugs me is the fact that the bar is below Root as well. If the only Places panel entries to get capacity bars were actual devices, I think it might feel better. And we're actually planning to remove the Root entry soon anyway: D15739. I just need to get around to fixing the test...

Ah, yeah. I had forgotten Root isn't at the top of the places list. The capacity bar would look weird if it was in the middle of the list. I'll see about only showing it in the devices group like the file picker.

Gentle ping :)