GNOME Bugzilla – Bug 155119
NE: Open Link with... menu
Last modified: 2013-05-27 16:10:17 UTC
I have implemented this new extension feature to create and populate an Open Link with... menu in the EphyLinkPopup and EphyImageLinkPopup contextual menus. Applications are initially listed by examining the extension of the link. After that, an asynchronous GnomeVFS request (HTTP HEAD) is initiated to determine the real mime type. This results in the most appropiate results being added to the application menu. In a perfect world, the server's mime type should be authoritative. However, there are many real world cases where it is not (.wmv = text/html). So, I believe listing both the extension's and the server's mime type's associations, clearly seperated, is appropiate. Currently there are a few thigns I am unhappy with. I'd rather not use GnomeVFS to issue the HEAD request, but Mozilla itself. That would preserve cookies and work with Keep-Alives, giving it a much higher chance of suceeding. Additionally, I'd like to list the application's icons in the menu next to the applications, and the mime type icon in the menu next to the mime type heading. As soon as I figure out how, I will. ;0 Two peices of the code are rather unsavory: First, to work with GtkUIManager, I am constructing Actions on the fly. To minimize the chances of a colision of Action names, there is a random number appended to them. This is ugly, but it works currently. When the server's authoritative mime type is received duplicates in the list of extension based associations (short_apps) are removed. The code to do this is rather ugly. If there is a standard intersect method on two GLists, i'd love to hear about it. Also, it freezes Epiphany every now and then, but it doesn't look like my fault. it seems to be deep in Mozilla, dealing with rendering using the gdk backend. My guess is my manipulation of the context menu is somehow unsavory. Please review.
Created attachment 32485 [details] [review] OpenWith Extension Patch
Created attachment 32486 [details] [review] This one applies cleanly Redid the patch. It now applies cleanly to 1.4.
*** Bug 140662 has been marked as a duplicate of this bug. ***
+#define EPHY_OPENWITH_EXTENSION_GET_PRIVATE(object) (G_TYPE_INSTANCE_GET_PRIVATE ((object), EPHY_TYPE_OPENWITH_EXTENSION, EphyOpenWithExtensionPrivate)) +struct EphyOpenWithExtensionPrivate +{ + +}; +enum +{ + PROP_0 +}; + +static void +ephy_openwith_extension_finalize (GObject *object) +{ + LOG ("EphyOpenWithExtension finalizing"); + G_OBJECT_CLASS (openwith_extension_parent_class)->finalize (object); +} + +static void +ephy_openwith_extension_set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) +{ + +} + +static void +ephy_openwith_extension_get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) +{ + +} Unused, remove it? + argv[0] = g_strchomp (app->command); app->command is owned by the GnomeVFSMimeApplication and must not be modified, but g_strchomp does that. Why is this needed anyway, where does the extraneous whitespace come from? + if (error != NULL) { + dialog = gtk_message_dialog_new (NULL, That dialogue won't be HIGgy... just add a FIXME comment saying to use gtk+ hig alert API (which doesn't exist yet :). + /* our file */ + dest = ephy_embed_persist_get_dest (persist); + openwith_launch (data->app, dest); + + g_free (data); Who owns data->app, do you need to unref it ? Same thing in openwith_download_cancelled. +ephy_openwith_action In the !app-can-download-itself case, you connect openwith_download_completed/cancelled with user_data data, and those funcs g_free it; in the other case however you don't g_free it... + uri = gnome_vfs_uri_new (link); + uris = g_list_append (uris, (gpointer) uri); gnome_vfs_uri_new can (and will for non-supported schemes) return NULL; but you don't seem to handle that elsewhere. + /* generate a custom name for this action */ + /* hack to make sure the action names are unique */ + action_number = g_random_int_range (0, 65535); + action_name = g_strdup_printf ("%s_%d", app->id, action_number); + action_path = g_strdup_printf ("OpenWithExtAction_%s_%d", app->id, action_number); No no no :P Those really have to be unique, not just 'randomly unique'. Why not just add a counter in the for loop, and use that? + /* some additional information needed by the action when it is invoked */ + action_data = g_new (ActionData, 1); + action_data->state = data; + action_data->app = app; + action_data->uri = uri; + g_object_set_data_full (G_OBJECT (action), "action_data", action_data, (GDestroyNotify) g_free); + g_signal_connect (G_OBJECT (action), "activate", G_CALLBACK (ephy_openwith_action), action_data); Aha! So you _mustn't_ g_free that data in the completed/cancelled callbacks after all. Also, you could use g_signal_connect_data(). Again, who owns ->app and ->uri ? + /* add the new action's UI to the manager */ + gtk_ui_manager_add_ui (data->manager, data->merge_id, + "/EphyLinkPopup/OpenWithExtMenu", + action_path, action_name, + GTK_UI_MANAGER_MENUITEM, TRUE); Why add to the top? +/** + * Adds a insensitive menu item to the menu. The text is the full description + * of the mime type. + * Hmm, not really great UI... why not just set that as 'tooltip' property of the action, which will be displayed as help in statusbar? + /* clean up the window state */ + data->uri = NULL; That leaks the previous uri. + data->short_mime_type = NULL; + data->right_mime_type = NULL; Leaks too, I think. + right_apps = gnome_vfs_mime_get_all_applications (result->file_info->mime_type); + g_list_free (data->short_apps); + g_list_free (data->right_apps); + data->short_apps = NULL; + data->right_apps = NULL; Did you own a ref to the list items, or not? + data = g_new0 (WindowData, 1); + data->window = window; + data->manager = manager; + data->action_group = NULL; + data->merge_id = 0; No need to set = NULL or = 0 when you've g_new0'd the struct. ---- Should really use mozilla APIs to get the mime types from the server (nsIURIChecker is HEAD request, then you have to get the mime types from the channel when it completes).
Created attachment 32751 [details] [review] new patch with suggestions Okay. Patched up most of the problems you had. Most of the char's you said I didn't free were const... and I think I am using them correctly. I am missing some really big unref someplace which is preventing the ActionData function from being called. I have spent a large number of hours staring at this and haven't seen it yet. =/ Any help is welcome.
Comment on attachment 32751 [details] [review] new patch with suggestions Sorry I took so long to review this, it had totally fallen off my list :( +static void +impl_detach_window (EphyExtension *extension, EphyWindow *window) This doesn't only need to disconnect all handlers, it also needs to undo all effects those handlers may have caused. I.e. you need to run the tab_removed_cb for all tabs in the window. And set the WINDOW_DATA_KEY data on the window to NULL. +static void +impl_attach_window (EphyExtension *extension, EphyWindow *window) + /* EphyLinkPopup UI */ + gtk_ui_manager_add_ui (manager, merge_id, + "/EphyLinkPopup", + "OpenWithExtMenu", "OpenWithExtAction", + GTK_UI_MANAGER_MENU, TRUE); Seems this extension needs a placeholder, since this just makes it the first menu item but that is reserved for the default action, i.e. "Open". Or do you want to replace it (then you'll need to hide the ephy Open action) ? + data->window = window; + g_object_ref (window); + data->manager = manager; + g_object_ref (manager); Don't ref here (and then don't unref in openwith_free_window_data). If you _need_ to be sure to have a valid window, add a weak pointer which NULLs out the ->window member when the window dies. + data->action_group = NULL; + data->merge_id = 0; No need to do this, since you g_new0'd the data. + g_signal_connect_after (notebook, "tab_added", G_CALLBACK (tab_added_cb), NULL); + g_signal_connect_after (notebook, "tab_removed", G_CALLBACK (tab_removed_cb), NULL); +static void +tab_added_cb (GtkWidget *notebook, EphyTab *tab, gpointer *user_data) +static void +tab_removed_cb (GtkWidget *notebook, EphyTab *tab, gpointer *user_data) +static void +context_menu_cb (EphyEmbed *embed, EphyEmbedEvent *event) You could pass |window| as user_data instead of NULL. That way, you cann connect the |context_menu_cb| with window too, and don't need gtk_widget_get_toplevel there. Or maybe pass |extension|. +static void +context_menu_cb (EphyEmbed *embed, EphyEmbedEvent *event) context menu signal has return type boolean, you MUST return FALSE. + data = g_object_get_data (G_OBJECT (window), WINDOW_DATA_KEY); Just to be safe, add a |g_return_val_if_fail (data != NULL, FALSE)|. + /* clean up the window state */ + data->uri = NULL; but later: + uris = openwith_get_uri_list_from_event (event); + if (uris == NULL) return; + path = gnome_vfs_uri_get_path (uris->data); + data->uri = uris->data; data->uri is the result of a gnome_vfs_uri_new(), so you can't just set ->uri to NULL, you need to gnome_vfs_uri_unref() it first. ================================================= I've one suggestion: make the link checking independent of the window, and do it in the extension itself. That - removes unnecessary complexity -- you only have to clean up afterwards in one place, which isn't likely to go away, as a window is - you can keep a cache of recently checked links and their return types
Jerry, what's the status of your extension?
I ran out of time to work on it. I probably won't get back to it for awhile, if ever. It no longer works with the latest Epiphanys. Surely somebody wants to take up the torch!
I would like to say that the absence of this feature is my biggest issue with epiphany. There is no way to choose which app to open a link with.
*BUMP* could we have this back??
@Javier and Lexual: The biggest chance of gaining progress on this bug is if you clean up the patch and resubmit it, or find someone else to do it...
Yeah, sorry I dropped the ball on this. I got busy and other things took priority. Somebody please finish it, I still realize how much I want it from time to time.
According to its developer, epiphany-extensions is not under active development anymore. (For reference: https://mail.gnome.org/archives/gnome-i18n/2013-May/msg00035.html and bug 700924.) It is unlikely that there will be any further active development. Closing this report as WONTFIX as part of Bugzilla Housekeeping - Please feel free to reopen this bug report in the future if anyone takes the responsibility for active development again.