[Notifications] Never manually hide() the NotificationPopup
ClosedPublic

Authored by broulik on Feb 16 2017, 2:44 PM.

Details

Summary

Otherwise the positioner will get completely confused.
When a notification is closed, the popup will already be closed in response to sourceRemoved.
Also, when triggering an action, call closePopup instead of hiding the popup.

(cherry picked from commit b97fdfa293dd4de59046c19f08aa6a3b940a790c)

Test Plan

This is a backport to Plasma/5.8 branch of a patch I made for 5.9 originally and I have never seen notifications being positioned in the wrong way (too far from the panel) ever since. Now that 5.9 is out for a while and we haven't had any complains as far as I can tell, I consider this patch to be safe to 5.8.

Without the patch, clicking an action in a notification usually proved enough to get the positioner confused.

(The only difference in this patch to the 5.9 one is that the onOpenUrl part is missing since that is a 5.9 addition)

  • Clicking actions on notification still works
  • Closing a notification (both X and clicking the popup, as we had in 5.8) works
  • Configure button also works

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik updated this revision to Diff 11407.Feb 16 2017, 2:44 PM
broulik retitled this revision from to [Notifications] Never manually hide() the NotificationPopup.
broulik updated this object.
broulik edited the test plan for this revision. (Show Details)
broulik added a reviewer: Plasma.
broulik set the repository for this revision to R120 Plasma Workspace.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 16 2017, 2:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik edited the test plan for this revision. (Show Details)Feb 16 2017, 2:44 PM
ivan accepted this revision.Feb 16 2017, 5:06 PM
ivan added a reviewer: ivan.
ivan added a subscriber: ivan.

Seems ok.

Would be nice to know the reasoning on why the positioning was bad without it - why it was "confused" in the first place.

This revision is now accepted and ready to land.Feb 16 2017, 5:06 PM

why it was "confused" in the first place.

If you just did a hide() it would never put the dialog back into the "available popups" pool, always reserving the space on screen as occupied by a popup. The C++ part already does some visibleChanged magic but only for when *showing* the dialog, it's quite a mess, but I didn't dare touching it. (Not in stable branch, anyway)

mck182 added a subscriber: mck182.Feb 16 2017, 8:22 PM

it's quite a mess

No it's not :P It's a delicate complex code with lots of comments. But let me know if you need some help with that.

ivan added a comment.Feb 17 2017, 6:08 AM

If you just did a hide() it would never put the dialog back into the "available popups" pool, always
reserving the space on screen as occupied by a popup

Heh, cool :)

As far as I'm concerned, you still have a green light to push.

This revision was automatically updated to reflect the committed changes.