GNOME Bugzilla – Bug 680426
Bunch of assorted changes
Last modified: 2012-10-16 16:53:52 UTC
I'm getting lazier and lazier with these bug filings. See patches.
Created attachment 219452 [details] [review] st-widget: Move reactivity tracking to StWidget, use "insensitive" This lets use remove another few pieces of code that do the tracking manually.
Created attachment 219453 [details] [review] st-widget: Move "can focus" logic into get_can_focus Rather than having the logic of whether we can *actually* focus a widget in each container, move it into the st_widget_get_can_focus() getter itself so that the accessibility stack and manual users of the can_focus property don't have to implement the check themselves.
Created attachment 219454 [details] [review] st-theme: Make the custom stylesheets have higher priority
Created attachment 219455 [details] [review] util: Fix number of parameters passed to spawn_sync We passed one too many
Created attachment 219456 [details] [review] boxpointer: Cope with a missing -arrow-border-color when we have no border
Created attachment 219457 [details] [review] messageTray: Use a GIcon for a notification's icon/secondary icon Using a GIcon instead of an actor means that we can always create a new icon with the right size from an old icon.
Created attachment 219458 [details] [review] autorunManager: Remove line that sets summary icon MessageTray does this automatically for us now.
Created attachment 219459 [details] [review] messageTray: Hook SourceActor up to source icon changes automatically Instead of manually tracking source icon changes, or requiring a manual call to _setSummaryIcon, add a way to emit a signal when we're guaranteed the icon has been changed, and then the source actor will automatically update the icon. Note that this will not work for legacy tray icons, as they require an X window to be mapped and on the screen in the right spot.
Review of attachment 219453 [details] [review]: Hmm this is kind of odd ... it means that setting can_focus to true could result into get_can_focus() returning false (for non reactive actors). So I'd rather use a separate method for this rather then abusing the getter.
Review of attachment 219454 [details] [review]: Looks good.
Review of attachment 219452 [details] [review]: Looks good.
Review of attachment 219455 [details] [review]: OK.
Attachment 219452 [details] pushed as 414fe75 - st-widget: Move reactivity tracking to StWidget, use "insensitive" Attachment 219454 [details] pushed as 75e4961 - st-theme: Make the custom stylesheets have higher priority Attachment 219455 [details] pushed as fd87468 - util: Fix number of parameters passed to spawn_sync
Created attachment 219855 [details] [review] lookingGlass: Don't pass too many arguments to Clutter.ungrab_*
Created attachment 219856 [details] [review] status: Clean up imports
Created attachment 220270 [details] [review] lookingGlass: Don't pass too many arguments to Clutter.ungrab_*
Created attachment 220271 [details] [review] status: Clean up imports
Review of attachment 220270 [details] [review]: Yep
Review of attachment 219456 [details] [review]: Ok
Review of attachment 219457 [details] [review]: The problem with this is that not all icons are of the same type, sometimes we want symbolic and sometimes we want fullcolor. (Or we could remove IconType altogether :) )
Review of attachment 219458 [details] [review]: .
Review of attachment 219459 [details] [review]: In fact, this doesn't seem to work for tray icons at all... Marking needs-work for now
Review of attachment 220271 [details] [review]: Ok
Attachment 219456 [details] pushed as e7e56e1 - boxpointer: Cope with a missing -arrow-border-color when we have no border Attachment 219458 [details] pushed as d8390ef - autorunManager: Remove line that sets summary icon Attachment 220270 [details] pushed as 66197b1 - lookingGlass: Don't pass too many arguments to Clutter.ungrab_* Attachment 220271 [details] pushed as e875b9c - status: Clean up imports
Created attachment 220297 [details] [review] messageTray: Hook SourceActor up to source icon changes automatically Instead of manually tracking source icon changes, or requiring a manual call to _setSummaryIcon, add a way to emit a signal when we're guaranteed the icon has been changed, and then the source actor will automatically update the icon. _setSummaryIcon is still available for sources such as the notification daemon, that require special treatment for the summary icon (to be used with tray icons) This one works, tested with Liferea configured to do tray icons + notifications. It also fixes the icon not appearing in the lock screen for new sources.
(In reply to comment #22) > Review of attachment 219459 [details] [review]: > > In fact, this doesn't seem to work for tray icons at all... Marking needs-work > for now This worked for me with gtk's teststatusicon. What's wrong with it?
(In reply to comment #26) > (In reply to comment #22) > > Review of attachment 219459 [details] [review] [details]: > > > > In fact, this doesn't seem to work for tray icons at all... Marking needs-work > > for now > > This worked for me with gtk's teststatusicon. What's wrong with it? Reading it again, nothing. It's just that it had no code at all to work with the NotificationDaemon, and thus was completely useless for non-mainIcon SourceActors.
(In reply to comment #27) > Reading it again, nothing. It's just that it had no code at all to work with > the NotificationDaemon, and thus was completely useless for non-mainIcon > SourceActors. We shouldn't put status icons in the screen shield, I thought. I'm not even sure how that would work.
(In reply to comment #28) > (In reply to comment #27) > > Reading it again, nothing. It's just that it had no code at all to work with > > the NotificationDaemon, and thus was completely useless for non-mainIcon > > SourceActors. > > We shouldn't put status icons in the screen shield, I thought. I'm not even > sure how that would work. Yes, we should: as far as the user is concerned, that's the icon for the notification source, and it doesn't matter if it's a X Window, a themed icon, a serialized ARGB image or whatnot. And yes, it's possible: you just need to use ClutterClone. Which is deprecated and everything, but it works fine for Clutter 1.0.
(In reply to comment #29) > > Yes, we should: as far as the user is concerned, that's the icon for the > notification source, and it doesn't matter if it's a X Window, a themed icon, a > serialized ARGB image or whatnot. But we don't handle clicks on it at all. Actually, what happens if the X window listens to clicks, and then sends out a notification. Clicking on the status icon would cause it to run the click handler and not open the notification stack, right? > And yes, it's possible: you just need to use ClutterClone. Which is deprecated > and everything, but it works fine for Clutter 1.0.
(In reply to comment #30) > (In reply to comment #29) > > > > Yes, we should: as far as the user is concerned, that's the icon for the > > notification source, and it doesn't matter if it's a X Window, a themed icon, a > > serialized ARGB image or whatnot. > > But we don't handle clicks on it at all. > > Actually, what happens if the X window listens to clicks, and then sends out a > notification. Clicking on the status icon would cause it to run the click > handler and not open the notification stack, right? I don't understand this. For _mainIcon / getSummaryIcon(), we have the actual ShellEmbeddedWindow there, so it's X that delivers the events. For the screenshield the source actor is not reactive, so no problem there. Anyway, rebased.
Created attachment 221644 [details] [review] messageTray: Hook SourceActor up to source icon changes automatically Instead of manually tracking source icon changes, or requiring a manual call to _setSummaryIcon, add a way to emit a signal when we're guaranteed the icon has been changed, and then the source actor will automatically update the icon. _setSummaryIcon is still available for sources such as the notification daemon, that require special treatment for the summary icon (to be used with tray icons)
Review of attachment 219453 [details] [review]: Accessibility is changing the logic anyway. Nope.
(In reply to comment #33) > Review of attachment 219453 [details] [review]: > > Accessibility is changing the logic anyway. Nope. Btw, all JS paths that got can_focus setters removed when I tied can_focus to reactive need fixing now.
(In reply to comment #34) > (In reply to comment #33) > > Review of attachment 219453 [details] [review] [details]: > > > > Accessibility is changing the logic anyway. Nope. > > Btw, all JS paths that got can_focus setters removed when I tied can_focus to > reactive need fixing now. Heh. When did that happen?
(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > Review of attachment 219453 [details] [review] [details] [details]: > > > > > > Accessibility is changing the logic anyway. Nope. > > > > Btw, all JS paths that got can_focus setters removed when I tied can_focus to > > reactive need fixing now. > > Heh. When did that happen? 26d3b1929e2cd9dcedf829a2224857a533040729
OK, it seems we can just revert the other half of that patch. https://bugzilla.gnome.org/show_bug.cgi?id=667439
Review of attachment 221644 [details] [review]: Looks fine.
Comment on attachment 221644 [details] [review] messageTray: Hook SourceActor up to source icon changes automatically Attachment 221644 [details] pushed as 5991c8d - messageTray: Hook SourceActor up to source icon changes automatically
(In reply to comment #20) > Review of attachment 219457 [details] [review]: > > The problem with this is that not all icons are of the same type, sometimes we > want symbolic and sometimes we want fullcolor. > > (Or we could remove IconType altogether :) ) Now what?
Review of attachment 219457 [details] [review]: Yeah, sure!
Created attachment 224330 [details] [review] messageTray: Use a GIcon for a notification's icon/secondary icon Using a GIcon instead of an actor means that we can always create a new icon with the right size from an old icon. Let's first update it to actually apply to git master. Network and telepathy stuff is untested.
In fact... JS ERROR: !!! Exception was: Error: Unrecognized parameter "secondaryIcon" JS ERROR: !!! message = '"Unrecognized parameter "secondaryIcon""' JS ERROR: !!! fileName = '"/opt/gnome/share/gnome-shell/js/misc/params.js"' JS ERROR: !!! lineNumber = '25' JS ERROR: !!! stack = '"parse([object Object],[object Object])@/opt/gnome/share/gnome-shell/js/misc/params.js:25 ("Luca Versari",null,[object Object])@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:399 wrapper("Luca Versari",null,[object Object])@/opt/gnome/share/gjs-1.0/lang.js:204 ([object Object],"Luca Versari",null,[object Object])@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:380 wrapper([object Object],"Luca Versari",null,[object Object])@/opt/gnome/share/gjs-1.0/lang.js:204 _parent([object Object],"Luca Versari",null,[object Object])@/opt/gnome/share/gjs-1.0/lang.js:166 ([object Object])@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:766 wrapper([object Object])@/opt/gnome/share/gjs-1.0/lang.js:204 ([object Object])@/opt/gnome/share/gjs-1.0/lang.js:145 ([object Object])@/opt/gnome/share/gjs-1.0/lang.js:239 ([object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object _private_TelepathyGLib_TextChannel],[object _private_TelepathyGLib_Contact],[object _private_Shell_TpClient])@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:460 wrapper([object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object _private_TelepathyGLib_TextChannel],[object _private_TelepathyGLib_Contact],[object _private_Shell_TpClient])@/opt/gnome/share/gjs-1.0/lang.js:204 ([object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object _private_TelepathyGLib_TextChannel],[object _private_TelepathyGLib_Contact],[object _private_Shell_TpClient])@/opt/gnome/share/gjs-1.0/lang.js:145 ([object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object _private_TelepathyGLib_TextChannel],[object _private_TelepathyGLib_Contact],[object _private_Shell_TpClient])@/opt/gnome/share/gjs-1.0/lang.js:239 ([object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object _private_TelepathyGLib_TextChannel],[object _private_TelepathyGLib_Contact])@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:162 wrapper([object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object _private_TelepathyGLib_TextChannel],[object _private_TelepathyGLib_Contact])@/opt/gnome/share/gjs-1.0/lang.js:204 ([object _private_Shell_TpClient],[object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object Array],null,[object Array],[object _private_TelepathyGLib_ObserveChannelsContext])@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:152 wrapper([object _private_Shell_TpClient],[object _private_TelepathyGLib_Account],[object _private_TelepathyGLib_Connection],[object Array],null,[object Array],[object _private_TelepathyGLib_ObserveChannelsContext])@/opt/gnome/share/gjs-1.0/lang.js:204
You also lost the 'secondary-icon' style class, so secondary icons are larger. Tested both telepathy and network, no other regressions noticed. PS: if you need telepathy testing in the future, feel free to ping me in GTalk.
Created attachment 224367 [details] [review] messageTray: Use a GIcon for a notification's icon/secondary icon Using a GIcon instead of an actor means that we can always create a new icon with the right size from an old icon.
Review of attachment 224367 [details] [review]: ::: js/ui/messageTray.js @@ +458,3 @@ + this._secondaryIcon = new St.Icon({ gicon: params.secondaryGIcon, + style_class: 'secondary-icon', + icon_size: this.ICON_SIZE }); icon_size overrides the size from style_class, so the icon is still bigger than designed.
Created attachment 224385 [details] [review] messageTray: Use a GIcon for a notification's icon/secondary icon Using a GIcon instead of an actor means that we can always create a new icon with the right size from an old icon.
Created attachment 224386 [details] [review] messageTray: Primarily use a GIcon to drive the source's icon This is a bit of a cleanup since we ported notification icons/secondary icons to be in the same situation.
Review of attachment 224385 [details] [review]: Wrong patch? You didn't fix my comment...
Review of attachment 224386 [details] [review]: ::: js/ui/messageTray.js @@ +1124,3 @@ // something more fancy. createIcon: function(size) { + return new St.Icon({ gicon: this._gicon, this.getIcon() maybe?
Created attachment 224408 [details] [review] messageTray: Use a GIcon for a notification's icon/secondary icon Using a GIcon instead of an actor means that we can always create a new icon with the right size from an old icon.
Created attachment 224409 [details] [review] messageTray: Primarily use a GIcon to drive the source's icon This is a bit of a cleanup since we ported notification icons/secondary icons to be in the same situation.
Review of attachment 224408 [details] [review]: Go!
Review of attachment 224409 [details] [review]: Yes
Are there any outstanding 3.6 fixes that depend on those patches? If not, I'd prefer them to land after branching at this point. There are still occasional regressions from the StIconType and sessionMode/components changes, no need to add another source of possible regressions IMHO.
Right. I was not going to land them at this point.
Thanks.
Attachment 224408 [details] pushed as 928ea3b - messageTray: Use a GIcon for a notification's icon/secondary icon Attachment 224409 [details] pushed as f5974f6 - messageTray: Primarily use a GIcon to drive the source's icon