D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
ngraham created this revision. ngraham added reviewers: Plasma, mart. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. ngraham requested review of this revision. REVISION SUMMARY SimpleKCM currently imposes a maximum height of `Kirigami.Units.gridUnit * 20`, which is arbitrary and results in many KCMs opened standalone in kcmshell still getting a scrollbar even when there's plenty of vertical space available. This patch changes the maximum value 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. CCBUG: 398820 TEST PLAN No actual changes can be observed because `flickable.contentHeight` always evaluates to 0 due to https://bugs.kde.org/show_bug.cgi?id=398820 and I haven't yet figured out the fix for that issue. But once that's fixed, SimpleKCM KCMs will have more sensible default heights when opened in kcmshell. REPOSITORY R296 KDeclarative BRANCH simplekcm-better-maximum-height (branched from master) REVISION DETAIL https://phabricator.kde.org/D20283 AFFECTED FILES src/qmlcontrols/kcmcontrols/qml/SimpleKCM.qml To: ngraham, #plasma, mart Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
ngraham added a comment. And @mart if you have some time, I could use a hand figuring out why `flickable.contentHeight` is always evaluating to 0 here. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D20283 To: ngraham, #plasma, mart Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
GB_2 added a comment. Ping REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D20283 To: ngraham, #plasma, mart Cc: GB_2, kde-frameworks-devel, michaelh, ngraham, bruns
D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
davidedmundson added a comment. > And @mart if you have some time, I could use a hand figuring out why flickable.contentHeight is always evaluating to 0 here. so right now this patch does nothing? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D20283 To: ngraham, #plasma, mart Cc: davidedmundson, GB_2, kde-frameworks-devel, michaelh, ngraham, bruns
D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
broulik requested changes to this revision. broulik added a comment. This revision now requires changes to proceed. 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. :) REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D20283 To: ngraham, #plasma, mart, broulik Cc: broulik, davidedmundson, GB_2, kde-frameworks-devel, michaelh, ngraham, bruns
D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
ngraham added a comment. Good idea, but the problem I keep running into is that `flickable.contentHeight` is set to 0 here. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D20283 To: ngraham, #plasma, mart, broulik Cc: broulik, davidedmundson, GB_2, kde-frameworks-devel, michaelh, ngraham, bruns
D20283: [SimpleKCM] Base the maximum height on the screen height, not some random value
ngraham updated this revision to Diff 56461. ngraham added a comment. Take headers and padding into account for height calculation REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20283?vs=55501&id=56461 BRANCH simplekcm-better-maximum-height (branched from master) REVISION DETAIL https://phabricator.kde.org/D20283 AFFECTED FILES src/qmlcontrols/kcmcontrols/qml/SimpleKCM.qml src/qmlcontrols/kquickcontrols/private/keysequencehelper.cpp To: ngraham, #plasma, mart, broulik Cc: broulik, davidedmundson, GB_2, kde-frameworks-devel, michaelh, ngraham, bruns
