Differential D20283

[RFC] [SimpleKCM] Fix height calculation
AbandonedPublic
Actions

Authored by ngraham on Apr 5 2019, 9:13 PM.

Details

Reviewers
mart
broulik
Group Reviewers
Plasma
Summary

SimpleKCM currently calculates the height of its page incorrectly in a few ways:

  • flickable.contentHeight, which determines the base height, always gets set to 0
  • Paddings and headers aren't taken into account
  • There's an arbitrary maximum height of Kirigami.Units.gridUnit * 20

This patch takes paddings and headers into account, and changes the maximum height
so that it's based on a fraction of the available screen height, ensuring that tall KCMs
will fill the available space as often as possible.

RFC because flickable.contentHeight always evaluates to 0 and I can't figure out why.

BUG: 398820
FIXED-IN: 5.58

Diff Detail

Repository
R296 KDeclarative
Lint
Lint Skipped
Unit
Unit Tests Skipped
ngraham created this revision.Apr 5 2019, 9:13 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 5 2019, 9:13 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Apr 5 2019, 9:13 PM
ngraham added a comment.Apr 5 2019, 9:14 PM

And @mart if you have some time, I could use a hand figuring out why flickable.contentHeight is always evaluating to 0 here.

davidedmundson added a subscriber: davidedmundson.Apr 16 2019, 1:02 AM
This comment was removed by davidedmundson.
broulik requested changes to this revision.Apr 16 2019, 1:10 PM
broulik added a subscriber: broulik.

Instead of swapping out one random number for another, please correctly calculate the implicit size of the page, like GridViewKCM does:
Something like

implicitHeight: Math.min(flickable.contentHeight, Kirigami.Units.gridUnit * 20)
                    + topPadding + bottomPadding
                    + (header && header.visible ? header.height + header.topPadding + header.bottomPadding : 0)
                    + (footer && footer.visible ? footer.height + footer.topPadding + footer.bottomPadding : 0) + Kirigami.Units.gridUnit

plus the page's own padding or so. :)

This revision now requires changes to proceed.Apr 16 2019, 1:10 PM
ngraham added a comment.Apr 16 2019, 2:07 PM

Good idea, but the problem I keep running into is that flickable.contentHeight is set to 0 here.

ngraham updated this revision to Diff 56461.Apr 17 2019, 5:43 PM

Take headers and padding into account for height calculation

ngraham updated this revision to Diff 56462.Apr 17 2019, 5:47 PM
ngraham retitled this revision from [SimpleKCM] Base the maximum height on the screen height, not some random value to [RFC] [SimpleKCM] Fix height calculation.
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

Remove accidentally added change

mart added inline comments.Apr 17 2019, 5:51 PM
src/qmlcontrols/kcmcontrols/qml/SimpleKCM.qml
55

i don't think adding header.padding is correct here, also because in most cases it won't have sich a property.
also, header.implicitheight should be used, as size propagation from child to parent is always with implicitHeight (while height is from parent to child) otherwise gets binding loops

ngraham updated this revision to Diff 56465.Apr 17 2019, 5:55 PM
ngraham marked an inline comment as done.
ngraham edited the summary of this revision. (Show Details)

Use implicitHeights for headers and footers

ngraham updated this revision to Diff 56466.Apr 17 2019, 5:56 PM

Remove unintentional change

ngraham updated this revision to Diff 56467.Apr 17 2019, 5:57 PM

arc, you are driving me crazy

mart added a comment.Apr 18 2019, 10:45 AM

so, now on master Kirigami should have correct contentHeight and implicitHeight/implicitWidth for Page andScrollablePage. this means that this patch *should* be not necessary anymore (only thing that should happen, is ro remove completely implicitWidth/implicitheight calculation from here)

*however* things are still broken, as the correct hint is not computed immediately, so it seems the kcmshell page doesn't resize correctly still.
for this i guess the intervention should be either in kcmshell or kcmModuleQml in kcmutils module

ngraham abandoned this revision.Apr 18 2019, 3:00 PM

@mart did this in the correct way instead!

Revision Contents
Changeset List

Diff 56467

View Options

src/qmlcontrols/kcmcontrols/qml/SimpleKCM.qml

Loading...