simplify positioning code
ClosedPublic

Authored by mart on Jun 13 2017, 4:37 PM.

Details

Summary

drop most of the positioning code, drop the animation of the
window position. position the popup in the signal handler of
visibleChanged as we are sure there the size is final and the
first expose event didn't arrive yet.
at that point we are sure the size is the final and correct one

the animation is supposed to be done by the morphingpopups effect
instead.

Test Plan

notification positions are always correct now, both on X11 and
Wayland.
Unfortunately on x11 the notification typ doesn't seem to pass,
so they are correctly animated only in wayland

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.Jun 13 2017, 4:37 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 13 2017, 4:37 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart updated this revision to Diff 15463.Jun 15 2017, 2:41 PM

just rely on visibleChanged, abouttoshow dropped

mart edited the summary of this revision. (Show Details)Jun 15 2017, 2:46 PM

Mostly seems sensible, mine doesn't animate as-is anyway. Just one question.

applets/notifications/plugin/notificationshelper.cpp
293

I find it odd that we use the contentItem height not the window height which includes the frame margins.

Especially given that whole other patch is about making sure the window is resized to the contentItem.

mart added inline comments.Jun 15 2017, 2:48 PM
applets/notifications/plugin/notificationshelper.cpp
293

didn't change that, giving a try to it, may simplify things further

mart updated this revision to Diff 15464.Jun 15 2017, 2:52 PM

use dialog size instead of its mainitem's

davidedmundson accepted this revision.Jun 15 2017, 2:53 PM
This revision is now accepted and ready to land.Jun 15 2017, 2:53 PM
mart updated this revision to Diff 15568.Jun 19 2017, 9:57 AM
  • don't depend from visibility
mart updated this revision to Diff 15578.Jun 19 2017, 12:51 PM
  • make sure Dialog::setHeight is used

disable height binding when notifications are repopulated

davidedmundson requested changes to this revision.Jun 19 2017, 1:12 PM
davidedmundson added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
33 ↗(On Diff #15578)

If in D6215 you've got a mainItem->setVisible() before the showEvent because that fixes what this is doing.

If that's the case, why do we still need these changes?

applets/notifications/package/contents/ui/NotificationPopup.qml
59

So instead of making this inhibitor and changing a bunch of code

just convert:

list.clear()
list.append()

into:

list = otherList

This revision now requires changes to proceed.Jun 19 2017, 1:12 PM
mart added inline comments.Jun 19 2017, 1:23 PM
applets/notifications/package/contents/ui/NotificationItem.qml
33 ↗(On Diff #15578)

not strictly need, but they should trigger less resizes anyways when the dialogs are shown i think, even if they happen before the window is actually visible

applets/notifications/package/contents/ui/NotificationPopup.qml
59

can't do that, as notificationItem.actions is a ListModel and notificationsProperties is a javascript array instead

apol added a subscriber: apol.Jun 21 2017, 10:11 AM

Can we maybe include a test? Then it would be much safer to do this kind of changes.

davidedmundson accepted this revision.Jun 21 2017, 11:15 AM
This revision is now accepted and ready to land.Jun 21 2017, 11:15 AM
This revision was automatically updated to reflect the committed changes.