After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 637135 - Add nice UI for password-less accounts
Add nice UI for password-less accounts
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Auth client
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
: 586562 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-12-13 09:49 UTC by Jonny Lamb
Modified: 2010-12-15 08:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
squashed diff (77.54 KB, patch)
2010-12-13 09:52 UTC, Jonny Lamb
reviewed Details | Review

Description Jonny Lamb 2010-12-13 09:49:31 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.
Comment 1 Jonny Lamb 2010-12-13 09:52:38 UTC
Created attachment 176328 [details] [review]
squashed diff
Comment 2 Guillaume Desmottes 2010-12-13 13:07:04 UTC
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.
Comment 3 Guillaume Desmottes 2010-12-13 13:08:22 UTC
Oh, and can we close bug #586562 as a dup of this bug?
Comment 4 Jonny Lamb 2010-12-14 16:42:38 UTC
(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!
Comment 5 Guillaume Desmottes 2010-12-15 08:49:44 UTC
(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. :)
Comment 6 Jonny Lamb 2010-12-15 08:59:30 UTC
Woo, thanks!
Comment 7 Jonny Lamb 2010-12-15 08:59:57 UTC
*** Bug 586562 has been marked as a duplicate of this bug. ***