GNOME Bugzilla – Bug 707647
gnome-autoar integration in attachments
Last modified: 2016-09-11 07:30:59 UTC
This project add two function to Evolution! Users can extract archives when saving attachments. They can choose to save the original archive or extracted files, or both. Users can add directories as archives in attachments. Archives will be automatically created for each directories. In order to share archives functions with other GNOME applications, I wrote a new shared library `gnome-autoar'. This library is hosted on git.gnome.org now. These patches will cause Evolution to depend on gnome-autoar and libarchive. More information about this project: https://wiki.gnome.org/SummerOfCode2013/Projects/TingWeiLan_GnomeArchives https://mail.gnome.org/archives/evolution-hackers/2013-August/msg00023.html
Created attachment 254289 [details] [review] [1/7] Add gnome-autoar to GNOME platform dependencies configure.ac | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Created attachment 254290 [details] [review] [2/7] Add properties and widgets to support archive extraction e-util/e-attachment-store.c | 74 +++++++++++++++++++++++++++++++++++++-- e-util/e-attachment-view.c | 1 + e-util/e-attachment.c | 85 +++++++++++++++++++++++++++++++++++++++++++++ e-util/e-attachment.h | 6 ++++ 4 files changed, 164 insertions(+), 2 deletions(-)
Created attachment 254291 [details] [review] [3/7] Add simple archive extraction support e-util/e-attachment-store.c | 42 ++++++- e-util/e-attachment.c | 276 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 265 insertions(+), 53 deletions(-)
Created attachment 254292 [details] [review] [4/7] Prevent using uninitialized value e-util/e-attachment.c | 1 + 1 file changed, 1 insertion(+)
Created attachment 254293 [details] [review] [5/7] Add widgets and object data to support archive creation e-util/e-attachment-store.c | 66 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 9 deletions(-)
Created attachment 254294 [details] [review] [6/7] Add simple archive creation support e-util/e-attachment.c | 138 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 114 insertions(+), 24 deletions(-)
Created attachment 254295 [details] [review] [7/7] Fix "Open with" problem "Open with" functions are broken after I integrated archives extraction into attachment saving. This commit fixes the problem caused by the commit: c44b5c5cd564134755eba7904b2efb894c2ea513. --- e-util/e-attachment.c | 4 ++++ 1 file changed, 4 insertions(+)
(In reply to comment #1) > Created an attachment (id=254289) [details] [review] > [1/7] Add gnome-autoar to GNOME platform dependencies > configure.ac | 3 ++- Has it been considered to not make this a hard dependency, but a compile time option? Like passing --autoar=yes or --autoar=no ? If so, what were reasons against it? I'm asking because some distributions might be reluctant to ship "yet another package" for live CDs which have limited space, plus it feels weird to me to make Evolution hard-depend on another package for a rather small (though useful) functionality. > [4/7] Prevent using uninitialized value Logically this patch should be merged into attachment 254291 [details] [review] where this variable is introduced, plus I'd probably initialize variables at the top of the function (or the logical block at least). I guess other Evolution developers might have a better opinion on this. > [2/7] Add properties and widgets to support archive extraction >+ extract_only = gtk_radio_button_new_with_mnemonic (NULL, >+ _("Save extracted files only")); >+ extract_org = gtk_radio_button_new_with_mnemonic (extract_group, >+ _("Save extracted files and the original archive")); These two string do not have mnemonics. >Subject: [PATCH] Add simple archive extraction support >+ if (suggested == NULL) >+ suggested = g_strdup (_("attachment.dat")); This might welcome a translator comment. See https://wiki.gnome.org/TranslationProject/DevGuidelines/Use%20comments >Subject: [PATCH] Add widgets and object data to support archive creation >+ option_format_label = gtk_label_new ( >+ _("Archive selected directories using this format: ")); Why is there a whitespace at the end of this string?
(In reply to comment #8) > (In reply to comment #1) > > Created an attachment (id=254289) [details] [review] [details] [review] > > [1/7] Add gnome-autoar to GNOME platform dependencies > > configure.ac | 3 ++- > > Has it been considered to not make this a hard dependency, but a compile time > option? Like passing --autoar=yes or --autoar=no ? If so, what were reasons > against it? I'm asking because some distributions might be reluctant to ship > "yet another package" for live CDs which have limited space, plus it feels > weird to me to make Evolution hard-depend on another package for a rather small > (though useful) functionality. > I can try to make gnome-autoar a optional dependency. I has not considered to do this because I think gnome-autoar is just a tiny library. > > [4/7] Prevent using uninitialized value > > Logically this patch should be merged into attachment 254291 [details] [review] where this > variable is introduced, plus I'd probably initialize variables at the top of > the function (or the logical block at least). > I guess other Evolution developers might have a better opinion on this. > I will merge this patch into patch 3 (attachment 254291 [details] [review]). > > [2/7] Add properties and widgets to support archive extraction > >+ extract_only = gtk_radio_button_new_with_mnemonic (NULL, > >+ _("Save extracted files only")); > >+ extract_org = gtk_radio_button_new_with_mnemonic (extract_group, > >+ _("Save extracted files and the original archive")); > > These two string do not have mnemonics. > I will upload new patches to fix this. > >Subject: [PATCH] Add simple archive extraction support > >+ if (suggested == NULL) > >+ suggested = g_strdup (_("attachment.dat")); > > This might welcome a translator comment. See > https://wiki.gnome.org/TranslationProject/DevGuidelines/Use%20comments > This string already appears in the other file with a translator comment, so I don't add a comment here. > >Subject: [PATCH] Add widgets and object data to support archive creation > >+ option_format_label = gtk_label_new ( > >+ _("Archive selected directories using this format: ")); > > Why is there a whitespace at the end of this string? Because I want to add some space between the string and the combo box. Maybe I should do this using gtk_box_pack_start.
Created attachment 254386 [details] [review] [2/7] Add properties and widgets to support archive extraction (update)
Created attachment 254387 [details] [review] [3/7] Add simple archive extraction support (update)
(In reply to comment #8) > Has it been considered to not make this a hard dependency, but a compile time > option? Like passing --autoar=yes or --autoar=no ? If so, what were reasons > against it? I'm asking because some distributions might be reluctant to ship > "yet another package" for live CDs which have limited space, plus it feels > weird to me to make Evolution hard-depend on another package for a rather > small (though useful) functionality. I agree this needs to be an optional dependency until gnome-autoar becomes more widespread. A good acid test is whether it's available in Debian Testing. Patches look pretty good on first glance. I'd like to see if we can rework the attachment API so this functionality could live under modules/ as an optional extension. Still need to examine this more closely to see if that's even feasible.
Comment on attachment 254387 [details] [review] [3/7] Add simple archive extraction support (update) >Subject: [PATCH 2/2] Add simple archive extraction support >+ if (attachment->priv->save_extracted) { >+ EAttachment *attachment; >+ GFileInfo *info; >+ char *suggested; >+ >+ attachment = save_context->attachment; >+ suggested = NULL; Is there a reason why you don't set "char *suggested = NULL;" directly? Just asking, I'm not a programmer.
Created attachment 254608 [details] [review] [8] Make gnome-autoar be an optional dependency I add --enable-autoar argument to configure, and I use #ifdef HAVE_AUTOAR to enable or disable autoar in e-attachment.c and e-attachment-store.c. gnome-autoar is an optional dependency now.
Would it make sense to consolidate all the patches into one, please? I'm slightly confused by the set of patches being attached in random order (obsoleted by updates) with missing sequence numbers. Just a note, it's too late for 3.12 right now, but we can add this for the next major version, while we have enough time to polish it.
*** Bug 712757 has been marked as a duplicate of this bug. ***
I have to rewrite some patches because I changed some gnome-autoar API. Currently there are some conflict in the master branch, but it is easy to resolve.
Created attachment 276312 [details] [review] [PATCH] Add archives integration support using gnome-autoar configure.ac | 30 +++ e-util/Makefile.am | 2 + e-util/e-attachment-store.c | 215 +++++++++++++++++- e-util/e-attachment.c | 522 +++++++++++++++++++++++++++++++++++++------- e-util/e-attachment.h | 6 + 5 files changed, 694 insertions(+), 81 deletions(-)
Created attachment 276312 [details] [review] [PATCH] Add archives integration support using gnome-autoar
Review of attachment 276312 [details] [review]: I thought I will just fix the below tiny things, but it turned out that evolution crashes when I attempt to save just attached tar.gz file with the below backtrace. I thought the change will ask me whether I want to attach the file (tar.gz) as a single file or unpack it, but there was no such question when I selected the tar.gz file to be attached. I compiled gnome-autoar from git master branch (commit f9c0af9) and libarchive-devel-3.1.2-7.fc20.x86_64.
+ Trace 233590
Thread 1 (Thread 0x7fc07b863a80 (LWP 14330))
::: e-util/e-attachment-store.c @@ +449,3 @@ +#ifdef HAVE_AUTOAR +#define LOAD_RESPONSE_ATTACH 1 Do not use a hard-coded value, rather use certain constant from the list of GTK_RESPONSE_* constants, once which is not used below yet. @@ +534,3 @@ + + option_format_label = gtk_label_new ( + _("Archive selected directories using this format: ")); This has still the extra space in the string. I would remove it and eventually change margins/padding for the widget instead. ::: e-util/e-attachment.c @@ +2100,3 @@ +{ + attachment_load_check_for_error (load_context, + g_error_new_literal (G_IO_ERROR, G_IO_ERROR_CANCELLED, "")); Empty error message is not good. As you do not seem to have an access to underlying GCancenllable, then provide the message on your own, like the GCancellable's "Operation was cancelled". @@ +3097,3 @@ +{ + attachment_save_check_for_error (save_context, + g_error_new_literal (G_IO_ERROR, G_IO_ERROR_CANCELLED, "")); empty error message, again. @@ +3321,3 @@ + EAttachment *attachment; + GFileInfo *info; + char *suggested; s/char/gchar/
(In reply to comment #20) > Review of attachment 276312 [details] [review]: > > I thought I will just fix the below tiny things, but it turned out that > evolution crashes when I attempt to save just attached tar.gz file with the > below backtrace. I thought the change will ask me whether I want to attach the > file (tar.gz) as a single file or unpack it, but there was no such question > when I selected the tar.gz file to be attached. I compiled gnome-autoar from > git master branch (commit f9c0af9) and libarchive-devel-3.1.2-7.fc20.x86_64. > > The crash is caused by the API change in f9c0af9. I will fix the problem.
Created attachment 276682 [details] [review] [PATCH] Add archives integration support using gnome-autoar
Thanks for the update. I tried to use it and I cannot attach a folder. I selected a folder, then chose "tar.xz" format, then I pressed the "Attach" button, but instead of creating an archive, the directory had been opened in the file chooser. This is with gtk 3.10.6. When saving archive attachment, I would prefer to: a) have the checkbox "extract if it is archive" unchecked by default b) do not hide the related widgets, rather insensitive them ad a), is there an option to detect whether the attachment is an archive or not? Say at least by some mime type and fallback for application/octet-stream? Then the checkbox to extract would be hidden when it is not (or the whole group insensitive).
Just an idea, would the attach of multiselection also use the compress option, if user agreed? I'd make there a new button, "Attach compressed" in the action area of the dialog, thus even single files could be compressed. The action area would then look like: [ Open ] [ Cancel ] [ Attach compressed] [ Attach ]
(In reply to comment #23) > Thanks for the update. I tried to use it and I cannot attach a folder. I > selected a folder, then chose "tar.xz" format, then I pressed the "Attach" > button, but instead of creating an archive, the directory had been opened in > the file chooser. This is with gtk 3.10.6. > I was sorry for the wrong fix because of the untested patch. My evolution was broken then, so I could not test it before uploading the new patch. The problem is that the dialog do not close when the user select a folder if I use GTK_RESPONSE_ACCEPT, GTK_RESPONSE_OK, GTK_RESPONSE_YES, GTK_RESPONSE_APPLY. I know this behavior is intended, so I workaround it by defining the new value LOAD_RESPONSE_ATTACH in the old patch. If I change it to GTK_RESPONSE_REJECT, it will work. Is there any better way to do it? Other developers may think the use of GTK_RESPONSE_REJECT is strange and misleading. I cannot find a GtkFileChooserDialog type that can be used to select both files and folders. > When saving archive attachment, I would prefer to: > a) have the checkbox "extract if it is archive" unchecked by default > b) do not hide the related widgets, rather insensitive them > > ad a), is there an option to detect whether the attachment is an archive or > not? Say at least by some mime type and fallback for application/octet-stream? > Then the checkbox to extract would be hidden when it is not (or the whole group > insensitive). The checkbox "extract if it is archive" are checked if either its filename or its mime type matches the list of known archives defined in gnome-autoar. Users can configure it using gsettings, but there is currently no user-visible user interface to configure it. I think I need to write a tool for users to easily change the settings. (In reply to comment #24) > Just an idea, would the attach of multiselection also use the compress option, > if user agreed? I'd make there a new button, "Attach compressed" in the action > area of the dialog, thus even single files could be compressed. The action area > would then look like: > > [ Open ] [ Cancel ] [ Attach compressed] [ Attach ] Making new options for users is good, but it may makes the UI more complex. I did not think of it before because the original idea of archives integration is to make archives work like folders. Althougn the ratio of the compression is not the primary goal of this integration work, we can still add it if it is convenient. We may have to tell users what "Attach compressed" means. By the way, the archives integration UI for Evolution, Epiphany, Empathy developed in the last summer is not consistent. I hope I can make the UI usable in other applications, so users will think they are all the same things.
Aha, OK, I'm fine with all you said above, especially the UI consistency might be a plus for users. The GTK_RESPONSE_REJECT really sounds incorrect. I'm not aware of a dialog which would allow to pick both files and/or folders at once, that might require more tweaking. I think an option is to listen for a "response" signal and either close the dialog when there gets the "Attach" button pressed with emission stop of the signal, or just pass the signal forward. You might check in gtk+ sources what they do there, thus also gtk_dialog_run() works as expected.
Created attachment 283130 [details] [review] [PATCH] Add archives integration support using gnome-autoar I change GTK_RESPONSE_ACCEPT to GTK_RESPONSE_CLOSE, which is more reasonable than GTK_RESPONSE_REJECT. The signal handler connected by GtkFileChooserDialog is run before the one connected by the user. It will stop emission when the response type is one of GTK_RESPONSE_ACCEPT, GTK_RESPONSE_OK, GTK_RESPONSE_YES, GTK_RESPONSE_APPLY, preventing handlers provided by users being called.
Thanks for the update. I'm sorry for a later response from my side, I was away and only now got to your changes. The folder attaching with compression looks and works fine. I would prefer to change the attachment save though: a) default to not extract files b) the UI change is quite distracting, hiding the two radio boxes is a huge dialog change (like flashing) while it would be better to make it three radio boxes, like: (o) Do not extract files from the attachment ( ) Save extracted files only ( ) Save extracted files and the original archive c) what about not adding the panel completely, when the attachment is not an archive? The sentence "if it is an archive" looks odd. You might have whole attachment available when the save dialog is invoked, thus you should be able to detect whether it is or is not an archive. I see that you have there some logic, because a .log file has the "extract files" option unchecked, while a .7z has it checked, thus this logic might be used to hide or show the panel with options. If you prefer, then we can commit current version and iteratively improve/change it. One issue found: when I extract a file test.7z into a folder which contains this file already, but I have set "Save extracted files only", then I'm asked whether I want to rewrite test.7z file, which is not what it should ask, right?
Created attachment 285161 [details] [review] [PATCH] Use three radio buttons instead of hiding things This patch can change the original hiding behavior to three radio buttons described in the above comment. I agree that three radio buttons are better. I think the overwrite confirmation dialog is the default behavior of GtkFileChooser. I can try to make it hide the dialog because gnome-autoar never overwrites existing files or directories.
Thanks. I did only two changes when saving an attachment: a) always pre-select "Do not extract" b) hide the extra panel from the save dialog when neither the file extension nor the mime type suggests that the attachment can be extracted; it's because the options to extract content did not have any meaning there. Created commit 2d345f3 in evo master (3.13.6+) [1] [1] https://git.gnome.org/browse/evolution/commit/?id=2d345f3
(In reply to Matthew Barnes from comment #12) > I agree this needs to be an optional dependency until gnome-autoar becomes > more widespread. A good acid test is whether it's available in Debian > Testing. This finally happened (not that gnome-autoar is a hard dependency for nautilus 3.22): https://tracker.debian.org/pkg/gnome-autoar
now