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 703487 - Show typing icon against composing members of a MUC
Show typing icon against composing members of a MUC
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Multi User Chat
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
Depends on:
Blocks:
 
 
Reported: 2013-07-02 20:51 UTC by Chandni Verma
Modified: 2013-08-14 12:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Show typing icon against composing members of a MUC (4.33 KB, patch)
2013-07-31 09:26 UTC, Chandni Verma
none Details | Review
Show typing icon against composing members of a MUC (4.49 KB, patch)
2013-08-06 08:30 UTC, Chandni Verma
reviewed Details | Review
Make individual_store_find_contact and free_iters public (3.62 KB, patch)
2013-08-14 08:40 UTC, Chandni Verma
reviewed Details | Review
Show typing icon against composing members of a MUC (3.16 KB, patch)
2013-08-14 08:40 UTC, Chandni Verma
reviewed Details | Review

Description Chandni Verma 2013-07-02 20:51:10 UTC
<glassrose> why can't I see the members of a MUC "composing" as they type?
* glassrose asks herself
<rishi> That might depend on whether the protocol supports it.
<glassrose> No I tried a MUC with jabber accounts and couldn't produce the typing icons adjacent to the typing members of the MUC. Something has gone wrong since I implemented it.


This feature was first introduced by me in commit 1acf899870 and was lost in commit 4f470e26bafd.

Why not bring it back.
Comment 1 Chandni Verma 2013-07-07 18:32:45 UTC
It is not easy to see how one would go for it.
The IndividualStore deals with complete individuals while compositors are EmpathyContacts.
Guillaume, can you explain how one would proceed to make it possible?
Comment 2 Guillaume Desmottes 2013-07-23 15:02:31 UTC
I didn't touch this code since a while, but you could fetch the TpContact from the individual and check the typing status from there using Telepathy API directly I think.
Comment 3 Chandni Verma 2013-07-31 09:25:15 UTC
I have "contact-chat-state-changed" signal connected to in EmpathyIndividualStoreChannel to track changes in chat states and have update the status-icon pixbuf in the store accordingly.

Patch follows.
Comment 4 Chandni Verma 2013-07-31 09:26:25 UTC
Created attachment 250527 [details] [review]
Show typing icon against composing members of a MUC
Comment 5 Chandni Verma 2013-08-06 08:28:24 UTC
The above patch didn't chain up to the parent's constructed method. Revised patch follows.
Comment 6 Chandni Verma 2013-08-06 08:30:43 UTC
Created attachment 250929 [details] [review]
Show typing icon against composing members of a MUC
Comment 7 Guillaume Desmottes 2013-08-13 15:03:53 UTC
Review of attachment 250929 [details] [review]:

::: libempathy-gtk/empathy-individual-store-channel.c
@@ +262,3 @@
+individual_store_channel_contact_chat_state_changed (TpTextChannel *channel,
+    TpContact *tp_contact,
+  if (typing_state == TP_CHANNEL_CHAT_STATE_COMPOSING)

TpChannelChatState

@@ +263,3 @@
+    TpContact *tp_contact,
+    guint state,
+  if (typing_state == TP_CHANNEL_CHAT_STATE_COMPOSING)

You can use 'EmpathyIndividualStoreChannel *self' directly.

@@ +271,3 @@
+  if (! (state == TP_CHANNEL_CHAT_STATE_COMPOSING ||
+         state == TP_CHANNEL_CHAT_STATE_PAUSED ||
+            GTK_ICON_SIZE_MENU);

you can simplify this logic expression:
if (! (a || b || c)) <=> if (!a && !b && !c)

@@ +272,3 @@
+         state == TP_CHANNEL_CHAT_STATE_PAUSED ||
+         state == TP_CHANNEL_CHAT_STATE_ACTIVE))
+

Actually shouldn't we always reset the pixbuf if the state is != COMPOSING?

@@ +277,3 @@
+  if (empathy_contact_is_user (contact))
+    {
+      g_free (icon_filename);

contact is not urefed if we don't enter in this block.

@@ +287,3 @@
+  self = EMPATHY_INDIVIDUAL_STORE_CHANNEL (user_data);
+  individual = g_hash_table_lookup (self->priv->individuals, tp_contact);
+           EMPATHY_INDIVIDUAL_STORE (model), individual);

I'd use a g_warning() instead to be a bit safer from crashes.

@@ +292,3 @@
+      GUINT_TO_POINTER (state));
+
+  gtk_tree_store_set (GTK_TREE_STORE (model), iter,

Couldn't you use individual_store_find_contact (make it public) and so save you from this foreach function and set_data()?

@@ +308,3 @@
+    chain_up (object);
+
+static void

Use tp_g_signal_connect_object

@@ +310,3 @@
+  g_signal_connect (self->priv->channel, "contact-chat-state-changed",
+      G_CALLBACK (individual_store_channel_contact_chat_state_changed),
+individual_store_channel_contact_chat_state_changed (TpTextChannel *channel,

Can't we do that in individual_store_channel_set_individual_channel ?
Comment 8 Chandni Verma 2013-08-14 08:39:22 UTC
(In reply to comment #7)
> Review of attachment 250929 [details] [review]:
> 
> ::: libempathy-gtk/empathy-individual-store-channel.c
> @@ +262,3 @@
> +individual_store_channel_contact_chat_state_changed (TpTextChannel *channel,
> +    TpContact *tp_contact,
> +  if (typing_state == TP_CHANNEL_CHAT_STATE_COMPOSING)
> 
> TpChannelChatState
> 
> @@ +263,3 @@
> +    TpContact *tp_contact,
> +    guint state,
> +  if (typing_state == TP_CHANNEL_CHAT_STATE_COMPOSING)
> 
> You can use 'EmpathyIndividualStoreChannel *self' directly.
> 
> @@ +271,3 @@
> +  if (! (state == TP_CHANNEL_CHAT_STATE_COMPOSING ||
> +         state == TP_CHANNEL_CHAT_STATE_PAUSED ||
> +            GTK_ICON_SIZE_MENU);
> 
> you can simplify this logic expression:
> if (! (a || b || c)) <=> if (!a && !b && !c)
> 

Done!

> @@ +272,3 @@
> +         state == TP_CHANNEL_CHAT_STATE_PAUSED ||
> +         state == TP_CHANNEL_CHAT_STATE_ACTIVE))
> +
> 
> Actually shouldn't we always reset the pixbuf if the state is != COMPOSING?
> 

We don't want to cause any updation on GONE or INACTIVE events uselessly. We just bother about when a user stops typing (PAUSED) or enters text (ACTIVE). Still I have removed the filter. :)

> @@ +277,3 @@
> +  if (empathy_contact_is_user (contact))
> +    {
> +      g_free (icon_filename);
> 
> contact is not urefed if we don't enter in this block.
> 
corrected!

> @@ +287,3 @@
> +  self = EMPATHY_INDIVIDUAL_STORE_CHANNEL (user_data);
> +  individual = g_hash_table_lookup (self->priv->individuals, tp_contact);
> +           EMPATHY_INDIVIDUAL_STORE (model), individual);
> 
> I'd use a g_warning() instead to be a bit safer from crashes.
> 
Done.

> @@ +292,3 @@
> +      GUINT_TO_POINTER (state));
> +
> +  gtk_tree_store_set (GTK_TREE_STORE (model), iter,
> 
> Couldn't you use individual_store_find_contact (make it public) and so save you
> from this foreach function and set_data()?
> 
Done.

> @@ +308,3 @@
> +    chain_up (object);
> +
> +static void
> 
> Use tp_g_signal_connect_object
> 
> @@ +310,3 @@
> +  g_signal_connect (self->priv->channel, "contact-chat-state-changed",
> +      G_CALLBACK (individual_store_channel_contact_chat_state_changed),
> +individual_store_channel_contact_chat_state_changed (TpTextChannel *channel,
> 
> Can't we do that in individual_store_channel_set_individual_channel ?

Done!
Patches follow.
Comment 9 Chandni Verma 2013-08-14 08:40:29 UTC
Created attachment 251577 [details] [review]
Make individual_store_find_contact and free_iters public
Comment 10 Chandni Verma 2013-08-14 08:40:35 UTC
Created attachment 251578 [details] [review]
Show typing icon against composing members of a MUC
Comment 11 Guillaume Desmottes 2013-08-14 10:37:04 UTC
Review of attachment 251577 [details] [review]:

Feel free to merge right away with these changes.

::: libempathy-gtk/empathy-individual-store.c
@@ +200,3 @@
 }
 
+GList *

Please add a comment:
/* (transfer full) free with empathy_individual_store_free_iters() */

::: libempathy-gtk/empathy-individual-store.h
@@ -144,2 +144,3 @@
     FolksIndividual *individual);
 
+GList *empathy_individual_store_find_contact (EmpathyIndividualStore *self,

Please put those below the '/* protected */' commment, only sub-classes should use those functions.
Comment 12 Guillaume Desmottes 2013-08-14 10:39:34 UTC
Review of attachment 251578 [details] [review]:

::: libempathy-gtk/empathy-individual-store-channel.c
@@ +159,3 @@
+  for (l = iters; l != NULL; l = l->next)
+    {
+    }

you can move the if/else block outside of the for loop so it will be done just once.
The store will keep a ref each time we call _set() so it's fine re-using the same pixbuf more than once I think (but please test).
Comment 13 Chandni Verma 2013-08-14 12:24:46 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.