GNOME Bugzilla – Bug 705599
Automatically extract downloaded archives
Last modified: 2018-08-03 20:01:40 UTC
This is part of Google Summer of Code 2013 project. https://wiki.gnome.org/SummerOfCode2013/Projects/TingWeiLan_GnomeArchives If downloaded files are archives, Epiphany will try to extract the archives into a single file or a single top-level directory. This feature can be disabled by users in the preferences dialog. In order to make this feature shared between applications, I have written a shared library called libgnome-autoar, which is available at: http://www.tfcis.org/~lantw44/cgit/cgit.cgi/gsoc2013-libgnome-autoar/ Patches in this bug report will cause Epiphany to depend on libgnome-autoar.
Created attachment 251019 [details] [review] 0000-Add-some-extensions-of-compressed-files.patch
Created attachment 251020 [details] [review] 0001-Add-gsettings-key-extract-downloaded-archives.patch
Created attachment 251022 [details] [review] 0002-Add-libarchive-to-dependencies.patch
Created attachment 251023 [details] [review] 0004-Add-a-checkbutton-to-toggle-the-automatical-extracti.patch
Created attachment 251024 [details] [review] 0011-Integrate-autoarchive-functions-into-Epiphany-downlo.patch
Created attachment 251025 [details] [review] 0013-Show-error-message-in-download-panel-if-extraction-f.patch
Created attachment 251026 [details] [review] 0015-Click-Show-in-folder-when-extracting-should-not-dest.patch
Created attachment 251027 [details] [review] 0014-Add-seek-and-skip-callback-functions-for-libarchive.patch
Created attachment 251028 [details] [review] 0015-Click-Show-in-folder-when-extracting-should-not-dest.patch
Created attachment 251029 [details] [review] 0016-Allow-users-to-click-Open-if-extraction-has-failed.patch
Created attachment 251030 [details] [review] 0017-Remove-all-things-related-to-autoarchive-which-becom.patch
Created attachment 251031 [details] [review] 0018-Use-new-libgnome-autoar-API.patch
Created attachment 251032 [details] [review] 0019-Add-cancellable-support-to-download-panel-when-extra.patch
+apinheiro from the release team. Without reviewing this yet, a few comments on its possibility for integration. (In reply to comment #0) > In order to make this feature shared between applications, I have written a > shared library called libgnome-autoar, which is available at: > http://www.tfcis.org/~lantw44/cgit/cgit.cgi/gsoc2013-libgnome-autoar/ I think you should be hosting your library in git.gnome.org if we're going to make the applications depend on it. Talk with your mentor about this. > Patches in this bug report will cause Epiphany to depend on libgnome-autoar. I am not sure what's the procedure nowadays to add new dependencies. Probably we need to propose this as a new feature for 3.12 in desktop-devel-list, and then the dependency would need to get some sort of approval. Only then we can make epiphany depend on it.
(In reply to comment #14) > +apinheiro from the release team. > > I think you should be hosting your library in git.gnome.org if we're going to > make the applications depend on it. Talk with your mentor about this. Another important point is that the library should remain maintained through time. Are you intending to maintain it after the GSoC project is over?
(In reply to comment #14) > Without reviewing this yet, a few comments on its possibility for integration. > > (In reply to comment #0) > > In order to make this feature shared between applications, I have written a > > shared library called libgnome-autoar, which is available at: > > http://www.tfcis.org/~lantw44/cgit/cgit.cgi/gsoc2013-libgnome-autoar/ > > I think you should be hosting your library in git.gnome.org if we're going to > make the applications depend on it. Talk with your mentor about this. > gnome-autoar is hosted in git.gnome.org now. https://git.gnome.org/gnome-autoar/
Created attachment 257171 [details] [review] Do not assume downloaded files are saved in ephy_file_get_downloads_dir ().
Some patches have to be modified because they conflict with the master branch.
Hey Ting-Wei, who was your GSoC mentor?
My GSoC mentor was Cosimo Cecchi.
Created attachment 283113 [details] [review] Add archives extraction support using gnome-autoar I have merged all old patches into this single patch file. It should work on current master branch.
(In reply to comment #14) > I am not sure what's the procedure nowadays to add new dependencies. Probably > we need to propose this as a new feature for 3.12 in desktop-devel-list, and > then the dependency would need to get some sort of approval. Only then we can > make epiphany depend on it. We don't need any permission as long as it's in gnome-suites-core or gnome-suites-core-deps, but it's not so we can't use it. So the next step would be to get this library into the GNOME modulesets. I don't want to hide my disappointment that yet another GSoC project has been basically left to waste. This is a big recurring problem.
Review of attachment 283113 [details] [review]: Hm, this has been on the unreviewed patches list for too long. Sorry. :( First of all, there are some issues with gnome-autoar that would need to be addressed before we merge this: * Most importantly, it looks like gnome-autoar has never been released, so first step is to make a release. If you haven't done this before, maybe your GSoC mentor can help. * Since gnome-autoar is not in any GNOME modulesets, we can't depend on it. Please first work with the release team to get gnome-autoar accepted into gnome-suites-core or gnome-suites-core-deps. * Please also get some gtkdoc up on developer.gnome.org. (I'm not sure what the process is for this.) Next, please get a GNOME designer to approve the design in this bug (the new preference and the automatic extraction user experience). I will do a preliminary review of just the code; probably one of the maintainers will want to do so as well. ::: embed/ephy-download.c @@ +439,3 @@ + **/ +AutoarExtract * +ephy_download_get_autoar_extract (EphyDownload *download) Is this useful anywhere except the signal handler for EphyDownload::archive? Can we just pass it as a parameter for that signal instead? Then we wouldn't need it in the private struct. @@ +599,3 @@ + + settings = g_settings_new (AUTOAR_PREF_DEFAULT_GSCHEMA_ID); + arpref = autoar_pref_new_with_gsettings (settings); I am not sure about this autoar API... wouldn't it be good to have a simple autoar_pref_new() that uses the right gsettings schema by default? @@ +605,3 @@ + + if (!autoar_pref_check_file_name_file (arpref, destination) && + !autoar_pref_check_mime_type_file (arpref, destination)) { Also not sure about this API. It feels like this is code that will be the same in every app that uses gnome-autoar, doesn't it? Maybe there should be a function to Do The Right Thing: autoar_is_archive (destination); And that would create an AutoarPref using the right gsettings schema, and return true if autoar_pref_check_file_name_file || autoar_pref_check_mime_type_file checks out. @@ +613,3 @@ + } + + g_assert ((destination_dir = g_file_get_parent (destination)) != NULL); Don't put important code inside a g_assert, since if you put -DG_DISABLE_ASSERT in CFLAGS then this code will disappear. @@ +623,3 @@ + g_signal_emit_by_name (download, "archive", cancellable); + + autoar_extract_start_async (arextract, cancellable); How do we know when it has finished? Shouldn't this require a GAsyncReadyCallback? @@ +631,3 @@ + g_object_unref (destination_dir); + + return; Don't put an empty return at the end of the void functions. @@ +653,3 @@ + g_object_unref (priv->arextract); + priv->arextract = NULL; + } This entire conditional can be replaced with g_clear_object() (it's null-safe). (But I think you can just get rid of priv->arextract, as mentioned before.) @@ +880,3 @@ + if (g_settings_get_boolean (EPHY_SETTINGS_MAIN, EPHY_PREFS_EXTRACT_DOWNLOADED_ARCHIVES)) + ephy_download_do_extract_archive (download); But since the extraction is asynchronous, it's not going to have completed before we call ephy_download_do_download_action(), right? Is it right to run that code in this case? ::: embed/ephy-download.h @@ +61,3 @@ char *suggested_filename); void (* completed) (EphyDownload *download); + void (* archive) (EphyDownload *download); I guess "archive" can be used as a verb, but it's not immediately clear what this signal is for. Hopefully you can find a better name for it. "archive_extraction_started" would work, but that's a bit verbose.... ::: lib/widgets/ephy-download-widget.c @@ +340,2 @@ static void +widget_archive_decide_cb (AutoarExtract *arextract, My nit for this function is just that it's feeling quite cramped. Try adding blank lines after the variable declarations, after the LOG statements, and before the g_frees. @@ +369,3 @@ + + progress = completed_size * 100 / widget->priv->total_size; + label = g_strdup_printf (_("Extracting: %u%%"), progress); Needs a translator comment; please explain to the translator what the %u%% does, or that is sure to be mangled in some translation -> crash maybe. @@ +382,3 @@ + char *errmsg; + + errmsg = g_strdup_printf (_("Finished (extraction error: %s)"), error->message); Please add a comment above, for the translators. I would ask a designer about using "Finished" for an error. @@ +389,3 @@ + + g_object_unref (widget->priv->arextract); + widget->priv->arextract = NULL; g_clear_object() @@ +397,3 @@ +{ + revert_download_icon_and_text (widget); + update_download_label_and_tooltip (widget, _("Finished (extraction cancelled)")); Needs a translator comment. @@ +408,3 @@ + update_download_label_and_tooltip (widget, _("Finished")); + g_object_unref (widget->priv->arextract); + widget->priv->arextract = NULL; g_clear_object() @@ +418,3 @@ + AutoarExtract *arextract = ephy_download_get_autoar_extract (ephy_download); + if (widget->priv->arextract != NULL) + g_object_unref (widget->priv->arextract); g_clear_object() @@ +421,3 @@ + widget->priv->arextract = g_object_ref (arextract); + if (widget->priv->arcancel != NULL) + g_object_unref (widget->priv->arcancel); g_clear_object() @@ +428,3 @@ + g_signal_connect (arextract, "cancelled", G_CALLBACK (widget_archive_cancelled_cb), widget); + g_signal_connect (arextract, "completed", G_CALLBACK (widget_archive_completed_cb), widget); + update_download_label_and_tooltip (widget, _("Preparing for extraction…")); Translator comment @@ +443,3 @@ if (ephy_download_do_download_action (widget->priv->download, + EPHY_DOWNLOAD_ACTION_BROWSE_TO) && + (widget->priv->arextract == NULL)) Looks like the whitespace is messed up herer. @@ +654,3 @@ + g_signal_handlers_disconnect_by_func (arextract, widget_archive_completed_cb, widget); + g_object_unref (widget->priv->arextract); + widget->priv->arextract = NULL; Use g_clear_object() here as well. @@ +659,3 @@ + if (widget->priv->arcancel != NULL) { + g_object_unref (widget->priv->arcancel); + widget->priv->arcancel = NULL; And here (replace the entire conditional).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/202.