GNOME Bugzilla – Bug 682683
ScreenShield/MessageTray: fix crash after the introduction of close buttons
Last modified: 2012-09-03 21:13:22 UTC
Commit 5c6b1fd0c modified the hierarchy of SummaryItem contents, by introducing a notificationStackWidget in place of the StackView, but forgot to update the bits in ScreenShield.NotificationsBox that accessed and reparented that directly, causing a crash by invalid theme node access.
Created attachment 222427 [details] [review] ScreenShield/MessageTray: fix crash after the introduction of close buttons
Review of attachment 222427 [details] [review]: The close button fixes seem unrelated. ::: js/ui/messageTray.js @@ +1260,3 @@ + + set closeButtonVisible(v) { + this._closeButton.visible = v; The notification controls its own closeButton visibility with show/hide. Maybe you should have a policy setter instead (showCloseButton = false; in screenShield) @@ +2333,2 @@ this._summaryBoxPointer.bin.child = this._summaryBoxPointerItem.notificationStackWidget; + this._summaryBoxPointerItem.closeButtonVisible = true; I don't understand this. It should already be fine, right?
(In reply to comment #2) > Review of attachment 222427 [details] [review]: > > The close button fixes seem unrelated. No they're not: previous code assumed that all of the summary icon nSV was good to show, which is wrong after the introduction of the close button. > ::: js/ui/messageTray.js > @@ +1260,3 @@ > + > + set closeButtonVisible(v) { > + this._closeButton.visible = v; > > The notification controls its own closeButton visibility with show/hide. Maybe > you should have a policy setter instead (showCloseButton = false; in > screenShield) No, the notification does not interact with closeButtons. The message tray controls its own closeButton, but that's a different one. This is the one shown in summary mode. > @@ +2333,2 @@ > this._summaryBoxPointer.bin.child = > this._summaryBoxPointerItem.notificationStackWidget; > + this._summaryBoxPointerItem.closeButtonVisible = true; > > I don't understand this. It should already be fine, right? Not after you go to the lock screen and back.
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 222427 [details] [review] [details]: > > > > The close button fixes seem unrelated. > > No they're not: previous code assumed that all of the summary icon nSV was good > to show, which is wrong after the introduction of the close button. I'd still prefer this in a separate patch. > > ::: js/ui/messageTray.js > > @@ +1260,3 @@ > > + > > + set closeButtonVisible(v) { > > + this._closeButton.visible = v; > > > > The notification controls its own closeButton visibility with show/hide. Maybe > > you should have a policy setter instead (showCloseButton = false; in > > screenShield) > > No, the notification does not interact with closeButtons. The message tray > controls its own closeButton, but that's a different one. This is the one shown > in summary mode. er, I was thinking "summary item" in my head, but wrote "notification" > > @@ +2333,2 @@ > > this._summaryBoxPointer.bin.child = > > this._summaryBoxPointerItem.notificationStackWidget; > > + this._summaryBoxPointerItem.closeButtonVisible = true; > > > > I don't understand this. It should already be fine, right? > > Not after you go to the lock screen and back. But this is for the summary box pointer? What's the steps to reproduce this?
Review of attachment 222427 [details] [review]: ::: js/ui/messageTray.js @@ -1263,3 @@ } - this.notificationStackWidget.width = this.notificationStackView.width; I left this in on purpose. Why does this break?
(In reply to comment #4) > > > > No, the notification does not interact with closeButtons. The message tray > > controls its own closeButton, but that's a different one. This is the one shown > > in summary mode. > > er, I was thinking "summary item" in my head, but wrote "notification" Even so, the summary item does not control the visibility of the closeButton, it merely adds to nSW. > > > @@ +2333,2 @@ > > > this._summaryBoxPointer.bin.child = > > > this._summaryBoxPointerItem.notificationStackWidget; > > > + this._summaryBoxPointerItem.closeButtonVisible = true; > > > > > > I don't understand this. It should already be fine, right? > > > > Not after you go to the lock screen and back. > > But this is for the summary box pointer? What's the steps to reproduce this? - Have a resident notification - Go to the lock screen - Without the patch, it just crashes immediately - Exit the lock screen - Open the notification in summary mode - With the patch, without this line, the screenshield hides the close button, and nothing else shows it, so you don't have a close button there (In reply to comment #5) > Review of attachment 222427 [details] [review]: > > ::: js/ui/messageTray.js > @@ -1263,3 @@ > } > > - this.notificationStackWidget.width = this.notificationStackView.width; > > I left this in on purpose. Why does this break? ScreenShield.NotificationsBox builds its contents before being added to the lockScreenContents actor (differently from MessageTray / SummaryItem), but accessing notificationStackView.width causes a get_preferred_width(), that in turn causes a get_theme_node() and crashes.
Attachment 222427 [details] pushed as 47d46e3 - ScreenShield/MessageTray: fix crash after the introduction of close buttons