[Desktop] Pad cellWidth/cellHeight with the extra space
ClosedPublic

Authored by Zren on Jun 12 2017, 2:45 AM.

Details

Summary

This is an initial attempt. There's a few edge cases I need to deal with, but I just want to be sure I'm on the right track first.
This turned into something way more complicated than I originally thought.

The right/bottom corners will have an empty area where icons should be touching.
https://bugs.kde.org/show_bug.cgi?id=380424

I drew a few rectangles in FolderItemDelegate to demonstrate.

With the patch applied:

This creates a real/floating point cellWidth/cellHeight if the column count isn't divisible. If we floor the extraWidth/extraHeight, we'll still have a margin in the below and to the right. If we ceiled it, it would probably cause a scrollbar. I honestly don't trust floating points to not cause a scrollbar anyways though, but we can't set a cellSize for individual cells, so we can't use integers to solve this.

Test Plan
  • Does not work with icons arranged in row if there's a panel on the bottom. The panel covers the last row.
  • Does not work with icons arranged in columns if there's a panel on the right. The panel covers the last column.
  • Works fine if the panel is on the top or left.
  • When arranged in columns, dropping an icon with the center past the edge of the screen causes a horizontal scrollbar, since it creates an empty column, but places the icon on the column before it. Yo fix it, you have to drop the icon on the new column, then drop on a regularly visible column (trigger a columnCount change?).
  • If changing icon size in the config to a larger size and it causes a horizontal scrollbar, the viewport is too thin. The scrollbar cannot reveal the further column on the right.

TODO:

  • Test other screen resolutions
  • Test if text labels are at a floating point coordinate, causing uglyness.
  • Test spring loaded folder views
  • Test folder view widgets

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
Zren created this revision.Jun 12 2017, 2:45 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 12 2017, 2:45 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Zren updated this revision to Diff 15368.Jun 12 2017, 2:50 AM
Zren edited the summary of this revision. (Show Details)

Cleaner logic in calcExtraSpacing().

About panels covering the contents, perhaps you want to take into account plasmoid.availableScreenRect but root already has a margin based on this, so maybe just use the ScrollView's size instead of viewport.

hein requested changes to this revision.Jun 12 2017, 12:12 PM

Looks pretty good to me. It needs some polish as marked, but we should test it in master soon.

containments/desktop/package/contents/ui/FolderView.qml
526

Coding style: missing termination semicolae (also elsewhere)

539

readonly (also elsewhere)

This revision now requires changes to proceed.Jun 12 2017, 12:12 PM
anthonyfieroni added inline comments.
containments/desktop/package/contents/ui/FolderView.qml
538

Why it used (6 * units.smallSpacing) it makes more space at bottom? It should be same as iconwidth calculation, about me. I see you not change it, but it should.

anthonyfieroni added inline comments.Jun 12 2017, 1:49 PM
containments/desktop/package/contents/ui/FolderView.qml
538

You can use padding (or right/top/bottom/left padding), we depend to Qt 5.6 now,

Zren added a comment.Jun 12 2017, 5:01 PM

About panels covering the contents, perhaps you want to take into account plasmoid.availableScreenRect but root already has a margin based on this, so maybe just use the ScrollView's size instead of viewport.

I think my faulty logic for using viewport was because ScrollView goes under the panel. See below where the the purple outline goes under the panel. I mistakenly thought scrollView.width was the container's width for some reason (it's not).

It seems there's an anchor.fill: parent chain from GridView => FolderView => FolderViewLayer => all the way to the root object in main.qml which is a FolderViewDropArea with

FolderViewDropArea {

    // preferredWidth(...) and preferredHeight(...) return -1 when (isContainment || !folderViewLayer.ready),
    // so I've just expanded them as -1 for us. Basically they fill it will expand as far as it can.
    width: -1
    Layout.minimumWidth: -1
    Layout.preferredWidth: 0
    Plasmoid.switchWidth: -1

    height: -1
    Layout.minimumHeight: -1
    Layout.preferredHeight: 0
    Plasmoid.switchHeight: -1

    anchors {
        leftMargin: (isContainment && plasmoid.availableScreenRect) ? plasmoid.availableScreenRect.x : 0
        topMargin: (isContainment && plasmoid.availableScreenRect) ? plasmoid.availableScreenRect.y : 0

        // Don't apply the right margin if the folderView is in column mode and not overflowing.
        // In this way, the last column remains droppable even if a small part of the icon is behind a panel.
        rightMargin: folderViewLayer.ready && (folderViewLayer.view.overflowing || folderViewLayer.view.flow == GridView.FlowLeftToRight
            || folderViewLayer.view.layoutDirection == Qt.RightToLeft)
            && (isContainment && plasmoid.availableScreenRect) && parent
            ? parent.width - (plasmoid.availableScreenRect.x + plasmoid.availableScreenRect.width) : 0

        // Same mechanism as the right margin but applied here to the bottom when the folderView is in row mode.
        bottomMargin: folderViewLayer.ready && (folderViewLayer.view.overflowing || folderViewLayer.view.flow == GridView.FlowTopToBottom)
            && (isContainment && plasmoid.availableScreenRect) && parent
            ? parent.height - (plasmoid.availableScreenRect.y + plasmoid.availableScreenRect.height) : 0
    }
}

The final state for "arranged in rows" with a 30px bottom panel is:

qml: rightMargin folderViewLayer.ready true
qml: rightMargin folderViewLayer.view.overflowing false
qml: rightMargin flow == GridView.FlowLeftToRight true
qml: rightMargin layoutDirection == Qt.RightToLeft false
qml: rightMargin isContainment true
qml: rightMargin plasmoid.availableScreenRect QRect(0, 0, 1920, 1050)
qml: rightMargin parent ContainmentInterface(0x1d82390)
qml: rightMargin parent.width 1920
qml: rightMargin plasmoid.availableScreenRect.x 0
qml: rightMargin plasmoid.availableScreenRect.width 1920
qml: rightMargin return 0
qml: bottomMargin folderViewLayer.ready true
qml: bottomMargin folderViewLayer.view.overflowing false
qml: bottomMargin flow == GridView.FlowTopToBottom false
qml: bottomMargin return 0

So it seems that it's hiccuping on (folderViewLayer.view.overflowing || folderViewLayer.view.flow == GridView.FlowTopToBottom).

For "arrange in columns" with a panel on the right (and bottom since I don't want to delete my main panel)

qml: bottomMargin folderViewLayer.ready true
qml: bottomMargin folderViewLayer.view.overflowing false
qml: bottomMargin flow == GridView.FlowTopToBottom true
qml: bottomMargin isContainment true
qml: bottomMargin plasmoid.availableScreenRect QRect(0, 0, 1838, 1050)
qml: bottomMargin parent ContainmentInterface(0x1d82390)
qml: bottomMargin parent.height 1080
qml: bottomMargin plasmoid.availableScreenRect.y 0
qml: bottomMargin plasmoid.availableScreenRect.height 1050
qml: bottomMargin return 30
qml: rightMargin folderViewLayer.ready true
qml: rightMargin folderViewLayer.view.overflowing false
qml: rightMargin flow == GridView.FlowLeftToRight false
qml: rightMargin layoutDirection == Qt.RightToLeft false
qml: rightMargin return 0

So it's definitely the flow check. Gonna test without that condition in a bit.

Zren added a comment.Jun 12 2017, 5:14 PM

So removing the following two conditionals fixes it. I'm not sure why they are there. Right now it only stops at the panels if we're already overflowing.
in rightMargin: (folderViewLayer.view.overflowing || folderViewLayer.view.flow == GridView.FlowLeftToRight || folderViewLayer.view.layoutDirection == Qt.RightToLeft)
in bottomMargin: (folderViewLayer.view.overflowing || folderViewLayer.view.flow == GridView.FlowTopToBottom)

Zren added a comment.Jun 12 2017, 5:20 PM

Oh now I remember why I used scrollView.viewport. We don't want to count the scrollbars as "extra" space. Notice in this screenshot it would perfectly fit another row, but the scrollbar made the area shorter.

Zren updated this revision to Diff 15393.Jun 12 2017, 6:10 PM
Zren edited edge metadata.

+readonly. +semicolons.
Remove check for if we're already overflowing, using rtl layout, or if it's "arrange in columns/rows", since the ScrollView shouldn't be covered by panels.

Zren marked 2 inline comments as done.Jun 12 2017, 6:11 PM
hein added a comment.Jun 12 2017, 7:14 PM
In D6188#115977, @Zren wrote:

Remove check for if we're already overflowing, using rtl layout, or if it's "arrange in columns/rows", since the ScrollView shouldn't be covered by panels.

You didn't change the comment above the code you modified. Did you test that this is no longer an issue? If not, the comment needs adjusting.

Zren added a comment.Jun 12 2017, 8:28 PM
In D6188#115982, @hein wrote:

You didn't change the comment above the code you modified. Did you test that this is no longer an issue? If not, the comment needs adjusting.

Ah, yeah, I should remove that comment. Beforehand, you were much more likely to create a horizontal scrollbar than the panel overlapping the icon by 1-2px.
Someone who's panel was just barely overlapping the icons before this patch will lose a row of icons though. It's unfortunate, but they already had their icons shift around in Plasma 5.10.0

containments/desktop/package/contents/ui/FolderView.qml
538

Fairly certain was smallSpacing * 2 + iconSize + smallSpacing * 2 + defaultFont.height * lines + smallSpacing * 2

Right now:

  • frame has y: root.useListViewMode ? 0 : units.smallSpacing
  • frame has height: (icon.height + (2 * units.smallSpacing) + (label.lineCount * theme.mSize(theme.defaultFont).height) + (3 * units.smallSpacing));
  • icon has topMargin: (2 * units.smallSpacing)
  • label has anchors.topMargin: units.smallSpacing

So it's actually:

smallSpacing | frame | smallSpacing*2 | icon | smallSpacing | text | smallSpacing*2
hein added a comment.Jun 12 2017, 8:32 PM

The comment is mostly about the fact that this means you can no longer drop icons there then. Keep in mind that when the view is in columnar mode, horizontal overflow is (currently) the expected and correct behavior - if the folder is fuller than the screen can fill, the view has to start scrolling horizontally then.

Zren added a comment.Jun 12 2017, 8:53 PM

What behaviour should the folder widget use:

  1. Old behaviour: https://streamable.com/5gk5p
  2. Current behaviour with the patch: https://streamable.com/qs4s4
  3. Combination of the two, when "arranged in rows", it should stretch to fill the width, but keep a fixed height so that it's easy to spot at a glance that there's more icons. Vice versa for "arranged in columns", where it'll fill the height, but have a fixed width.
In D6188#116002, @hein wrote:

The comment is mostly about the fact that this means you can no longer drop icons there then. Keep in mind that when the view is in columnar mode, horizontal overflow is (currently) the expected and correct behavior - if the folder is fuller than the screen can fill, the view has to start scrolling horizontally then.

A scrollbar will still appear when needed (too many icons). It's just harder to get the scrollbar accidentally by dropping in the bottom right when there isn't an overflow.
You can still create a scrollbar accidently like this: https://streamable.com/why77
Where you mouse is still hovering the FolderView, but the center of the dropped icon is hovering the next column.
You can also force it by selecting a group of icons (eg: 3x3) and dropping icons off the screen. You need to drop them twice. Once to trigger the "overflow" mode, and a second time to actually place them. I think it's a bug that you need to do it twice. I think this happened before this patch though (I'll test after supper).

hein added a comment.Jun 12 2017, 9:01 PM

The widget should keep using the old behavior, grid-fitting is pretty awkward there (and I can already hear "my icons don't line up across widgets" complaints). This is really a desktop-specific special case motivated by people trying to put icons into opposing screen corners (or on edges), and being surprised to find no grid column on both ends. The widget doesn't support free icon placement, so it's not needed there.

Zren updated this revision to Diff 15398.Jun 12 2017, 9:14 PM

We don't really need to wait for folderViewLayer.ready before setting the bottom/right anchors anymore.
Don't add extra spacing if we're not a containment (the desktop).
Remove comment that's no longer applicable.

Zren updated this revision to Diff 15400.Jun 12 2017, 9:17 PM

Semicolons... (I should install a linter)

hein accepted this revision.Jun 15 2017, 1:03 PM
This revision is now accepted and ready to land.Jun 15 2017, 1:03 PM
This revision was automatically updated to reflect the committed changes.
Zren added a comment.Jun 20 2017, 9:59 PM

https://cgit.kde.org/plasma-desktop.git/commit/?id=f49ca6ee277142539e0302f2848c79ff41d319cf

@hein Good catch with the && isRootView. It's just optimization since the extraSpace for the popup is 0 right? Now that I think on it, not sure why I made the variables into properties. Probably for logging onVarChanged when debugging it. Would this be more efficient?

cellWidth: {
    if (root.useListViewMode) {
        return gridView.width;
    } else {
        var iconWidth = iconSize + (2 * units.largeSpacing) + (2 * units.smallSpacing);
        if (root.isContainment) {
            var extraWidth = calcExtraSpacing(iconWidth, scrollArea.viewportWidth);
            return iconWidth + extraWidth;
        } else {
            return iconWidth;
        }
    }
}

or is it just better to keep those properties exposed for future use?

hein added a comment.Jun 21 2017, 11:25 AM

Yeah, it would be more efficient. Actually, could you audit your code a little more? I think the same issue I had with the busy-looping popups (I was working on D6247 and noticed my plasmashell using 30% CPU and stuttering a lot, and Kai mentioned the popups were continously growing) actually also happens on the desktop sometimes - I still periodically get very high CPU usage after mucking about with FV. It's possible you have some sort of binding loop in there? De-propifying might already help.

Zren added a comment.Jun 21 2017, 6:34 PM

So before my commit, it was only needing to do:

qml: cellHeight iconHeight 108
qml: cellWidth iconWidth 92
qml: cellHeight iconHeight 108

with no updates after the panel is added.

When the panel slides in, it seems to sometimes update at height=1979px, but it may skip that and just calculate at height=1050px (I have a 30px panel).

We could check if && scrollArea.viewportHeight > 0 however that's still calculating the extra space a few times before the panel is added.

https://gist.github.com/Zren/1d93dfb5e687e68732d470d26104bf6d

Ideally we'd only want to calculate cellWidth/cellHeight once like it was before. seeing as how viewportHeight/viewportWidth is recalculated.

We could also wait for the scrollArea component to complete loading.

PlasmaExtras.ScrollArea {
    id: scrollArea

    readonly property int viewportWidth: ready && viewport ? Math.ceil(viewport.width) : 0
    readonly property int viewportHeight: ready && viewport ? Math.ceil(viewport.height) : 0
    property bool ready: false
    Component.onCompleted: {
        ready = true
    }

which brings us to this before the panels are added.

qml: cellHeight iconHeight 108
qml: onCellHeightChanged 108
qml: cellWidth iconWidth 92
qml: onCellWidthChanged 92
qml: cellHeight iconHeight 108
qml: cellHeight iconHeight 108
qml: cellWidth iconWidth 92
qml: scrollArea.onCompleted
qml: cellWidth iconWidth 92
qml: cellHeight iconHeight 108
qml: cellHeight iconHeight 108
qml: cellWidth iconWidth 92
qml:     availableColumns 20.869565217391305  = containerSize / cellSize 1920 92
qml:     availableColumns 20  floored
qml:     allColumnSize 1840
qml:     extraSpace 80
qml:     extraSpacing 4
qml: cellWidth extraWidth 4
qml: onCellWidthChanged 96
qml: cellHeight iconHeight 108
qml:     availableColumns 10  = containerSize / cellSize 1080 108
qml:     availableColumns 10  floored
qml:     allColumnSize 1080
qml:     extraSpace 0
qml:     extraSpacing 0                                                                                                                                                                                                                                                        
qml: cellHeight extraHeight 0

That seems good enough to me.

After the panels are added... I'm not sure what else to do. There isn't really a way to tell plasmoid.screenGeomertry is animating. As for the binding loops, I'd need to rewrite D6201 to get rid of the binding loops on the label without breaking other stuff.