GNOME Bugzilla – Bug 707885
need to escape spaces in action names
Last modified: 2013-09-25 13:29:56 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.
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...
*** Bug 707465 has been marked as a duplicate of this bug. ***
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.
Ryan: does this patch look good to you from a GAction pov?
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?
(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.
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.
Created attachment 254689 [details] [review] menu_removed_cb: call g_menu_model_get_n_items() once No need to call it during each iteration.
Created attachment 254690 [details] [review] menu_removed_cb: fix 'name' leak
Created attachment 254691 [details] [review] use (room-name, account-path) as primary key when removing GMenuItem
Review of attachment 254688 [details] [review]: Looks good to me. Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Review of attachment 254689 [details] [review]: Looks good to me. Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Review of attachment 254690 [details] [review]: Looks good to me. Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Review of attachment 254691 [details] [review]: Looks good to me. Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
hmm. After ~12h empathy-chat crashed again :( I can't reproduce yet. But it crashed w/ SIGSEGV.
https://bugzilla.redhat.com/show_bug.cgi?id=1005033
this backtrace when conference window crashed, but I have this problem before and after patches.
+ Trace 232499
(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.
(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.
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