GNOME Bugzilla – Bug 637135
Add nice UI for password-less accounts
Last modified: 2010-12-15 08:59:57 UTC
Following on from the previous bug: http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/sasl-gui This should all be working now. It needs the tp-glib released on Friday, and will only work with haze or gabble from git master. It should act just as before on older versions though. Cooooooool.
Created attachment 176328 [details] [review] squashed diff
Review of attachment 176328 [details] [review]: Did you try editing and creating new jabber/haze accounts? (just to be sure you checked for regressions). ::: configure.ac @@ +40,3 @@ LIBNOTIFY_REQUIRED=0.7.0 TELEPATHY_FARSIGHT_REQUIRED=0.0.14 +TELEPATHY_GLIB_REQUIRED=0.13.9 I already updated jhbuild and the wiki so you don't have to do it. ::: libempathy/empathy-account-settings.c @@ +369,3 @@ + if (priv->protocol_obj != NULL) + g_object_unref (priv->protocol_obj); + priv->protocol_obj = NULL; In tp_clear_object() we trust. ::: libempathy/empathy-auth-factory.c @@ +476,3 @@ NULL)); + tp_base_client_take_observer_filter (client, tp_asv_new ( A comment explaining why we observe would be nice as it's not straightforward ::: src/empathy-event-manager.c @@ +172,3 @@ } + if (event->public.account) if (bla != NULL) or tp_clear_object(). @@ +1332,3 @@ + + priv->auth_approver = tp_simple_approver_new (dbus, + "Empathy.AuthEventManager", FALSE, approve_channels, manager, I'm wondering if the event-manager is the best place to do this. It doesn't seem to fit really well in it. Shouldn't it be clearer to be its own object? ::: src/empathy-main-window.c @@ +145,3 @@ guint size_timeout_id; GHashTable *errors; + GHashTable *auths; Please document the key/value and their ownership (yeah I know, the errors is not properly documented, bouh!) @@ +378,3 @@ + add_button = gtk_button_new (); + gtk_button_set_image (GTK_BUTTON (add_button), image); + gtk_widget_set_tooltip_text (add_button, _("Provide Password")); Wouldn't it better to allow user to type the password there? @@ +384,3 @@ + close_button = gtk_button_new (); + gtk_button_set_image (GTK_BUTTON (close_button), image); + gtk_widget_set_tooltip_text (close_button, _("Cancel")); 'Cancel' isn't very clear imho. Shouldn't it be "Disconnect" or something? @@ +1804,3 @@ NULL); + priv->auths = g_hash_table_new_full (g_direct_hash, you can use g_hash_table_new (NULL, NULL) as a simple optimisation.
Oh, and can we close bug #586562 as a dup of this bug?
(In reply to comment #2) > Review of attachment 176328 [details] [review]: > > Did you try editing and creating new jabber/haze accounts? (just to be sure you > checked for regressions). Yep, and I also made sure the account assistant works still too. > +TELEPATHY_GLIB_REQUIRED=0.13.9 > > I already updated jhbuild and the wiki so you don't have to do it. Huh? But we still need empathy to check for the right version of tp-glib? I don't understand. > ::: libempathy/empathy-account-settings.c > @@ +369,3 @@ > + if (priv->protocol_obj != NULL) > + g_object_unref (priv->protocol_obj); > + priv->protocol_obj = NULL; > > In tp_clear_object() we trust. I actually tried to keep the freeing consistent with the rest of the function, so I didnt scatter if (foo) g_blah; and tp_clear_badger. Fixed anyway. > + tp_base_client_take_observer_filter (client, tp_asv_new ( > > A comment explaining why we observe would be nice as it's not straightforward Done. > ::: src/empathy-event-manager.c > @@ +172,3 @@ > } > > + if (event->public.account) > > if (bla != NULL) or tp_clear_object(). Done. > @@ +1332,3 @@ > + > + priv->auth_approver = tp_simple_approver_new (dbus, > + "Empathy.AuthEventManager", FALSE, approve_channels, manager, > > I'm wondering if the event-manager is the best place to do this. It doesn't > seem to fit really well in it. > Shouldn't it be clearer to be its own object? I'm not sure. I think it's okay, especially for now. I agree that if it grew any more and more functionality was added perhaps it'd be worthwhile putting it elsewhere. > + GHashTable *auths; > > Please document the key/value and their ownership (yeah I know, the errors is > not properly documented, bouh!) Heh, okay. :-) > @@ +378,3 @@ > + add_button = gtk_button_new (); > + gtk_button_set_image (GTK_BUTTON (add_button), image); > + gtk_widget_set_tooltip_text (add_button, _("Provide Password")); > > Wouldn't it better to allow user to type the password there? In the info bar? I think it'd look really bad? > @@ +384,3 @@ > + close_button = gtk_button_new (); > + gtk_button_set_image (GTK_BUTTON (close_button), image); > + gtk_widget_set_tooltip_text (close_button, _("Cancel")); > > 'Cancel' isn't very clear imho. Shouldn't it be "Disconnect" or something? It's cancel the authentication. Perhaps it's not clear enough though. I'll change it. > @@ +1804,3 @@ > NULL); > > + priv->auths = g_hash_table_new_full (g_direct_hash, > > you can use g_hash_table_new (NULL, NULL) as a simple optimisation. Done. I pushed my changes to the same place. No rebasing so it should be clear which patches are new and address your comments. Thanks for the review!
(In reply to comment #4) > (In reply to comment #2) > > Review of attachment 176328 [details] [review] [details]: > > > > Did you try editing and creating new jabber/haze accounts? (just to be sure you > > checked for regressions). > > Yep, and I also made sure the account assistant works still too. awesome. > > +TELEPATHY_GLIB_REQUIRED=0.13.9 > > > > I already updated jhbuild and the wiki so you don't have to do it. > > Huh? But we still need empathy to check for the right version of tp-glib? I > don't understand. Sure. But when we bump and external dep we have to update jhbuild and http://live.gnome.org/TwoPointNinetyone/ExternalDependencies as well. > > + add_button = gtk_button_new (); > > + gtk_button_set_image (GTK_BUTTON (add_button), image); > > + gtk_widget_set_tooltip_text (add_button, _("Provide Password")); > > > > Wouldn't it better to allow user to type the password there? > > In the info bar? I think it'd look really bad? I don't know. Well we can keep it as it for now, see how it goes and improve later if needed. > I pushed my changes to the same place. No rebasing so it should be clear which > patches are new and address your comments. Thanks for the review! Ok, let's merge this thing. :)
Woo, thanks!
*** Bug 586562 has been marked as a duplicate of this bug. ***