GNOME Bugzilla – Bug 768646
Add support for extracting compressed files
Last modified: 2016-08-22 21:42:58 UTC
See patches.
Created attachment 331180 [details] [review] file-conflict-dialog: separate file logic from UI management In Nautilus, file conflicts are handled by a specific dialog. Previously, the dialog class managed both the UI and the related nautilus files. This lead to it being inflexible due to operation specific logic being mixed with the rest of the functionality. In order to change this, move file logic to a separate module and add methods for controlling the UI elements in the dialog. Create an operation-manager module to handle dialog controlling. Move anything related to Nautilus files from the dialog class to the new module.
Created attachment 331181 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Created attachment 331182 [details] [review] files-view: add context menu actions for extracting files The context menu actions are similar to the ones offered by file-roller, but make use of the internal extract operation.
Created attachment 331183 [details] [review] general: add preference for automatic decompression of archives With the extract operation being handled internally, compressed files can be automatically extracted instead of being opened in file-roller. In order to implement this, make extraction the default action for activating selected archives.
Review of attachment 331180 [details] [review]: Looks beautiful :) Some minor reviews: ::: src/Makefile.am @@ +274,3 @@ nautilus-file-operations.c \ + nautilus-operations-manager.h \ + nautilus-operations-manager.c \ I'm not sure I like the name. How about operations-dialog-manager or operations-ui-manager? ::: src/nautilus-file-operations.c @@ +4391,3 @@ +static FileConflictResponse * +signal_copy_move_conflict (CommonJob *job, the name is misleading, it doesn't signal anything. How about handle_copy_move_conflict? ::: src/nautilus-operations-manager.c @@ +10,3 @@ +typedef void (*FileConflictDialogFunc) (FileConflictDialogData *data); + +struct _FileConflictDialogData { Why declare it in this way and not just typedef struct { blah} FileConflictDialogData? It looks like a private of a gobject but it's not. @@ +31,3 @@ + gulong dest_handler_id; + /* Dialogs are ran from operation threads, which need to be blocked until + * the user gives a valid response needs to be valid? I guess "a response" is what we receive. I wonder if we can somehow explain the GCond stuff in a comment. I don't have a good proposal though. Can you try to make a comment? @@ +44,3 @@ + g_autofree gchar *secondary_text = NULL; + const gchar *message_extra; + time_t src_mtime, dest_mtime; one per line @@ +149,3 @@ +set_file_labels (FileConflictDialogData *data) +{ + char *size, *date, *type = NULL; one per line @@ +187,3 @@ + g_string_append_printf (destination_label, "%s %s", _("Last modified:"), date); + + g_free (size); gautofree and use source_date, destination_date, etc? @@ +314,3 @@ + +static gboolean +run_file_conflict_dialog (gpointer _data) don't do that, just use user_data or what the docs have. ::: src/nautilus-operations-manager.h @@ +10,3 @@ + + +FileConflictResponse * get_copy_move_file_conflict_response (GtkWindow *parent_window, I don't like the "get". how about something along the lines of handle_copy_move_file_conflict? copy_move_file_conflict_ask_user_action? copy_move_file_conflict_request_action?
Review of attachment 331181 [details] [review]: Looking good overall! A few questions/issues raised: ::: configure.ac @@ +272,3 @@ gtk+-3.0 >= gtk_minver glib-2.0 >= glib_minver + gnome-autoar >= autoar_minver identation ::: src/nautilus-file-operations.c @@ +52,3 @@ #include <gio/gio.h> #include <glib.h> +#include <gnome-autoar/autoar.h> why not gnome-autoar? I don't think a library should need to specify part of it (although is pretty common and good practice in other languages) @@ +7130,3 @@ +static FileConflictResponse * +signal_extract_conflict (CommonJob *job, same as previous patch @@ +7151,3 @@ +} + + is there a way that this returns TRUE and there is an error set and the opposite? I have the feeling we can just use the error parameter without the need of a boolean @@ +7152,3 @@ + +static gboolean + response = get_extract_file_conflict_response (job->parent_window, Why not use directly the delete_file with permanent delete? If it's because delete_file does much more than what we need (undo, etc) couldn't we share some code with this? @@ +7153,3 @@ +static gboolean +remove_file (GFile *file, + response = get_extract_file_conflict_response (job->parent_window, aligment @@ +7155,3 @@ + GError **error) +{ + src, is better to assume otherwise, then explicitly set when is successful. @@ +7217,3 @@ + g_clear_object (&extract_job->decided_destination); + + and the disconnect from the signals? And the monitors from src_handler_id etc? @@ +7296,3 @@ + + decided_destination = get_target_file_for_display_name (destination_directory, + g_clear_object (&extract_job->source); aligment @@ +7314,3 @@ + + if (decided_destination != NULL) { +extract_scanned_handler (AutoarExtract *arextract, you ref this twice, I believe you are leaking it. or is ti a ref for the extract_job and another for the return param? In that case, why not set the extract_job->decided_destination in the client? In any case, this way is confusing @@ +7340,3 @@ + elapsed = g_timer_elapsed (common->time, NULL); + transfer_rate = 0; + GList *files, This shouldn't happen. Set the initialization to something invalid, like a negative number if you believe this could fail. But this should be handled explicitly in some way. @@ +7468,3 @@ + ExtractJob *extract_job = task_data; + g_autoptr (GSettings) archive_settings; + break; ar_preferences Basically is fine to me to abbreviate names, but not words @@ +7477,3 @@ + arpref); + + no magic numbers, use constants.
Review of attachment 331182 [details] [review]: looks mostly good! ::: src/nautilus-application.c @@ +966,3 @@ nautilus_ensure_extension_points (); nautilus_ensure_extension_builtins (); + spurious new line @@ +1049,3 @@ +AutoarPref * +nautilus_application_get_arpref (NautilusApplication *self) get_autoar_preferences. API is the most important place to be clear. ::: src/nautilus-application.h @@ +25,3 @@ #include <gio/gio.h> #include <gtk/gtk.h> +#include <gnome-autoar/autoar.h> same as previous patch? ::: src/nautilus-file.c @@ +7022,3 @@ + mime_type = nautilus_file_get_mime_type (file); + + return autoar_pref_check_mime_type_d (arpref, mime_type); is this sync?
Review of attachment 331183 [details] [review]: looks nice! :) ::: data/org.gnome.nautilus.gschema.xml @@ +105,3 @@ </key> + <key type="b" name="automatic-decompression"> + <default>false</default> true by default ::: src/nautilus-global-preferences.h @@ +33,3 @@ #define NAUTILUS_PREFERENCES_CONFIRM_TRASH "confirm-trash" +/* Compressed files options */ Automatic decompression
Created attachment 333717 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Created attachment 333718 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Created attachment 333719 [details] [review] files-view: fix hash table usage in tracking newly added locations Newly added locations are tracked in a hash table that only keeps keys with no values. In order to fix this, use the hash table as a set.
Created attachment 333720 [details] [review] files-view: add context menu actions for extracting files The context menu actions are similar to the ones offered by file-roller, but make use of the internal extract operation.
Created attachment 333721 [details] [review] general: add preference for automatic decompression of archives With the extract operation being handled internally, compressed files can be automatically extracted instead of being opened in file-roller. In order to implement this, make extraction the default action for activating selected archives.
Comment on attachment 331180 [details] [review] file-conflict-dialog: separate file logic from UI management As extraction no longer has its own conflict dialog, I moved this to a new bug: https://bugzilla.gnome.org/show_bug.cgi?id=770160.
Review of attachment 333718 [details] [review]: Add also the gnome-autoar to jhbuild, or link a bug report here in this bug report about that please. ::: src/nautilus-file-operations.c @@ +185,3 @@ + guint64 total_size; + guint total_files; + AutoarExtractor *extractor; As explained on IRC a job, as we do for other operations, is a single user action. Here instead of having a job per each extraction, create a job for all the files. @@ +199,3 @@ #define SECONDS_NEEDED_FOR_RELIABLE_TRANSFER_RATE 8 #define NSEC_PER_MICROSEC 1000 +#define PROGRESS_NOTIFY_INTERVAL 100 * NSEC_PER_MICROSEC what is NSEC_PER_MICROSEC? :/ Not your fault though, is there a reason 1000 nanoseconds wouldnt be a microsecond? @@ +7084,3 @@ + _("Verifying destination")); + + } don't multiline in the first line @@ +7129,3 @@ + + if (elapsed < SECONDS_NEEDED_FOR_RELIABLE_TRANSFER_RATE || + extract_job->total_size = autoar_extractor_get_total_size (extractor); I wonder if we could share all of this code with the other operations that report in this way. It's fine in this way for now though. @@ +7250,3 @@ + + g_signal_connect (extract_job->extractor, "scanned", + if (extract_job->total_files == 1) { use the *_on_* pattern ::: src/nautilus-file-utilities.c @@ +587,3 @@ nautilus_ensure_unique_file_name (const char *directory_uri, + const char *base_name, + const char *extension) this file modifications in a different patch, before this one please. Let's make this a helper function, if you think it worth, and just work with a GFile API like in the following function. I think a good API is GFile * nautilus_generate_unique_file_in_directory (GFile *parent, const gchar* basename).
Review of attachment 333719 [details] [review]: The commit message is misleading, what are you fixing exactly? It's just a code improvement. Also the title is too long. How about: files-view: Use a hashset for newly added locations There is no hash set in glib, so we have to use GHashTable. However there are functions that allow to use the hash table as hash set. Use that instead of the regular hash table functions.
Review of attachment 333720 [details] [review]: looks good apart of some nitpicks! ::: src/nautilus-files-view.c @@ +675,2 @@ static gboolean +showing_remote_directory (NautilusFilesView *view) we have nautilus_directory_is_remote for this. @@ +5428,3 @@ + gboolean extracting_to_current_directory; + + nautilus_file_get (l->data)); is this a valid case? if not, g_return_if_fail is better. @@ +5439,3 @@ + locations = g_list_reverse (locations); + + nautilus_files_view_reveal_selection (data->view); no multiline for first line @@ +5448,3 @@ + data = g_new (ExtractData, 1); + data->view = view; + gboolean acknowledged; dito
Review of attachment 333721 [details] [review]: commit message: "make extraction the default action for activating selected archives and add an option to opening them instead of extract" ::: data/org.gnome.nautilus.gschema.xml @@ +106,3 @@ + <key type="b" name="automatic-decompression"> + <default>true</default> + <summary>Whether to extract compressed files instead of opening them</summary> them in an external application ::: src/nautilus-files-view.c @@ +6430,3 @@ + can_extract_files && + (!settings_automatic_decompression || + can_extract_here)); why erxtract to depends on can_extract_here? It just depends on can_extract_files no? @@ +6735,3 @@ + file = NAUTILUS_FILE (l->data); + + if (!nautilus_mime_file_extracts (file)) { It's slightly confusing that depending on a gsetting we return an app for a mime type or not. I wonder if we could improve the understandability of that, took me some time. Let's not get stuck on this now though. ::: src/resources/ui/nautilus-preferences-window.ui @@ +781,3 @@ + <child> + <object class="GtkCheckButton" id="automatic_decompression_checkbutton"> + <property name="xalign">0</property> Extract the files on open. But let's ask designers too.
Created attachment 333775 [details] [review] file-utilities: refactor ensure_unique_file_name The function works with strings instead of GFiles. Replace it with a function that generates unique files in a directory using GFiles.
Created attachment 333776 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Created attachment 333777 [details] [review] files-view: use a hashset for newly added locations There is no hash set in GLib, so we have to use GHashTable. However there are functions that allow to use the hash table as hash set. Use that instead of the regular hash table functions.
Created attachment 333778 [details] [review] files-view: add context menu actions for extracting files The context menu actions are similar to the ones offered by file-roller, but make use of the internal extract operation.
Created attachment 333779 [details] [review] general: add preference for automatic decompression of archives Make extraction the default action for activating selected archives and add an option to open them instead of extracting.
Review of attachment 333775 [details] [review]: ::: src/nautilus-file-utilities.c @@ +585,3 @@ +static GFile * +nautilus_ensure_unique_file_in_directory (GFile *directory, I'm not sure it worth splitting this function. Doesn't make much sense since it's actually generating a new name too. How about we merge this code into the lower function?
Review of attachment 333776 [details] [review]: ::: src/nautilus-file-operations.c @@ +7056,3 @@ +static void +extract_job_on_progress (AutoarExtractor *extractor, + GList *files, completed implies what have done in the past, which is the sum til the current. I think this was fine before, any reason why you changed it? @@ +7077,3 @@ + f (_("Extracting “%B”"), source_file)); + + basename = g_file_get_basename (destination); current here means for this specific file? Maybe we are not searching for "current" as the word, but for current_file_total_size or file_total_size, implying that is the current one, since the parameter is the extractor of a single file. @@ +7247,3 @@ + + if (info) { + nautilus_progress_info_set_remaining_time (common->progress, sizes? @@ +7254,3 @@ + if (extract_job->total_size) { + for (i = 0; i < total_files; i++) { + } ugh, I would do this when required, not stored like this because it's slightly confusing.
Review of attachment 333777 [details] [review]: +1
Review of attachment 333778 [details] [review]: Looking good!
Review of attachment 333779 [details] [review]: Apart of the things to improve in future mentioned before, this looks good to me!
(In reply to Carlos Soriano from comment #24) > Review of attachment 333775 [details] [review] [review]: > > ::: src/nautilus-file-utilities.c > @@ +585,3 @@ > > +static GFile * > +nautilus_ensure_unique_file_in_directory (GFile *directory, > > I'm not sure it worth splitting this function. Doesn't make much sense since > it's actually generating a new name too. How about we merge this code into > the lower function? Agreed.
Created attachment 333826 [details] [review] file-utilities: refactor ensure_unique_file_name The function works with strings instead of GFiles. Replace it with a function that generates unique files in a directory using GFiles.
Created attachment 333837 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Review of attachment 333826 [details] [review]: +1
Review of attachment 333837 [details] [review]: hope I got the names right now ::: src/nautilus-file-operations.c @@ +7077,3 @@ + f (_("Extracting “%B”"), source_file)); + + decided_destination = nautilus_generate_unique_file_in_directory (extract_job->destination_directory, archive_total_extracted_size @@ +7079,3 @@ + archive_total_size = autoar_extractor_get_total_size (extractor); + + basename); archive_extracted_progress = archive_current_extracted_size / archive_total_extracted_size @@ +7080,3 @@ + + archive_progress = (gdouble)archive_completed_size / (gdouble)archive_total_size; + basename); archive_weight = extract_job->unextracted_total_size ? extract_job->archive_unextracted_size / extract_job->unextracted_total_size;
Created attachment 333850 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Review of attachment 333850 [details] [review]: yeah!
Created attachment 333855 [details] [review] file-operations: implement extract operation Add a new operation for extracting archives using gnome-autoar.
Created attachment 333857 [details] [review] files-view: add context menu actions for extracting files The context menu actions are similar to the ones offered by file-roller, but make use of the internal extract operation.
Review of attachment 333855 [details] [review]: LGTM
Review of attachment 333857 [details] [review]: ::: src/resources/ui/nautilus-files-view-context-menus.ui @@ +237,3 @@ <attribute name="id">extensions</attribute> + <item> + <attribute name="label" translatable="yes">Extract Here</attribute> you are still missing the nmemonics
Created attachment 333871 [details] [review] files-view: add context menu actions for extracting files The context menu actions are similar to the ones offered by file-roller, but make use of the internal extract operation.
Review of attachment 333871 [details] [review]: ::: src/resources/ui/nautilus-files-view-context-menus.ui @@ +237,3 @@ <attribute name="id">extensions</attribute> + <item> + <attribute name="label" translatable="yes">_Extract _Here</attribute> _o_m_g :)
Created attachment 333873 [details] [review] files-view: add context menu actions for extracting files The context menu actions are similar to the ones offered by file-roller, but make use of the internal extract operation.
Review of attachment 333873 [details] [review]: yeah!
Attachment 333777 [details] pushed as 211bfdf - files-view: use a hashset for newly added locations Attachment 333779 [details] pushed as 51dbfc9 - general: add preference for automatic decompression of archives Attachment 333826 [details] pushed as 8d9a036 - file-utilities: refactor ensure_unique_file_name Attachment 333855 [details] pushed as 895ec57 - file-operations: implement extract operation Attachment 333873 [details] pushed as 9f0c9a9 - files-view: add context menu actions for extracting files