Differential D15855

Port Kickoff to shared view component using single-MouseArea pattern
ClosedPublic
Actions

Authored by hein on Sep 30 2018, 1:16 PM.

Details

Reviewers
ngraham
davidedmundson
Group Reviewers
Plasma
Commits
R119:48b32259e73f: Port Kickoff to shared view component using single-MouseArea pattern
Summary

This:

  • Moves MouseArea out of the item delegate and uses a single MouseArea per view.
  • Ports all the pages to using a shared view component.

Aside from saving a lot of QObjects, using a single MouseArea also
has the advantage that MouseArea.positionChanged isn't fired when
QQuickWindow synthesizes a hover event per frame, unlike MouseArea
.containsMouse. This fixes a user-reported bug in the search page,
and the same pattern is used by various other UIs we have for
similar reasons.

BUG:397693

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3361
Build 3379: arc lint + arc unit
hein created this revision.Sep 30 2018, 1:16 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 30 2018, 1:16 PM
hein requested review of this revision.Sep 30 2018, 1:16 PM
hein updated this revision to Diff 42595.Sep 30 2018, 1:20 PM

Focus scope handling.

hein updated this revision to Diff 42596.Sep 30 2018, 1:23 PM

Update copyright years

ngraham requested changes to this revision.Sep 30 2018, 2:50 PM
ngraham added a subscriber: ngraham.

Now Kickoff doesn't open. When I click on it, I get a little error pop-up that says:

Error loading QML file: file:///usr/share/plasma/plasmoids/org.kde.plasma.kickoff/contents/ui/Kickoff.qml:45:34: Type FullRepresentation unavailable
file:///usr/share/plasma/plasmoids/org.kde.plasma.kickoff/contents/ui/FullRepresentation.qml:132:13: Type FavoritesView unavailable
file:///usr/share/plasma/plasmoids/org.kde.plasma.kickoff/contents/ui/FavoritesView.qml:125:5: KickoffListView is not a type
This revision now requires changes to proceed.Sep 30 2018, 2:50 PM
hein updated this revision to Diff 42841.Oct 4 2018, 11:16 AM

Add missing file, d'oh.

davidedmundson added a subscriber: davidedmundson.Oct 4 2018, 4:37 PM
davidedmundson added inline comments.
applets/kickoff/package/contents/ui/KickoffListView.qml
57 ↗(On Diff #42841)

This already is a focusScope

could this not be the root item?

hein added inline comments.Oct 6 2018, 12:42 AM
applets/kickoff/package/contents/ui/KickoffListView.qml
57 ↗(On Diff #42841)

I don't think so because I need the MouseArea to cover the ScrollArea, but the ScrollArea has to manage its single child (the ListView). I could do zany things like parent the MA to the ListView and z-order it above the contentItem but it's kinda brittle. Compared to having a MA in every single delegate, it's still a handy win on fewer objects.

davidedmundson accepted this revision.Oct 6 2018, 12:11 PM
This revision was not accepted when it landed; it landed in state Needs Review.Oct 24 2018, 8:01 AM
Closed by commit R119:48b32259e73f: Port Kickoff to shared view component using single-MouseArea pattern (authored by hein). · Explain Why
This revision was automatically updated to reflect the committed changes.
ngraham added a comment.Oct 24 2018, 7:03 PM

Oops, sorry, I forgot to change my status!

ngraham added a comment.Nov 19 2018, 5:08 AM

@hein This broke the scrollbar for any views that are scrollable. Now the scrollbars don't accept clicks and drags anymore, and don't change on hover. Maybe the MouseArea doesn't pass on the mouse events to the ScrollArea underneath it or something?

We can't ship 5.15 this way; please fix. Thanks!

hein added a comment.Nov 19 2018, 6:23 AM
In D15855#362015, @ngraham wrote:

@hein This broke the scrollbar for any views that are scrollable. Now the scrollbars don't accept clicks and drags anymore, and don't change on hover. Maybe the MouseArea doesn't pass on the mouse events to the ScrollArea underneath it or something?

We can't ship 5.15 this way; please fix. Thanks!

Plasma releases are time-based. Declaring release blockers is not a power you have. Using powers you don't actually have to "please fix, thanks" bugs in code under common ownership is a "you broke it, you fix it" style in which we don't work. Because it is demotivating for one, but also because it increases the likelihood of individual contributors becoming overly entrenched in areas of the code and pushing bus numbers down. In the future, please file a bug ticket and reference the commit that caused the regression.

I'll look into the bug.

ngraham added a comment.EditedNov 19 2018, 2:17 PM

With the benefit of a good night's sleep, I can see now that my comment was really passive-aggressive, for which I apologize. You're right, of course. Thanks for the quick fix!

Revision Contents
Changeset List

Diff 42596

View Options

applets/kickoff/package/contents/ui/ApplicationsView.qml

Loading...
View Options

applets/kickoff/package/contents/ui/BaseView.qml

Loading...
View Options

applets/kickoff/package/contents/ui/Breadcrumb.qml

Loading...
View Options

applets/kickoff/package/contents/ui/FavoritesView.qml

Loading...
View Options

applets/kickoff/package/contents/ui/KickoffItem.qml

Loading...
View Options

applets/kickoff/package/contents/ui/SearchView.qml

Loading...