Scrollview - Don't fill the parent with the view
ClosedPublic

Authored by davidre on Jul 30 2019, 12:51 PM.

Details

Summary

If the style sets a padding this is not respected if we set anchors.fill: parent.

BUG: 407643

Test Plan

Before:


After:

Diff Detail

Repository
R296 KDeclarative
Branch
scrollview (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14569
Build 14587: arc lint + arc unit
davidre created this revision.Jul 30 2019, 12:51 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 30 2019, 12:51 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
davidre requested review of this revision.Jul 30 2019, 12:51 PM
davidre updated this revision to Diff 62791.Jul 30 2019, 12:52 PM

Commit message

davidre retitled this revision from Scrollview - Don't fill the parent with the view If the style sets a padding this is not respected if we set `anchors.fill: parent`. to Scrollview - Don't fill the parent with the view.Jul 30 2019, 12:55 PM
davidre edited the summary of this revision. (Show Details)
davidre edited the test plan for this revision. (Show Details)
davidre added a reviewer: mart.
davidre edited the test plan for this revision. (Show Details)

An alternative approach could be to set the respective anchor.*margin to view.parent.*padding.

filipf added a subscriber: filipf.Jul 30 2019, 3:41 PM

Also fixes the Virtual Desktops KCM fwiw

davidre edited the summary of this revision. (Show Details)Jul 30 2019, 6:29 PM

So the alternative approach would be something like this:

diff --git a/src/qmlcontrols/kcmcontrols/qml/ScrollView.qml b/src/qmlcontrols/kcmcontrols/qml/ScrollView.qml
index 8ef57a2..443f063 100644
--- a/src/qmlcontrols/kcmcontrols/qml/ScrollView.qml
+++ b/src/qmlcontrols/kcmcontrols/qml/ScrollView.qml
@@ -48,14 +48,28 @@ QtControls.ScrollView {
     onViewChanged: {
         view.parent = scroll;
         view.anchors.fill = view.parent;
+        setMargins();
     }
 
     activeFocusOnTab: false
     Kirigami.Theme.colorSet: Kirigami.Theme.View
     Kirigami.Theme.inherit: false
 
-    Component.onCompleted: scroll.background.visible = true;
+    Component.onCompleted: {
+        scroll.background.visible = true;
+        setMargins();
+    }
+
+    function setMargins() {
+        if (view.parent.padding) {
+            view.anchors.padding = view.parent.padding;
+        } else {
+            view.anchors.topMargin = view.parent.topPadding;
+            view.anchors.leftMargin = view.parent.leftPadding;
+            view.anchors.bottomMargin = view.parent.bottomPadding;
+            view.anchors.rightMargin = view.parent.rightPadding;
+        }
+    }
 
-    
     QtControls.ScrollBar.horizontal.visible: false
 }

I has the same result but maybe would be more correct?

ngraham accepted this revision.Aug 1 2019, 5:12 PM
ngraham added a subscriber: ngraham.

This looks like a sane and simpler implementation to me, but let's get @mart's opinion too.

This revision is now accepted and ready to land.Aug 1 2019, 5:12 PM
mart accepted this revision.Aug 1 2019, 5:18 PM
This revision was automatically updated to reflect the committed changes.