GNOME Bugzilla – Bug 695800
The context menu pops up when you right-click on a notification
Last modified: 2013-04-19 17:44:04 UTC
This is just a simple example of a bug that happens because of the basic confusion we have of the notification actor being inside the tray actor. This is a large change, but I've been testing it for a bit now, and I haven't found any regressions.
Created attachment 238814 [details] [review] messageTray: Remove some dead positioning code We don't modify the tray box's anchor position, so this shouldn't ever get called.
Created attachment 238815 [details] [review] messageTray: Move hover tracking to the notification widget This does nothing while the tray is active, so it doesn't make sense to track it on the tray. This also makes the code a lot easier to read, with notification behavior being labeled "notification" rather than "tray".
Created attachment 238816 [details] [review] messageTray: Move the notification actor out of the tray Putting the notification actor in the tray actor has caused a lot of various bugs and glitches over the years related to syncing the two, fizzling out events, and so on. It's a much simpler model if we consider the notification actor and tray to be separate widgets. As a side effect, this makes the context menu not pop up when we right-click on notifications.
Review of attachment 238814 [details] [review]: This is correct anyway.
IMHO, it is too late for this change in 3.8. We shipped 3.6 with many tray bugs, that were not fixed until .3, I don't want us to repeat the mistake. This bug can be fixed just as well by checking the state of the notification before popping up the menu. Then, in 3.9.1, we can consider the correct fix.
Review of attachment 238815 [details] [review]: This one seems correct, but I don't feel it is safe for 3.8.
Review of attachment 238816 [details] [review]: And this has regressions all over it. For example, the sizing and positioning code in notificationWidget assumes a certain set of expand flags on its parent, and probably assumes a certain style hierarchy too.
(In reply to comment #6) > Review of attachment 238815 [details] [review]: > > This one seems correct, but I don't feel it is safe for 3.8. The core change here is very small -- move the notify::hover to notificationWidget. The rest is just renaming all the bookkeeping. (In reply to comment #7) > Review of attachment 238816 [details] [review]: > > And this has regressions all over it. For example, the sizing and positioning > code in notificationWidget assumes a certain set of expand flags on its parent, > and probably assumes a certain style hierarchy too. You encountered regressions when testing, or you assume there will be regressions? I don't see any assumptions about the set of expand flags on its parent -- the tray box is allocated the full width, and it wants to be centered inside that. The expand flags on the child are the standard ClutterBinLayout workaround. All we're doing is removing the #message-tray parent. There's no child selectors for #message-tray, and all three properties in #message-tray are non-inheriting.
(In reply to comment #8) > (In reply to comment #6) > > Review of attachment 238815 [details] [review] [details]: > > > > This one seems correct, but I don't feel it is safe for 3.8. > > The core change here is very small -- move the notify::hover to > notificationWidget. The rest is just renaming all the bookkeeping. Which is the unsafe part. It's easy to forget one place where the variable is used, or to introduce a typo. > (In reply to comment #7) > > Review of attachment 238816 [details] [review] [details]: > > > > And this has regressions all over it. For example, the sizing and positioning > > code in notificationWidget assumes a certain set of expand flags on its parent, > > and probably assumes a certain style hierarchy too. > > You encountered regressions when testing, or you assume there will be > regressions? I don't see any assumptions about the set of expand flags on its > parent -- the tray box is allocated the full width, and it wants to be centered > inside that. The expand flags on the child are the standard ClutterBinLayout > workaround. No regressions yet, but yes, I expect there will be.
Comment on attachment 238814 [details] [review] messageTray: Remove some dead positioning code Attachment 238814 [details] pushed as dc54472 - messageTray: Remove some dead positioning code Pushing just this one for now.
Review of attachment 238816 [details] [review]: Uhm, I've been testing this for a while without seeing regressions, and we're out of 3.8, so just push it.
Attachment 238815 [details] pushed as 1b13509 - messageTray: Move hover tracking to the notification widget Attachment 238816 [details] pushed as 8c32102 - messageTray: Move the notification actor out of the tray