Created attachment 70212 [details]
Backtrace

Created attachment 70213 [details] [review]
Proposed fix

Daniel, unfortunately you attached the stacktrace twice, rather than the patch. Please attach the patch again, thanks. :)

Created attachment 70348 [details] [review]
Actual proposed fix this time

Sorry about that.

Daniel, I dont see the crash. From which view did u save? Also, I have only a path in gconf and Im surprised to see uri in gconf. If someone is storing a URI that has to fixed. AFAICS path is only storing in gconf. Let me know, if u found where we are storing URI.

I didn't do anything special;  I just selected a number of messages in the message list (out of my junk folder, to be precise), and hit CTRL-C.

This would only be a problem with USE_GTKFILECHOOSER defined, for sure, but here's the offending code, I believe:

static void
emu_save_parts_response (GtkWidget *filesel, int response, GSList *parts)
{
        char *path = NULL;
        GSList *selected;
        if (response == GTK_RESPONSE_OK) {
#ifdef USE_GTKFILECHOOSER
                path = gtk_file_chooser_get_current_folder_uri (GTK_FILE_CHOOSER (filesel));       
#else 


Note that, with my change, either a URI or a filename works.  With the old code, a URI fails, but a filename works.  I believe this makes my change harmless, and allows saving files to non-local locations (like sftp).

Sorry, CTRL-S.  Need more coffee this morning.

Ha.. OK. Then that part was storing the URI, which wasnt known to me. OK. You have given  a nice point. With the support to store/open remote files, URI is the best candidate. We have to save/read uri every where. IIRC, calendar too uses this key. It could be great, if you can give patch to save/read uri every where. em-utils saves path at few places too, those have to be converted. Also the calendar/ too uses it just one place.


Thanks for the bug, patch, point :)

I've been staring at this for a couple of hours now, and there's a problem.  em-utils mixes uris and paths, and the non USE_GTKFILECHOOSER is horribly broken.  I'm not sure that accessing non-file:// uris will work at this point, as the actual methods used for file access are g_ methods not gnome_vfs methods, and so only work on local files.  What I'm going to do at this point is prepare a patch that 1) removes the non USE_GTKFILECHOOSER case; 2) Uses uris consistantly for em-utils and anything else accessing that gconf key.  This will not get us remote file saving, but will get us closer to it, by using uris in general.

If you'd prefer that I swith the all the paths to local, non-uri paths, I can do that instead.

Im OK to remove the NON_GTKFILECHOOSER part. I really wonder, do some one use that at all? Why was that introduced. Daniel, I guess, we can just store URI's every where. I dont see a problem. Please let me know, what problems you are facing. I implemented the remote support in 2.8. Ill be happy to fix any bug there.

daniel: ping srag in #evolution if you need some help instead of bugzilla discussions (consumes time :)

Created attachment 71472 [details] [review]
Switch to URIs everywhere

Here's what I believe to be the minimal fix for switching to URIs.  It removed the non-gtkchooser code, because there were referenced variables that were not being set in that code branch;  It changes all the dialogs to use URIs; It changes some functions to handle the fact that all strings returned by gtk_file_chooser_* and gnome_vfs_get_* are allcoated and need to be freed (thus removing some small memory leaks).

It turns out that the camel backend is using gnome-vfs, so some remote access should be possible.  However, in the save-all-attachments code, it specifically builds up local filenames.  That would have to be changed to handle URIs.

Finally, the test for popping up the overwrite dialog checks using g_access(), which only works on local files.  I believe there is a "confirm-overwrite" from gtk_file_chooser that could be used in place of this code, but I have not yet investigated it.

I've tested saving single and multiple emails, and saving single and multiple attachments, and they all work, with one exception that appears to be a bug in gtk_file_chooser:  If you turn on the ability to type in a path, when trying to save all attachments (ie, when the chooser is in folder mode), then clicking save does nothing in the case where you haven't typed anything into the path box.

I'm dang in #evolution, I'll ping you when we're both on together.

Dang Awesome patch and observations. I really love this. Just few top level things

-  e-util/e-dialog-utils (Same as em-utils) and calendar/gui/dialogs/alarm-dialog.c uses the same key 

- "res" variable has to be gboolean in emu_can_save

- some formatting issue in emu_save_parts_response

- For Save all attachments, just pass the folder uri/filename. 

file_path = g_build_filename (uri, file_name, NULL);
if (!emu_file_check_local(file_path) || !g_file_test(file_path, (G_FILE_TEST_EXISTS)) || e_error_run(NULL, E_ERROR_ASK_FILE_EXISTS_OVERWRITE, file_name, NULL) == GTK_RESPONSE_OK)
			
	mail_save_part(part, file_path, NULL, NULL, FALSE);
else
	g_warning ("Could not save %s. File already exists", file_path);

should solve. Atm just the variable name is path, but it holds a URI received from file chooser.

Thanks for your great work Dang.

Created attachment 71545 [details] [review]
new URI patch

Okay, here's take 2 on the URI patch.  What this patch does:
- Create a new gconf key for the calendar save location /apps/evolution/calendar/save_dir
- Create schemas for both mail/save_dir and calendar/save_dir
- Fix a memory leak for default path in the calendar save-as code
- Get rid of the non-USE_GTKFILECHOOSER code in e-utils/e-dialog-utils and mail/em-utils
    - It was broken in almost all cases (ie, not filling in local variables thate were later referenced in unconditional code)
- move emu_get_save_filesel to e-utils/e-dialog-utils and rename it e_file_get_save_filesel
- Make all file selecter creation use e_file_get_save_filesel in both places
- move emu_can_save to e_file_can_save, and consolidate all checks in both files to use that (and it's associated overwrite confirmation dialog)
- move emu_file_check_local to e_file_check_local, change it to use gnome_vfs instead of camel, and make all local file checks use it in both files.
- move emu_update_save_path and emu_get_save_path to e_file_update_save_path and e_file_get_save_path and fix all access to /apps/evolution/mail/save_dir to use those and to use URIs.

It would be nice if there was some clean gnome_vfs functions to check for file existance and writability by URI, but there aren't that I could find.  So, I'm still using the glib functions that act on local paths.

How I tested this:
I tested mail saving (both singly and in groups), attachment saving (both singly and in groups), and adding attachments to calendar entries.  All worked correctly, and all used the correct gconf keys.

String freeze break:
This breaks the string freeze in two ways: 1) It adds a schema for /apps/evolution/mail/save_dir, where one did not exist before; and 2) it adds /apps/evolution/calendar/save_dir and it's associated schema.

The reason for adding the new calendar save_dir is that the calendar code uses the older gnome_file_entry code, which does not work on URIs as far as I can tell.  Rather than converting it all to use gtk_file_chooser, I chose to let it save it's own path to gconf in local-path format, rather than URI.

If it's necessary, I can look into porting the calendar code to use gtk_file_chooser instead.  this would be more work and a bigger patch.

Dang, the patch looks perfectly OK. But for some reason, the calendar doesnt remember the path, though, it is set there. Im not so keen on it atm. Can you request for a string freeze break to release-team/gnome-i18n ? 

The main point could be that, there is a key used without a schema.

Adding attachments to email and appointments, as opposed to saving them, doesn't seem to use a saved directory.  Instead, it always opens up in the homedir.

I've submitted the string freeze request.  srag and I decided to rename the calendar save key, new patch attached.

Created attachment 71610 [details] [review]
new URI patch with key rename

The same as the previous patch, except that the new gconf key is renamed from
/apps/evolution/calendar/save_dir
to
/apps/evolution/calendar/audio_dir

Dan, we should commit it to head, when the branching is done.

srini: if your last comment is a "accepted-commit_after_freeze", feel free to set the patch status accordingly. :-)

Please commit to head once we branch.

I'm afraid the patch does not apply clean on the HEAD anymore. 
Dan/Srini : Can you push in an updated patch pl. ?

Created attachment 74827 [details] [review]
Here is the updated patch without the conflict.

Fixed to HEAD.