GNOME Bugzilla – Bug 601458
Add presence items to status menu and port to JS
Last modified: 2009-11-13 19:22:35 UTC
This ports ShellStatusMenu to JS (it was originally C because it was just copied from the f-u-s applet) and then adds the presence/messaging items to it. In the process I also removed some bits that weren't really used in the C code, and ported from Big to St as well. There are still some FIXMEs; the error handling when launching external programs is non-existent (but we use the same interface as the run dialog and should probably share a "failed" dialog with that). Also, the presence icons are probably wrong, but Jon knows we need right ones. This doesn't actually depend on the message-tray branch. It just talks to gnome-session, and affects anything that listens to that.
Created attachment 147419 [details] [review] Introspect src/gdmuser In addition to the Makefile changes, we also change uid_t to gulong in the public API (which matches how it was already represented in the gobject properties).
Created attachment 147420 [details] [review] Port ShellStatusMenu to javascript
Created attachment 147421 [details] [review] Add Presence items to the status menu
Review of attachment 147419 [details] [review]: Looks good to me
Review of attachment 147420 [details] [review]: Looks good, various comments in detail ::: js/ui/statusMenu.js @@ +42,3 @@ + + _onDestroy: function() { + this._user.disconnect(this._userNameChangedId); Think this will warn if this._user == null, which you try to handle on connection Shouldn't you destroy this._menu() as well? @@ +51,3 @@ + this._updateNameText(); + + if (this._user) { This is the place that I'm thinking of that tries to handle _user == NULL @@ +53,3 @@ + if (this._user) { + this._userNameChangedId = this._user.connect('notify::display-name', Lang.bind(this, this._updateNameText)); + } Excess {} @@ +57,3 @@ + + _updateNameText: function() { + this._name.set_text(this._user.get_real_name()); But this doesn't handle this._user == NULL @@ +62,3 @@ + _updateSwitchUser: function() { + let users = this._gdm.list_users(); + if (users && users.length > 1) Doesn't list_users() always return an GSList => array? why the 'users &&' ? @@ +77,3 @@ + function() { + image.set_pixel_size(this._pixelSize); + image.set_from_icon_name(iconName, this._iconSize); I would have guessed this was related to the gir-repository bug you filed about GtkIconSize, but the C code seems to have done it the same way rather than setting the icon_size property. Do you know if there was a reason for that? @@ +94,3 @@ + function() { + this._spawn(['gnome-about-me']); + })); It's weird to me that some of the items that just spawn things are inlined and some have separate callbacks (is the distinction whether they take arguments or not?); I think either is fine as long as it is consistent @@ +149,3 @@ + let settings; + if (this._menu.has_screen()) + settings = Gtk.Settings.get_for_screen(this._menu.get_screen()); I don't see how menu.has_screen() could ever be false ... GtkMenu is put inside a GtkWindow in its _init() and gtk_widget_has_screen() for a window inside a toplevel is always %TRUE @@ +178,3 @@ + + _spawn: function(args) { + // FIXME: errors? Not sure the half-hearted fixme adds anything; either it needs to be fixed, or it doesn't @@ +206,3 @@ + } + if (!panel) + return; How could this happen? Better to let it backtrace than silently do nothing? ::: src/shell-global.c @@ +1043,3 @@ + * @menu: a #GtkMenu + * @x: x coordinate of mouse click + * @y: y coordinate of mouse click You don't use these - why are they passed in?
Review of attachment 147421 [details] [review]: Generally looks good to me, just a few questions about the UI from reading the patch (didn't actually try it out) ::: js/ui/statusMenu.js @@ +89,3 @@ + this._iconBox.child = this._invisibleIcon; + else + this._iconBox.child = null; Does this cause the notification area to move over when idle? does that look weird? @@ +116,3 @@ + item = this._createImageMenuItem(_('Available'), 'gtk-yes'); + item.connect('activate', Lang.bind(this, this._setPresenceStatus, GnomeSessionPresenceStatus.AVAILABLE)); + this._menu.append(item); Hmm, I guess the icons won't be shown by default with the way we configure GTK+ in the GNOME Desktop. I wonder if the currently selected item should be bold to make it clear what one corresponds to the current status.
> Looks good, various comments in detail So, the answer to most of your questions is "because that's how the C version did it". > This is the place that I'm thinking of that tries to handle _user == NULL That's a bug; the C code checked user!=NULL everywhere, except there wasn't actually any way user could ever be NULL. So I removed most of the checks, but missed that one. > Doesn't list_users() always return an GSList => array? why the 'users &&' ? ah, right, NULL return becomes [], not null > I would have guessed this was related to the gir-repository bug you filed about > GtkIconSize, but the C code seems to have done it the same way rather than > setting the icon_size property. Do you know if there was a reason for that? Nope. (Oh, but this patch depends on the gir-repository one.) > It's weird to me that some of the items that just spawn things are inlined and > some have separate callbacks The ones that aren't inlined were longer in an earlier version. (Eg, the "lock screen" callback tried to work with both gnome-screensaver and xscreensaver, and also made a second call to gnome-screensaver-command passing it an invalid argument.) I'll make them consistent. > I don't see how menu.has_screen() could ever be false Probably can't be. More straight porting from the C version. > + // FIXME: errors? > > Not sure the half-hearted fixme adds anything; either it needs to be fixed, or > it doesn't The C version was equally lame, so this isn't a regression. But it's lame. ShellProcess doesn't give any error information, so we can't do anything right now anyway. (And also, do we make a clutter-based error dialog? Or use zenity?) > You don't use these - why are they passed in? Hm... I think I was thinking gtk_menu_popup() took them.
oops, forgot to comment on your second set of comments yesterday > Does this cause the notification area to move over when idle? does that look > weird? actually, I'd forgotten about the "idle" state when I wrote that, so the "else" was more of a "can't happen". But since it can't happen I'll just ignore that case. (So it shows the "idle" icon in the can't happen case now.) > Hmm, I guess the icons won't be shown by default with the way we configure GTK+ > in the GNOME Desktop. Oh... hm... interesting. I must have tried out no-icons-in-menus previously before we switched the default, and then switched back, and so as a result now I don't get the new default (and can't change it in the UI since there's no UI any more). Anyway, I fixed it to set the always_show_image property on those items (but not the others). (In reply to comment #7) > > I would have guessed this was related to the gir-repository bug you filed about > > GtkIconSize, but the C code seems to have done it the same way rather than > > setting the icon_size property. Do you know if there was a reason for that? > > Nope. I started chasing the C code back through the various modules it's been imported through to find the original reason, but gave up. But yeah, it seems like that code is just totally broken and I've simplified it now.
Created attachment 147488 [details] [review] Port ShellStatusMenu to javascript make more consistent, remove weird historical cruft
Created attachment 147489 [details] [review] Add Presence items to the status menu add idle icon too. set always_show_image.
(In reply to comment #9) > > + // FIXME: errors? > > > > Not sure the half-hearted fixme adds anything; either it needs to be fixed, or > > it doesn't > > The C version was equally lame, so this isn't a regression. But it's lame. > ShellProcess doesn't give any error information, so we can't do anything right > now anyway. (And also, do we make a clutter-based error dialog? Or use zenity?) This was more of a philosophical comment about FIXME comments. As I see it, there are three purposes of a FIXME 1) As a marker of something that is a problem that really needs to be fixed but it would break your coding flow to fix it when you are first writing it. These should never be committed. (I tend to use the BSD style XXX for these because it looks ugly to me and it will pop-out when looking back over the code) 2) As a way of recording specific information that you have now about some problem that you think is low priority and doesn't need to be fixed but you expect that someone might come back to and fix later // FIXME: we don't handle errors here. To handle errors, we'd need // to extend ShellProcess to throw an exception on error. 3) As a help to the reader who is wondering why you didn't handle some error condition // FIXME: we don't handle errors here. We expect the programs that // we are launching to always be installed on a functioning GNOME // install so there's not much we can do on failure other than // tell the user that there install is broken Both of those are fine (though I'm not sure the FIXME tag adds anything), but FIXME's that don't add extra information always raise the question to me "OK, so did this need to be fixed or not? If it did, why didn't the person who was writing it just fix it? If it didn't, then why are they wasting my time with this comment?" and then there's always the disheartening feeling coming back to a "FIXME: this is a bad hack" that I wrote a year ago and having absolutely no idea what I meant or why it's a bad hack. Anyways, enough FIXME philosophy :-)
Review of attachment 147488 [details] [review]: Looks good to me.
Review of attachment 147489 [details] [review]: Looks good
Attachment 147419 [details] pushed as 9ddebf0 - Introspect src/gdmuser Attachment 147488 [details] pushed as 985d707 - Port ShellStatusMenu to javascript Attachment 147489 [details] pushed as bb63d51 - Add Presence items to the status menu