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 707885 - need to escape spaces in action names
need to escape spaces in action names
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
: 707465 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-09-10 22:46 UTC by Matthias Clasen
Modified: 2013-09-25 13:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Create one single GAction to join rooms (4.90 KB, patch)
2013-09-11 09:13 UTC, Guillaume Desmottes
needs-work Details | Review
Create one single GAction to join rooms (4.83 KB, patch)
2013-09-11 14:42 UTC, Guillaume Desmottes
reviewed Details | Review
menu_removed_cb: call g_menu_model_get_n_items() once (1014 bytes, patch)
2013-09-11 14:42 UTC, Guillaume Desmottes
committed Details | Review
menu_removed_cb: fix 'name' leak (1.43 KB, patch)
2013-09-11 14:42 UTC, Guillaume Desmottes
committed Details | Review
use (room-name, account-path) as primary key when removing GMenuItem (2.24 KB, patch)
2013-09-11 14:42 UTC, Guillaume Desmottes
committed Details | Review

Description Matthias Clasen 2013-09-10 22:46:02 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=1004301 has a stacktrace of empathy running into a g_error in GMenu, because win.Join-Fedora Linux is not a valid detailed action name. Spaces are not allowed, you'll have to escape that.
Comment 1 Allison Karlitskaya (desrt) 2013-09-10 22:54:03 UTC
The practice of encoding arbitrary data in action names is probably a result of a direct port from GtkAction (where this was required).  In the GAction world, this is what action parameters are for...

The action should be called "win.join" with a parameter of type "s" which is the name of the room to join...
Comment 2 Igor Gnatenko 2013-09-11 05:28:21 UTC
*** Bug 707465 has been marked as a duplicate of this bug. ***
Comment 3 Guillaume Desmottes 2013-09-11 09:13:01 UTC
Created attachment 254655 [details] [review]
Create one single GAction to join rooms

The proper way to handle multi menu items having the same semantic is to have
one single GAction associated with different GMenuItem having specific
parameters instead of having one action per menu item.

Fix bgo#707885 as we no longer have spaces in GMenuItem action names.
Comment 4 Guillaume Desmottes 2013-09-11 09:13:41 UTC
Ryan: does this patch look good to you from a GAction pov?
Comment 5 Allison Karlitskaya (desrt) 2013-09-11 14:12:08 UTC
Review of attachment 254655 [details] [review]:

General idea is correct.  A couple small issues remain, though.

::: src/empathy-roster-window.c
@@ +1314,3 @@
 
+static void
+add_join_action (EmpathyRosterWindow *self)

Nuke this function and add "join" to your GActionEntry list instead.  The third field in that struct is the parameter type, as a string, so you'd just have: 

 { "join", roster_window_join_chatroom_menu_activate_cb, "(ss)" },


But note that you'll have to change the third parameter of the above function to 'gpointer user_data' to avoid the type error (or cast).

@@ +1318,3 @@
+  GAction *action;
+
+  action = (GAction *) g_simple_action_new ("join",

This will leak the type.  Use G_VARIANT_TYPE("(ss)") instead.

@@ +1352,1 @@
   g_menu_item_set_attribute (item, "room-name", "s", name);

This is kinda weird.  I assume you use this to lookup and remove items later?

@@ +1375,2 @@
   for (i = 0; i < g_menu_model_get_n_items (
         G_MENU_MODEL (self->priv->rooms_section)); i++)

Slightly unrelated to this patch, but this code for removing the item from the menu has a number of problems:

 1) calling _get_n_items() on each iteration is inefficient
 2) same with constantly casting the section to G_MENU_MODEL over and over
 3) you're leaking the attribute that you retrieve from the model
 4) the room name (as seen by the user) probably makes a bad unique key for this process.  What if the user has two rooms with the same display name?
Comment 6 Guillaume Desmottes 2013-09-11 14:42:09 UTC
(In reply to comment #5)
> ::: src/empathy-roster-window.c
> @@ +1314,3 @@
> 
> +static void
> +add_join_action (EmpathyRosterWindow *self)
> 
> Nuke this function and add "join" to your GActionEntry list instead.  The third
> field in that struct is the parameter type, as a string, so you'd just have: 
> 
>  { "join", roster_window_join_chatroom_menu_activate_cb, "(ss)" },

> But note that you'll have to change the third parameter of the above function
> to 'gpointer user_data' to avoid the type error (or cast).

Ah great, thanks.
 
> @@ +1318,3 @@
> +  GAction *action;
> +
> +  action = (GAction *) g_simple_action_new ("join",
> 
> This will leak the type.  Use G_VARIANT_TYPE("(ss)") instead.

Removed.

> @@ +1352,1 @@
>    g_menu_item_set_attribute (item, "room-name", "s", name);
> 
> This is kinda weird.  I assume you use this to lookup and remove items later?

Yeah.

> @@ +1375,2 @@
>    for (i = 0; i < g_menu_model_get_n_items (
>          G_MENU_MODEL (self->priv->rooms_section)); i++)
> 
> Slightly unrelated to this patch, but this code for removing the item from the
> menu has a number of problems:
> 
>  1) calling _get_n_items() on each iteration is inefficient
>  2) same with constantly casting the section to G_MENU_MODEL over and over
fixed.

>  3) you're leaking the attribute that you retrieve from the model

good catch; fixed.

>  4) the room name (as seen by the user) probably makes a bad unique key for
> this process.  What if the user has two rooms with the same display name?

Good point, we need to use (room-name, tp-account-path) as primary key.

Thanks for all your reviewing, this code has been written quickly when porting from GtkAction and hasn't been touched since.
Comment 7 Guillaume Desmottes 2013-09-11 14:42:31 UTC
Created attachment 254688 [details] [review]
Create one single GAction to join rooms

The proper way to handle multi menu items having the same semantic is to have
one single GAction associated with different GMenuItem having specific
parameters instead of having one action per menu item.

Fix bgo#707885 as we no longer have spaces in GMenuItem action names.
Comment 8 Guillaume Desmottes 2013-09-11 14:42:34 UTC
Created attachment 254689 [details] [review]
menu_removed_cb: call g_menu_model_get_n_items() once

No need to call it during each iteration.
Comment 9 Guillaume Desmottes 2013-09-11 14:42:38 UTC
Created attachment 254690 [details] [review]
menu_removed_cb: fix 'name' leak
Comment 10 Guillaume Desmottes 2013-09-11 14:42:41 UTC
Created attachment 254691 [details] [review]
use (room-name, account-path) as primary key when removing GMenuItem
Comment 11 Igor Gnatenko 2013-09-16 20:32:03 UTC
Review of attachment 254688 [details] [review]:

Looks good to me.
Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Comment 12 Igor Gnatenko 2013-09-16 20:32:19 UTC
Review of attachment 254689 [details] [review]:

Looks good to me.
Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Comment 13 Igor Gnatenko 2013-09-16 20:32:28 UTC
Review of attachment 254690 [details] [review]:

Looks good to me.
Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Comment 14 Igor Gnatenko 2013-09-16 20:32:45 UTC
Review of attachment 254691 [details] [review]:

Looks good to me.
Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Comment 15 Igor Gnatenko 2013-09-17 06:20:27 UTC
hmm. After ~12h empathy-chat crashed again :(

I can't reproduce yet. But it crashed w/ SIGSEGV.
Comment 16 Igor Gnatenko 2013-09-17 06:51:03 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=1005033
Comment 17 Igor Gnatenko 2013-09-18 18:04:05 UTC
this backtrace when conference window crashed, but I have this problem before and after patches.

  • #0 empathy_contact_dup_from_tp_contact
    at empathy-contact.c line 1779
  • #1 empathy_contact_dup_from_folks_individual
    at empathy-utils.c line 512
  • #2 individual_store_contact_sort
    at empathy-individual-store.c line 1117
  • #3 individual_store_name_sort_func
    at empathy-individual-store.c line 1242
  • #4 gtk_tree_store_sort_iter_changed
    from /lib64/libgtk-3.so.0
  • #5 gtk_tree_store_insert_with_values
    from /lib64/libgtk-3.so.0
  • #6 add_individual_to_store
    at empathy-individual-store.c line 110
  • #7 empathy_individual_store_add_individual
    at empathy-individual-store.c line 294
  • #8 individual_store_add_individual_and_connect
    at empathy-individual-store.c line 811
  • #9 add_members
    at empathy-individual-store-channel.c line 80
  • #10 ffi_call_unix64
    from /lib64/libffi.so.6
  • #11 ffi_call
    from /lib64/libffi.so.6
  • #12 g_cclosure_marshal_generic
    from /lib64/libgobject-2.0.so.0
  • #13 g_closure_invoke
    from /lib64/libgobject-2.0.so.0
  • #14 signal_emit_unlocked_R
    from /lib64/libgobject-2.0.so.0
  • #15 g_signal_emit_valist
    from /lib64/libgobject-2.0.so.0
  • #16 g_signal_emit_by_name
    from /lib64/libgobject-2.0.so.0
  • #17 members_changed_prepared_cb
    from /lib64/libtelepathy-glib.so.0
  • #18 g_simple_async_result_complete
    from /lib64/libgio-2.0.so.0
  • #19 contacts_queue_head_ready
    from /lib64/libtelepathy-glib.so.0
  • #20 connection_capabilities_fetched_cb
    from /lib64/libtelepathy-glib.so.0
  • #21 g_simple_async_result_complete
    from /lib64/libgio-2.0.so.0
  • #22 complete_in_idle_cb
    from /lib64/libgio-2.0.so.0
  • #23 g_main_context_dispatch
    from /lib64/libglib-2.0.so.0
  • #24 g_main_context_iterate.isra.23
    from /lib64/libglib-2.0.so.0
  • #25 g_main_context_iteration
    from /lib64/libglib-2.0.so.0
  • #26 g_application_run
    from /lib64/libgio-2.0.so.0
  • #27 main
    at empathy-chat.c line 158

Comment 18 Guillaume Desmottes 2013-09-25 13:20:54 UTC
(In reply to comment #17)
> this backtrace when conference window crashed, but I have this problem before
> and after patches.


Please open another bug with this trace if you can reproduce with 3.10.
Comment 19 Igor Gnatenko 2013-09-25 13:22:25 UTC
(In reply to comment #18)
> (In reply to comment #17)
> > this backtrace when conference window crashed, but I have this problem before
> > and after patches.
> 
> 
> Please open another bug with this trace if you can reproduce with 3.10.
Sure. Opened:
https://bugzilla.gnome.org/show_bug.cgi?id=708471
https://bugzilla.gnome.org/show_bug.cgi?id=708450
About this 4 patch-set.. It's fixed this bug.
Comment 20 Guillaume Desmottes 2013-09-25 13:29:44 UTC
Attachment 254689 [details] pushed as ff897a9 - menu_removed_cb: call g_menu_model_get_n_items() once
Attachment 254690 [details] pushed as a0602d0 - menu_removed_cb: fix 'name' leak
Attachment 254691 [details] pushed as f4ebc8f - use (room-name, account-path) as primary key when removing GMenuItem