Slightly reduce vertical gutter width.
ClosedPublic

Authored by Zren on Dec 5 2017, 1:08 PM.

Details

Summary

Removed 1 smallSpacing from the top and bottom padding of each cell.

Allows more rows to fit on screen. Results in less goofy-looking
vertical gutters with the default of two text rows (as most labels
only have one, it's a lot of dead space on screen in the common
case).

Based on user feedback.

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein created this revision.Dec 5 2017, 1:08 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 5 2017, 1:08 PM
hein requested review of this revision.Dec 5 2017, 1:08 PM
Zren added a subscriber: Zren.EditedDec 5 2017, 7:02 PM

Before patch:

  • GridView.cellHeight was icon.height + text.height + 6 * smallSpacing
  • Loader {id: frameLoader } was icon.height + text.height + 5 * smallSpacing. It also has a y offset giving it a 1 * smallSpacing top margin.

After patch:

  • GridView.cellHeight is now icon.height + text.height + 3 * smallSpacing
  • Loader {id: frameLoader } is now icon.height + text.height + 4 * smallSpacing, which will overlap the icon below it by 1 * smallSpacing no? Even though it still has the 1 * smallSpacing top margin, I'm seeing some overlap.

It's more noticeable if you do var extraHeight = 0;

  • yellow = GridView.delegate outline
  • cyan = Loader { id: frameLoader } outline

Outline with:

Rectangle { border.color: "#0ff"; anchors.fill: parent; border.width: 1; color: "transparent"; }

Before the patch, we had:

  • 6 * smallSpacing + extraHeight cellHeight
    • 1 * smallSpacing top margin (frameLoader.y)
    • 2 * smallSpacing top padding above the icon (icon.anchors.topMargin)
    • 1 * smallSpacing spacing between icon and label (label.anchors.topMargin)
    • 2 * smallSpacing bottom padding (the remaining height of the frameLoader for the bottom padding inside the frame.
    • extraHeight bottom margin which gives each cell a bit of extra height so that we don't have a bunch of empty space at the bottom.
hein updated this revision to Diff 23791.Dec 12 2017, 8:04 AM

Fix overlap. Thanks for the analysis.

abetts added a subscriber: abetts.Dec 12 2017, 3:09 PM

Does the grid and spacing cover only the areas in yellow, or does it cover the entire screen?

hein added a comment.Dec 13 2017, 9:29 AM

Does the grid and spacing cover only the areas in yellow, or does it cover the entire screen?

This is about the vertical spacing between desktop icons basically - it covers the entire screen in the sense that desktop icons can be anywhere on screen.

Zren added a comment.Jan 2 2018, 11:07 PM

So you removed 2 units from the overall cell height, but only 1 unit from the frameloader height. This means you removed 1 unit from the "leftover" padding at the bottom of the frameloader, and are overlapping the cell below by another 1 unit.

Before (cellHeight=6 units, top margin=1 unit, frameloader=5 units)

After (cellHeight=4 units, top margin=1 unit, frameloader=4 units)

It's much more obvious when we zoom in.

How to fix:

Option A) Remove just 1 unit of padding from the bottom of the frameloader.

Add 1 unit to the cellHeight. (cellHeight=5 units, top margin=1 unit, frameloader=4 units)

This makes it look weird, since there's 2 units of top padding, and only 1 unit of bottom padding.

Option B) Remove 1 unit from the top padding, and 1 unit from the bottom padding of the frameloader.

Remove 1 more unit from icon.anchors.topMargin

From

PlasmaCore.IconItem {
    id: icon

    anchors {
        topMargin: (2 * units.smallSpacing)
        leftMargin: units.smallSpacing
    }

to

PlasmaCore.IconItem {
    id: icon

    anchors {
        topMargin: units.smallSpacing
        leftMargin: units.smallSpacing
    }

and change the frameLoader.height calculation

Loader {
    id: frameLoader

    height: {
        if (root.useListViewMode) {
            return parent.height;
        }

        // Note: frameLoader.y = units.smallSpacing (acts as top margin)
        return (units.smallSpacing // icon.anchors.topMargin (acts as top padding)
            + icon.height
            + units.smallSpacing // label.anchors.topMargin (acts as spacing between icon and label)
            + (label.lineCount * theme.mSize(theme.defaultFont).height)
            + units.smallSpacing); // leftover (acts as bottom padding)
    }

Zren added a comment.Jan 3 2018, 4:54 PM

I don't think I can attach a new diff to your differential, so here's a link to it:

https://phabricator.kde.org/differential/diff/24677/?revisionID=9201

Here's an example without the outlines. 1080p with 32px panel on bottom and IconSize=3 (1 smaller than default).

Here's an example with the default IconSize=4

ngraham added a subscriber: ngraham.Jan 3 2018, 5:03 PM
hein added a comment.Jan 4 2018, 10:00 PM

I think you have to use "Commandeer revision" and then you can upload a different diff here. Or maybe I should abandon and you refile?

Zren commandeered this revision.Jan 4 2018, 11:17 PM
Zren added a reviewer: hein.

Oh I see, "Commandeer revision" is in the "Add action..." dropdown, not the sidebar.

Zren updated this revision to Diff 24677.Jan 4 2018, 11:23 PM
Zren edited the summary of this revision. (Show Details)
hein accepted this revision.Jan 5 2018, 5:10 PM

Thanks Chris!

This revision is now accepted and ready to land.Jan 5 2018, 5:10 PM
This revision was automatically updated to reflect the committed changes.