Ensure that GlobalDrawer topContent always stays on top
AbandonedPublic

Authored by ngraham on Mon, Oct 28, 9:27 PM.

Details

Summary

Right now, if you define topContent for your GlobalDrawer, it's added to the scrollview
so it can actually scroll out of sight, making it not always on top.

This patch moves the topContent above the scrollview so it always stays on top.

Test Plan

Discover's GlobalDrawer toolbar no longer strangely scrolls out of view:

No regressions in Discover when the view is not scrollable

No regressions that I could detect in Kirigami Gallery, though this probably needs lots
more testing to ensure that nothing has exploded

BUG: 389533
FIXED-IN: 5.64

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Mon, Oct 28, 9:27 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptMon, Oct 28, 9:27 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Mon, Oct 28, 9:27 PM
ngraham edited the summary of this revision. (Show Details)Tue, Oct 29, 4:36 PM
ahiemstra added inline comments.Thu, Oct 31, 1:16 PM
src/controls/GlobalDrawer.qml
241–244

+1 for getting rid of these paddings.

252

Uhm, what's this value?

260

In my opinion, it is nicer to be explicit about types instead of relying on implicit conversion. So this should be (!root.collapsed || showTopContentWhenCollapsed) ? 1 : 0.

370

We really should fix scrollview's scrollbars...

ngraham updated this revision to Diff 69116.Thu, Oct 31, 3:14 PM
ngraham marked 3 inline comments as done.

Address review comments

ngraham marked an inline comment as done.Thu, Oct 31, 3:14 PM
ngraham added inline comments.
src/controls/GlobalDrawer.qml
370

See https://phabricator.kde.org/T9126.

I *really* dislike overlay scrollbars, both in terms of utility and also because of the horrible code hacks required to make them work properly.

Material for another patch though.

ahiemstra accepted this revision.Thu, Oct 31, 3:58 PM
This revision is now accepted and ready to land.Thu, Oct 31, 3:58 PM
ngraham added a comment.EditedThu, Oct 31, 8:35 PM

Got confirmation from @mart in chat that this is good to go. Landing it!

This revision was automatically updated to reflect the committed changes.
mart reopened this revision.Tue, Nov 5, 11:59 AM

I just reverted this, it causes all sort of breakages in the side drawers of many apps, mostly those that support the "collapsed sidebar" mode, like the emoji selector and ksysguardqml
Unfortunately, it has been pushed too near to framework release, so now it has been released, breaking several apps for a whole framework release.
big changes like that should go in only in like the first half of the month

This revision is now accepted and ready to land.Tue, Nov 5, 11:59 AM
mart requested changes to this revision.Tue, Nov 5, 11:59 AM
This revision now requires changes to proceed.Tue, Nov 5, 11:59 AM
davidedmundson added inline comments.
src/controls/GlobalDrawer.qml
258

General rule of thumb:

  • implicit size goes up from children to parent (including Layout.min/max/preferred)
  • current size goes downwards from parent to child

If you ever mix them you're asking for recursive loops.
I suspect it's the cause of the problem we're hitting.

wbauer added a subscriber: wbauer.Tue, Nov 5, 1:36 PM
In D25019#558919, @mart wrote:

Unfortunately, it has been pushed too near to framework release, so now it has been released, breaking several apps for a whole framework release.

It has only been tagged so far, it's not been released yet.
The release will be next Saturday, maybe you should ask for a respin before that.

ngraham added a subscriber: dfaure.Tue, Nov 5, 2:27 PM

Done, I've asked @dfaure for a re-spin to pick up the reverting commit.

Sorry about this. :/ I'll try to fix it and re-submit for review for 5.65.

ngraham planned changes to this revision.Tue, Nov 5, 9:40 PM
ngraham abandoned this revision.Wed, Nov 20, 9:48 PM

Abandoning in favor of D25426 and D25425.