GNOME Bugzilla – Bug 508878
Nautilus extension submenu is not activated
Last modified: 2008-06-09 21:12:52 UTC
Please describe the problem: I'm making nautilus extension which creates submenu feature. The menu itself is shown as a submenu currectly, but when I click the submenu item, it is not activated. Steps to reproduce: 1. install some extension which provides submenu. 2. select the submenu item. 3. the selected menu item will not activated. Actual results: Nothing. Expected results: The selected menu item should be activated. Does this happen every time? Always. Other information: This bug happens because nautilus checks whether it has the extension menu or not, before to activate a extension menu item, in the function extension_action_slow_mime_types_ready_callback(), but it doesn't find the menu item. Nautilus only searches the extension menu list, not submenus. So it thinks the menu item is not valid one. The extension menu validating code should search the submenus, also.
Created attachment 102639 [details] [review] a patch to search submenu items recursively I made a patch for this problem. This patch makes the nautilus search the submenu items recursively to check whether the menu item is valid or not.
can you also bring this patch up at nautilus-list, please? http://mail.gnome.org/mailman/listinfo/nautilus-list
Commited.
This patch does not fix the problem. This bug should be reopened.
Rlan, A test case is appreciated. For any chance, have you tried to open a submenu over the background or over a group of files/folders? As fas as I tried (using nautilus-python): - get_file_items() shows submenus right. - get_background_items() doesn't show submenus. I haven't reopened this bug, because I wanted to determine where the problem is located: nautilus or nautilus-python (see bug 530845).
I'm reopening this bug, because the patch solves the problem partially. It works for iface->get_file_items, but it doesn't for iface->get_background_items.
Hi German The problem isn't that the submenus are shown correctly. The problem is that callbacks registered on the "activate" signal of NautilusMenuItem objects that are under submenus are not called. I am working on presenting a test case to clarify the issue. Thanks for attending to this Rian
Rian, I see. So, we have two issues then :-) However, I have a test case where submenus are not shown when get_background_items is called. I'm cleaning the code before upload it.
Created attachment 112340 [details] Test case to reproduce the issues Test case that shows: * Submenus aren't shown when iface->get_background_items it called * Submenus shown aren't activated (iface->get_file_items)
Created attachment 112341 [details] Screenshot when get_file_items is called This screenshot show how menu works using libnautilus-extension. The submenu is shown properly, but it is not activated. A simple menu is shown and it works as expected.
Created attachment 112343 [details] Screenshot when get_background_items is called This screenshot show how menu works using libnautilus-extension. The submenu is not shown properly, only the main one is shown. A simple menu is shown and it works as expected. The code to show the menu is exactly the same as the implemented for get_file_items.
Created attachment 112350 [details] [review] Fix the submenu activation Apparently the proposed patch wasn't applied verbatim and a typo was introduced. The patch applied can be viewed at http://svn.gnome.org/viewvc/nautilus/branches/gnome-2-22/src/file-manager/fm-directory-view.c?r1=13586&r2=13596 The recursive calls to search_in_menu_items is done with the wrong variable. Evenmore, it is called with a variable which was freed previously. So, the menu never matches with the action name requested. This patch fix that typo. May I apply it? (with a ChangeLog entry, of course).
Created attachment 112354 [details] [review] Patch to fix submenu items recursively The menus created with ::get_background_items are created by nautilus_window_load_extension_menus, which didn't have any code supporting submenus. I took, as template, the function add_extension_menu_items used by reset_extension_actions_menu to create menus from ::get_file_items. I tried this patch and fixes the issues with submenus. I tried also, a Python plugin using submenus without any problem.
just seen your patch on the ml. Quick note, code looks clean but you could replace this bit: + for (l = items; l != NULL; l = l->next) { + g_object_unref (l->data); + } by a simple call to g_list_foreach
Dear Germán, thanks for your debugging efforts and your patch. From skimming through the patch, I think there is one problem: The UI manager menu paths might not reflect the actual widget hierarchy: add_extension_menu_items () is called recursively with subdir = g_build_path ("/", subdirectory, gtk_action_get_name (action), NULL); For a layout like "foo" "barfoo" "bar" -> "foo" "bar" -> "foobar" [where -> denotes a submenu / submenu item relation] We'd end up with the following menu paths: "MENU_PATH_EXTENSION_ACTIONS/foo" "MENU_PATH_EXTENSION_ACTIONS/barfoo" "MENU_PATH_EXTENSION_ACTIONS/bar" "MENU_PATH_EXTENSION_ACTIONS/barfoo" IMO, the recursive sub directory should be constructed like subdir = g_build_path ("/", subdirectory, "/", gtk_action_get_name (action), NULL); This way we'd have the menu paths "MENU_PATH_EXTENSION_ACTIONS/foo" "MENU_PATH_EXTENSION_ACTIONS/barfoo" "MENU_PATH_EXTENSION_ACTIONS/bar" "MENU_PATH_EXTENSION_ACTIONS/bar/foo" Since you took reset_extension_actions_menu as a template, it probably has the same problem. Maybe you could rework the patch accordingly?
Dear Christian, (In reply to comment #15) > Dear Germán, > > thanks for your debugging efforts and your patch. > > From skimming through the patch, I think there is one problem: The UI manager > menu paths might not reflect the actual widget hierarchy: > > add_extension_menu_items () > > is called recursively with > > subdir = g_build_path ("/", subdirectory, gtk_action_get_name (action), NULL); > > For a layout like > > "foo" > "barfoo" > "bar" -> "foo" > "bar" -> "foobar" > > [where -> denotes a submenu / submenu item relation] > > We'd end up with the following menu paths: > > "MENU_PATH_EXTENSION_ACTIONS/foo" > "MENU_PATH_EXTENSION_ACTIONS/barfoo" > "MENU_PATH_EXTENSION_ACTIONS/bar" > "MENU_PATH_EXTENSION_ACTIONS/barfoo" > > IMO, the recursive sub directory should be constructed like > > subdir = g_build_path ("/", subdirectory, "/", > gtk_action_get_name (action), NULL); > > This way we'd have the menu paths > > "MENU_PATH_EXTENSION_ACTIONS/foo" > "MENU_PATH_EXTENSION_ACTIONS/barfoo" > "MENU_PATH_EXTENSION_ACTIONS/bar" > "MENU_PATH_EXTENSION_ACTIONS/bar/foo" > > Maybe you could rework the patch accordingly? Is there any chance you are confusing g_build_path() with g_strconcat()? Because, the first parameter of g_build_path is the separator between elements. And the elements are the remaining arguments. That is according to http://library.gnome.org/devel/glib/2.16/glib-Miscellaneous-Utility-Functions.html#g-build-path Anyway, and just to be sure, I tested it both ways and both produces the same results (having printing path and subdir properly). Also, I created a three-depth-level submenu (just to be sure). > Since you took reset_extension_actions_menu as a template, it probably has the > same problem. I tested in both of them.
Created attachment 112370 [details] [review] Patch to fix submenu items recursively Updated patch (using g_list_foreach instead of for iterations). The menus created with ::get_background_items are created by nautilus_window_load_extension_menus, which didn't have any code supporting submenus. I took, as template, the function add_extension_menu_items used by reset_extension_actions_menu to create menus from ::get_file_items. This patch has been tested with three levels of submenus.
(In reply to comment #14) > just seen your patch on the ml. Quick note, code looks clean but you could > replace this bit: > > + for (l = items; l != NULL; l = l->next) { > + g_object_unref (l->data); > + } > > by a simple call to g_list_foreach > I made the change in both functions. Thanks.
(in replay to comment #16): > Is there any chance you are confusing g_build_path() with g_strconcat()? Shame on me. Please commit the patch to trunk and to the GNOME 2.22 branch.
I committed the patch in both stable and trunk.
*** Bug 530845 has been marked as a duplicate of this bug. ***