GNOME Bugzilla – Bug 319859
"open image with" function
Last modified: 2008-04-25 16:20:33 UTC
A "open image with" menu item or something would really come in handy. Use Case: I often browse my images with nautilus, open it with eog to see if I got the right one and then I want to open it with the Gimp for example. At the moment I have to close EOG, search for the right image in nautilus again, then right click and say "open with Gimp". This would be really nice!
I agree, this could in deed be very useful. What would be _really_ cool is if you got the same "Open With" dialog as right-clicking the image in nautilus. Otherwise, you'd have to check for the different applications.
not sure how you plan on implementing this but you might consider doing things a bit like how Adobe Photoshop does it by having File, Jump to, App name Other appname The question of integrating with other applications could be handled in other ways, like maybe adding an option File Browse... to open up a program like Gthumb, or maybe just the existing thumbnail view. (Haven't thought these ideas out but I figured I'd quickly mention them and let you decided how you thought was best to implement it)
I would suggest to make it just a simple button in the toolbar and a menu item under 'File'. The dialog I would expect would contain a simple list, that provides the same applications as nauilus does for the image type. Just my 2c!
Well, I'm sure EOG won't have a toolbar item for this. :-) IMHO, there would be only a "Open with" submenu (under 'File' menu) for easy access to the apps that can handle that image type.
*** Bug 302080 has been marked as a duplicate of this bug. ***
I would suggest to add a context menue, so right-click on the image gives the "open with" options (like in nautilus)... Additionally, of course, a submenue like comment4 from Lucas Rocha would be needed.
*** Bug 353174 has been marked as a duplicate of this bug. ***
Could someone with permissions assing this bug to me?
(In reply to comment #8) > Could someone with permissions assing this bug to me? > Done.
Created attachment 78705 [details] [review] first implementation patch The code add a open with menu under 'File' and a context menu with the applications installed in the system that can handle the myme type of the current image. Anyway, i have a doubt passing more than one image to an external program. What if the user select a jpg and a svg image?. The program have to add to the menu only the applications that support both myme types?.
Hey Rodrigo, Cool that you have your first patch, thanks a lot! However, there are some problems with the patch, which I couldn't test yet. You need to set GLists and GSLists to NULL before adding contents to them, otherwise you risk segmentation faults when freeing them and other undesired behaviors. I tried testing your patch, but it crashes upon startup. Please double check you are using the GLists in the right way, it seems to be the problem. Also, please check the naming convention for the functions you add. Instead of eog_myfunction(), you need to name your functions eog_window_myfunction(), or simply myfunction() (The former 'always' for public methods, the latter is fine for convenience functions).
Created attachment 78718 [details] [review] polished patch Hi Claudio, Thanks for the quick answer. I made the changes that you suggest. I tested this patching a fresh copy of the cvs branch. I hope this version work's better. Tanks in advance
Just from a quick look you could try to use gnome_vfs_mime_get_all_applications_for_uri() instead of gnome_vfs_mime_get_all_applications(). The advantage is that it additionally filters out applications which cannot open remote uris (if you supply it with a remote uri). Also the application list should be freed with gnome_vfs_mime_applications_list_free().
Created attachment 78755 [details] screenshot I try with gnome_vfs_mime_applications_list_free() but the program crash. It works with g_list_free(). Don't know why. gnome_vfs_mime_get_all_applications_for_uri() works very well. But, somebody was able to make the patch work? . It works on my system (Ubuntu edgy) and i need to know if the patch works on other system. Thanks
Rodrigo, Your patch works, I haven't tested it exhaustively, but it does. It's good, but it needs some polishing. I'll give you some hints: 1. You don't need to set GList iterators to NULL, nor free them. You only do that for the persistent pointer to the list. 2. After freeing open_with_menu_ids, it's a good idea to set it to NULL. 3. gnome_vfs_get_mime_type (gnome_vfs_uri_to_string (...)) leaks the string. You can either fix the leak, or use a shortcut: gnome_vfs_get_mime_type_from_uri (). The problem with the latter is that the mimetype will be guessed from the filename and not from the file contents. 4. gnome_vfs_get_mime_type () returns a const gchar*. That means you don't have to free the returned string. 5. Check if you really need to set apps to NULL before calling gnome_vfs_mime_get_all_applications (). 6. If you followed my advice on (2), then don't set open_with_menu_ids to NULL before beginning to fill it with the menu identifiers. 7. The check to avoid adding eog to the menu is a bit tricky. The desktop entry specification[1] indicates that the command in the "exec" field could even include a full path. Moreover, if there is an application called 'eogito' associated with the mimetype, then you won't add it either. Even worse, you can't guess that the executable will be called 'eog' at all. I think this may or may not be a really important issue, but it is definitively not that trivial to solve. Maybe you can check how gthumb solves this. 8. I am not quite sure if passing the uri as the user data for the "activate" callback in the actions is the best idea. I have my doubts because of the lifetime of the URI, though I can't say it's wrong. 9. Use g_list_prepend () instead of g_list_append () (it's more efficient and the order you store the ids is not important). 10. You need to use gnome_vfs_mime_application_list_free to free the list of applications. g_list_free () only frees the list itself, and doesn't care if the elements in the list were allocated or not. 11. In the launch application callback, remember that gnome_vfs_uri_to_string () allocates memory. You should free the returned string before freeing the list. 12. I am not sure if you should unref the uri inside the callback. 13. I like the tips you added to the actions but as long as the other menues are not using tips, it's better not to use them (this could be a 'gnome-love' bug?). 14. I like more to use iter than scan as a name for GList iterators. Please don't get discouraged by this long list. All of the suggestions (besides (7), of course) are pretty small improvements and details. I think your patch is very good! Keep the good work and thanks for your help!
Created attachment 79445 [details] [review] new patch Hi Claudio Thanks so much for your help. I made almost all the changes that you suggest but i have some trouble with three of them. Following your list: 3. I try with gnome_vfs_get_mime_type_from_uri () but the program crash. I don't understand so much what do you mean with leak string. Can you give me some more information or hints about how to fix the leak?. 7. I agree with you. It's tricky but it's almost a copy from the code of gthumb. Don't know what parameter or info i can use to avoid the problems that you list in your comment. 10. If i use gnome_vfs_mime_application_list_free the program crash. I take the idea from gthumb code and they do the same. Gthumb use g_list_free to free a list of GnomeVFSMimeApplication. I think I'm gonna have to ask to the developers of gthumb about the issue. I attach a copy of the polished patch that i test on my system and works very well. I hope to have made the changes well.
> 3. I try with gnome_vfs_get_mime_type_from_uri () but the program crash. I > don't understand so much what do you mean with leak string. Can you give me > some more information or hints about how to fix the leak?. gnome_vfs_uri_to_string () returns a malloc'd string, which needs to be freed by means of g_free (). On the other hand, gnome_vfs_get_mime_type () needs a const gchar *, which means that it only uses the given string, but doesn't take care of freeing the memory. That means, that once gnome_vfs_get_mime_type () returns, the string stays in memory and no one takes care of freeing it. That's what's called a memory leak. To fix it, use an intermediate pointer, which you can free after using it. Sort of: gchar *str_uri = gnome_vfs_uri_to_string (...); gnome_vfs_get_mime_type (str_uri); g_free (str_uri);
Hey Rodrigo, how are you doing with this? If you need any help, don't hesitate to ask! You've done great so far :-)
Created attachment 81186 [details] ghtumb code Hey Claudio I'm sorry for my delay but I have been so busy at the office that I can't work to much on this. Anyway, I solved pretty much all the comments that you made to my code. I have troubles only with the point 7 and 10. I'm pretty much stocked with this because y wrote the code reading the gthumb code. In comment 7, I tried to find other field in the GnomeVFSMimeApplication structure that I can use but I don't see nothing that can help me to solve this. In comment 10, I don't know why the program crash with the function gnome_vfs_mime_application_list_free. It is supposed to work according the documentation but the program crash. Again, the gthumb developers use g_list_free instead gnome_vfs_mime_application_list_free. Today I'm going to ask in the gthumb list about this issues. I attached the code of gthumb that inspires me.
Hi Rodrigo, (In reply to comment #19) > I'm sorry for my delay but I have been so busy at the office that I can't work > to much on this. It's completely fine, dude. Thanks so much for having written such a good patch and for having worked on improving it. I took the freedom to hack on your patch and fix the very few remaining issues I could find. I'll attach both your patch synchronized with svn and the new one, so you can diff'em and see the differences, in case you want to take a look to the details. > In comment 7, I tried to find other field in the GnomeVFSMimeApplication > structure that I can use but I don't see nothing that can help me to solve > this. I fixed this by using gnome_vfs_mime_application_get_binary_name () and g_get_prgname (). The former returns the binary name of the associated application (p.g. "eog") and the latter returns the name of the executable running. Comparing them looks like a much saner approach than the one in gthumb (we should even write a patch for gthumb). > > In comment 10, I don't know why the program crash with the function > gnome_vfs_mime_application_list_free. It is supposed to work according the > documentation but the program crash. Again, the gthumb developers use > g_list_free instead gnome_vfs_mime_application_list_free. Ok, I overlooked your patch and completely ignored we would later need the GnomeVFSMimeApplication pointers. When using gnome_vfs_mime_application_list_free (), we wouldn't be able to find later in open_with_launch_application_callback () the GnomeVFSMimeApplication needed to actually open the file. I solved this by using g_object_set_data_full () to attach the GnomeVFSMimeApplication to the GtkAction and tell GObject to free the GnomeVFSMimeApplication using gnome_vfs_mime_application_free () when we don't need it anymore. Also by immediately freeing the GnomeVFSMimeApplication related to eog. I'll double check the new patch and attach it here later. Once again, thank you Rodrigo!
Created attachment 82303 [details] [review] patch-open-image-with-2007-01-05-raguilar-sync.diff Patch by Rodrigo Aguilar synch'ed with current SVN.
Created attachment 82304 [details] [review] Rodrigo's patch updated. Updated patch. Summary of changes: - Minor style fixes (spacing, callback names, etc). - Free unref uri in open_with_application_callback. - Use EOG_IMAGE macro instead of (EogImage *). - If mimetype == NULL the uri is not unrefered. Unrefer it before the if block. - Fix the leaked uri string. - Use gnome_vfs_mime_application_get_binary_name () and g_get_prgname () to avoid hardcoding the executable name.
(In reply to comment #22) > Created an attachment (id=82304) [edit] > Rodrigo's patch updated. > > Updated patch. Summary of changes: > > - Minor style fixes (spacing, callback names, etc). > - Free unref uri in open_with_application_callback. > - Use EOG_IMAGE macro instead of (EogImage *). > - If mimetype == NULL the uri is not unrefered. Unrefer it before the if block. > - Fix the leaked uri string. > - Use gnome_vfs_mime_application_get_binary_name () and g_get_prgname () > to avoid hardcoding the executable name. Oh, and I forgot: - Do not set the tip yet, because no other action uses it. - Do not leak the GnomeVFSMimeApplication pointers.
There are two things I noticed: - As far as I know you don't need a new merge_id for every action. You should be able to merge all actions under the same merge_id and would have to request one only once every time you deleted the old menu. And it would save the cost to manage the list. - Although your approach to handle the GVFSMimeApplication objects looks pretty good so far it is I think not yet fully correct. The problem is that the actions are not freed/released/unreffed so the GDestroyNotify part doesn't actually run. But I still need to check that with a Valgrind run.
Created attachment 82357 [details] [review] patch updated (In reply to comment #24) > There are two things I noticed: > > - As far as I know you don't need a new merge_id for every action. You should > be able to merge all actions under the same merge_id and would have to request > one only once every time you deleted the old menu. And it would save the cost > to manage the list. That's right. I removed the list and instead stored a single merge_id. > > - Although your approach to handle the GVFSMimeApplication objects looks pretty > good so far it is I think not yet fully correct. The problem is that the > actions are not freed/released/unreffed so the GDestroyNotify part doesn't > actually run. But I still need to check that with a Valgrind run. > That's correct, too. I ran eog under GDB and breakpointed the calls to gnome_vfs_mime_application_free () and it's only called on the explicit call used to free the GVFSMimeApp related to eog. I changed the approach and now maintained the list of GFVFSMimeApps (it's not that expensive) and used gnome_vfs_mime_application_list_free () to free it when needed. This version also fixes a leak.
I think the patch is good enough to go in. If there were any pending issues we can work on them in SVN. Thank you Rodrigo for your contribution! Committed to the eog-ng branch (rev. 3600). Closing as this can't and won't be fixed in trunk. 2007-02-27 Claudio Saavedra <csaavedra@alumnos.utalca.cl> * data/eog-ui.xml: Add a 'OpenImageWith' menu item. * src/eog-window.c: (eog_window_display_image): Update the 'open with' menu when displaying an image. (+open_with_launch_application_cb): Call the chosen application with the selected image as parameter. (+eog_window_update_openwith_menu): Updates the 'open with' menu according with the mimetypes of the selected image. (eog_window_construct_ui): Get a new merge id for the menu. (eog_window_init), (eog_window_dispose): Properly init and finish the mime application list. Add a 'Open file with' functionality (Fixes bug #319859). Patch from Rodrigo Aguilar <raguilar@iee.ufro.cl> and me.
Jus for the record: great job guys!
*** Bug 440681 has been marked as a duplicate of this bug. ***
*** Bug 529919 has been marked as a duplicate of this bug. ***