GNOME Bugzilla – Bug 271551
File Manager Used During Composer Should Remember Last Directory.
Last modified: 2009-01-05 21:28:43 UTC
When you are composing a message, and hit the Insert Image button and navigate and put the image in it should keep that directory in memory. If when composing the same message you want to insert another image, it loses the directory you drew the last one from, and you have to navigate all over again. In general, Evo doesn't really do this in a few places, and this is a common UI flow that people are used to.
when fixing this one please also fix the second issue of bug 213660, it's the same problem and the same code!
"when fixing this one please also fix the second issue of bug 213660, it's the same problem and the same code!" Not even close to the same code ...
insert->image is part of the gtkhtml editor component. the composer has its own file selectors and stores the path separately. i dont see any simple way to merge the two to share the same path.
Created attachment 97277 [details] [review] proposed gtkhtml patch for gtkhtml; I added there new interface functions, but I'm not sure if I should change interface number or something, to be sure it will not crash in runtime when evo will have wrong gtkhtml installed.
Created attachment 97278 [details] [review] proposed evo patch for evolution; evolution's part of the patch.
Milan, sure that this works for you? I tried 1. Open composer in html mode 2. Insert image from non-home dir 3. send email 4. Open composer in html mode 5. Click insert image (It opens the file chooser in home dir and not the last selected dir)
Created attachment 98324 [details] [review] proposed gtkhtml patch ][ for gtkhtml; So Dave requested it only "...when composing the same message you want to insert another image..." but I agree it can be better to store to gconf, like save path, so this patch adds new key "/apps/evolution/mail/composer/load_dir" to gconf and stores here path last selected in composer. It's path, not URI, as in e_file_set_save_path/e_file_get_save_path.
Created attachment 98325 [details] [review] proposed evo patch ][ for evolution;
This might be bikeshedding but I would prefer the GConf key be named "current_folder" since the value winds up getting passed to gtk_file_chooser_set_current_folder(). Otherwise I really like the idea of using GConf.
Created attachment 98327 [details] [review] proposed evo patch ]I[ for evolution; here you are :)
Milan, here's a few suggestions that I think might simplify the code a bit. You can choose to implement them or not at your discretion. In the GtkHTML patch: - Never let cd->file_path be NULL. Use g_strdup (g_get_home_dir ()) as a fallback value in impl_set_file_path() and also initialize file_path to this in gtk_html_control_data_new(). This way you can always just pass cd->file_path to gtk_file_chooser_set_current_folder() without testing for NULL. - Connect to the GtkFileChooserDialog's "current-folder-changed" signal, and in the handler simply call send_path_changed_event(). This should automatically update the GConf key everytime the user changes folders in a file chooser dialog, and would eliminate the need for a string comparison after the dialog is closed. In the Evolution patch: - I'm not sure e-util.c is the best place for those composer path functions. mail/em-utils.c might be better. (And no, the save path functions don't belong there either.) - Always pass a GError to a GConfClient function, and print a warning if the operation failed. This applies to e_file_set_composer_path() and e_file_get_composer_path(). - In e_file_get_composer_path(), fall back to g_strdup (g_get_home_dir ()) if GConf returns an empty string. This ensures the function will never return NULL, saving you from having to check the return value everytime you call it. Looks good otherwise. I didn't actually test the patch but I trust you were able to get it working. Marking the patches as Reviewed. This will all be much cleaner once we exorcise Bonobo from the composer...
I can change it, but I have some questions/comments: About that g_strdup (g_get_home_dir ()), my idea was to consume as low memory as possible, even it needs more testing and it is not much memory, but it was the idea. Second ball of GtkHtml: I don't think this is a good idea, I don't want to store folder where they didn't end up. Consider you will hit Insert image, change directory but cancel dialog, so no image has been added, but the stored directory is now that wrong. First ans second ball of evo: Yes, it was the reason why I added it here to have it together on one place. And I simply copied those functions, only changed their names and gconf keys. I think you should not move save_path functions from e-util, it is used in e-dialog-utils.c too, and in plugins/backup-restore/backup-restore.c as well. So I would like to suggest keep it as is, if you don't mind :) Maybe only add that error handling to all four functions.
(In reply to comment #12) > I can change it, but I have some questions/comments: > About that g_strdup (g_get_home_dir ()), my idea was to consume as low memory > as possible, even it needs more testing and it is not much memory, but it was > the idea. I think the memory usage here is negligible, especially since it gets freed right away anyway. Memory is cheap, do what makes the code simpler. > Second ball of GtkHtml: I don't think this is a good idea, I don't want to > store folder where they didn't end up. Consider you will hit Insert image, > change directory but cancel dialog, so no image has been added, but the stored > directory is now that wrong. That's a valid point. Skip my suggestion then. > First ans second ball of evo: Yes, it was the reason why I added it here to > have it together on one place. And I simply copied those functions, only > changed their names and gconf keys. > > I think you should not move save_path functions from e-util, it is used in > e-dialog-utils.c too, and in plugins/backup-restore/backup-restore.c as well. > So I would like to suggest keep it as is, if you don't mind :) Maybe only add > that error handling to all four functions. No strong opinion here. I'm okay with leaving the save path functions alone, but I think I'd still prefer to see the new composer path functions under mail. Ask Srini if you need a second opinion; I'm okay with either choice.
Created attachment 98658 [details] [review] proposed gtkhtml patch ]I[ for gtkhtml;
Created attachment 98659 [details] [review] proposed evo patch IV for evolution; both last patches do things as suggested. I also removed those functions from e-utils and took their bodies on the only two places where they has been used, so no need to have them, if they are composer related only. And they are, from my point of view. :)
Matt, can you reivew-approve the patch for head?
Sure, I'll review and test it this weekend when I'm back from vacation. Been trying to sneak in little five minute tasks here and there but this looks like it's going to require some concentration.
Milan, this looks great. Good ahead and commit to trunk. I was thinking about how to implement this interface in my mbarnes-composer branch. With Bonobo out of the picture, I think I would just need to add a "current-folder" text property to the editor widget. Then Evolution can simply bind the property to the new configuration key (via GConfBridge, DConf, whatever). Should be nice and clean.
gtkhtml part committed to trunk. Committed revision 8636. evo part committed to trunk. Committed revision 34613.
*** Bug 248315 has been marked as a duplicate of this bug. ***