GNOME Bugzilla – Bug 618443
Revamp download management UI
Last modified: 2011-03-07 20:38:49 UTC
In GNOME Shell, AFAIK, there won't be any tray icon business anymore, so we need to rethink some of our UI in the download area. The minimum change would be to get rid of the tray icon and allow another way of accessing the downloads window (menu item?), but personally I'd favor redoing the thing completely and moving to something like Chromium does, which I find pretty nice.
Perhaps we should see how Salomon Sickert's GSoC project about a unified progress indicator plays out?
(In reply to comment #0) > In GNOME Shell, AFAIK, there won't be any tray icon business anymore AFAIK, the current plan is that trayicons will still exist in the message tray, just not in the "panel" (In reply to comment #1) > Perhaps we should see how Salomon Sickert's GSoC project about a unified > progress indicator plays out? Anything coming out of SoC is probably too late for 3.0.
Agree. I'll start working on rewiring/rewriting the downloads code this week. I look forward to also redo the UI. Should we ask folks at #gnome-design? I know from their dropbox folder that there are nice mockups for a couple of apps like gedit/totem. I guess they won't mind giving some thought to this.
Forgot to mention this URL: http://limi.net/articles/improving-download-behaviors-web-browsers/ It has a nice summary of how downloads work in almost all browsers.
*** Bug 631958 has been marked as a duplicate of this bug. ***
Quoting Jon's message from the other bug: GNOME 3 won't really have support for status icons. Currently it seems that epiphany uses a status icon to indicate or access downloaded files. We'll need to find a different solution for that. My recommendation is to simply use libnotify to send a message to the user when the downloads are done. To show progress of the downloads I recommend something within the browser window - possibly similar to how firefox4/chrome are handling this. For a bit more information please see: http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/Compatibility
Update about Taskview and its Epiphany integration: http://ssickert.wordpress.com/2010/11/22/taskview-release/
Marking a lot of old bugs as blocked-by.
How likely is it that anything is going to land here for gnome 3.0 ?
There's a branch in git: http://git.gnome.org/browse/epiphany/log/?h=downloads Together with Xan we should get this done before monday, likely.
(In reply to comment #10) > There's a branch in git: > http://git.gnome.org/browse/epiphany/log/?h=downloads > > Together with Xan we should get this done before monday, likely. Before monday might be optimistic (still have to look at the latest iteration), but I consider it a blocker for 3.0 so it's very likely that we'll get it done.
Created attachment 181535 [details] [review] ephy-download: new EphyDownload goodness Bug #618443
Created attachment 181536 [details] [review] port: ephy-embed to ephy-download Bug #618443
Created attachment 181537 [details] [review] port: popup-commands Bug #618443
Review of attachment 181535 [details] [review]: ::: embed/ephy-download.c @@ +637,3 @@ + "Internal WebKitDownload", + "The WebKitDownload used internally by EphyDownload", + GTK_TYPE_WINDOW, GTK_TYPE_WINDOW? Having a property you shouldn't "rely" on seems really crappy :/ @@ +716,3 @@ + "A EphyEmbed", + "Embed that produced this download.", + G_TYPE_OBJECT, GTK_TYPE_WIDGET @@ +732,3 @@ + "A GtkWidget", + "GtkWidget showing this download.", + G_TYPE_OBJECT, GTK_TYPE_WIDGET @@ +811,3 @@ + return FALSE; +} + This seems kinda useless. Sholudn't be the error reported by ED so that the UI can do something about it? ::: embed/ephy-embed-shell.c @@ +106,3 @@ + priv->downloads = NULL; + } + This should be done on finalize. @@ +357,3 @@ + g_signal_new ("download-added", + EPHY_TYPE_EMBED_SHELL, + G_SIGNAL_RUN_FIRST | G_SIGNAL_RUN_LAST, You probably don't want to do that. @@ +374,3 @@ + g_signal_new ("download-removed", + EPHY_TYPE_EMBED_SHELL, + G_SIGNAL_RUN_FIRST | G_SIGNAL_RUN_LAST, Same. @@ +659,3 @@ + priv->downloads = NULL; + } + What's the point of freeing this here?
Review of attachment 181536 [details] [review]: Crazy.
Created attachment 181547 [details] [review] ephy-download: add the new EphyDownload object EphyDownload is a wrapper object around WebKitDownload that handles common behavior in downloads: auto-destination, default action for the MIME type. It can be used to wrap a WebKitDownload coming from a WebKitView or to download a url: ephy_download_new_for_uri and ephy_download_new_for_download are provided. Its lifetime is not automagic like EphyEmbedPersist, so you have to unref it when you no longer need it. This new object replaces EphyEmbedPersist and enables us to use a single codepath for downloads in all Epiphany. Bug #618443
Created attachment 181569 [details] [review] embed-persist: downloader-view: removed Remove all our legacy downloads code. Bug #618443
Created attachment 181570 [details] [review] ephy-file-helpers: only one downloads_dir function Remove all the ambiguity, we always throw downloads to the same place or the Desktop. Bug #618443
Created attachment 181571 [details] [review] ephy-file-helpers: allow desktop in ephy_file_browse_to Previously we were denying browsing to the desktop or children of it because the downloads automatically opened insane numbers of windows. Bug #618443
Created attachment 181572 [details] [review] ephy-file-helpers: add uninstalled ui/ in ephy_file Bug #618443
Created attachment 181573 [details] [review] ephy-window: add new downloads UI Downloads are now shown per-window, imitating Chromium's bottom bar. If a window with ongoing downloads is closed, their downloads will go to another window. TBD: it might prove better to just ask if you want to cancel them or not. Bug #618443
Created attachment 181574 [details] [review] e-window: make construct_confirm_close_dialog reusable Bug #618443
Created attachment 181575 [details] [review] e-window: confirm close with downloads Bug #618443
Created attachment 181576 [details] [review] totem-glow-button: a glow button derived from libwnck Bug #618443
Created attachment 181577 [details] [review] e-d-widget: new widget Bug #618443
Created attachment 181578 [details] [review] popup-commands: indent fix Bug #618443
Review of attachment 181547 [details] [review]: Looks OK to me overall. ::: embed/ephy-download.c @@ +72,3 @@ + PROP_EMBED, + PROP_WIDGET +}; Being super nit-picky, lose the blank lines :P @@ +357,3 @@ + char *scheme; + + g_return_if_fail (EPHY_IS_DOWNLOAD (download)); probably also check destination is not NULL. @@ +363,3 @@ + scheme = g_uri_parse_scheme (destination); + g_return_if_fail (scheme != NULL); + g_free (scheme); I suppose you want to double check here that this has file:// on it, why don't you just check that...? Otherwise do a more thorough check of the whole thing, if there's a method to do that. @@ +523,3 @@ + * Gets the source URI that this download is/will download. + * + * Returns: (transfer none): source URI. Are annotations needed for "const char*"? I don't think so.
Review of attachment 181569 [details] [review]: OK.
Review of attachment 181570 [details] [review]: OK.
Review of attachment 181577 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +111,3 @@ + + widget = EPHY_DOWNLOAD_WIDGET (object); +/* vim: set sw=2 ts=2 sts=2 et: */ w_download is ugly, just download. @@ +131,3 @@ + + if (widget->priv->download) + g_object_unref (widget->priv->download); this goes in dispose. @@ +146,3 @@ + object_class->set_property = ephy_download_widget_set_property; + object_class->dispose = ephy_download_widget_dispose; + object_class->finalize = ephy_download_widget_finalize; not needed anymore. @@ +158,3 @@ + "The EphyDownload shown by this widget", + G_TYPE_OBJECT, + G_PARAM_READWRITE | read only and construct? @@ +202,3 @@ + icon = gtk_icon_info_load_icon (icon_info, NULL); + + return icon; just return and get rid of the variable? @@ +313,3 @@ + status = webkit_download_get_status (WEBKIT_DOWNLOAD (object)); + + if ((status == WEBKIT_DOWNLOAD_STATUS_FINISHED)) extra () @@ +373,3 @@ + char *basename; + + WebKitDownload *w_download; w_download!! @@ +452,3 @@ + + if (download != NULL) + g_object_add_weak_pointer (G_OBJECT (download), (gpointer *) &widget->priv->download); I think we should just ref here, this owns the download. ::: lib/widgets/ephy-download-widget.h @@ +55,3 @@ +#define EPHY_DOWNLOAD_WIDGET_GET_CLASS(obj) \ + (G_TYPE_INSTANCE_GET_CLASS ((obj), \ + EPHY_TYPE_DOWNLOAD_WIDGET, EphyDownloadWidgetClass)) align properly. @@ +79,3 @@ +EphyDownload *ephy_download_widget_get_download (EphyDownloadWidget *widget); +void ephy_download_widget_set_download (EphyDownloadWidget *widget, + EphyDownload *download); align properly.
[Removing GNOME3.0 target as decided in release-team meeting on March 03, 2011. This report has an "important" categorisation for GNOME3.0 but is not considered a hard blocker. For querying use the corresponding whiteboard entry added.]
Review of attachment 181571 [details] [review]: Hrm, why will it open less windows now?
Review of attachment 181572 [details] [review]: OK.
Review of attachment 181573 [details] [review]: Change the TBD comment ;) ::: src/ephy-session.c @@ -218,0 +220,7 @@ + windows = ephy_session_get_windows (session); + if (windows->data) + { ... 4 more ... As commented on jabber, this does not make much sense. ::: src/ephy-window.c @@ +3131,3 @@ +void +ephy_window_show_downloads_box (EphyWindow *window, + gboolean show) Also as commented, this should be 'set_visisible' or something, since it show/hides.
Review of attachment 181574 [details] [review]: OK.
Review of attachment 181575 [details] [review]: OK; write a bit more of prose in the commit message...
Review of attachment 181576 [details] [review]: This should just be added in whatever patch it uses it first IMHO.
Review of attachment 181578 [details] [review]: OK.
A comment about the code in the branch: - I think the ED should ref the widget/embed, otherwise they can go away randomly and it will just crash. Let's put all the patches ready for download once more in the bug and try to merge this today!
Attachment 181572 [details] pushed as 3454067 - ephy-file-helpers: add uninstalled ui/ in ephy_file Attachment 181578 [details] pushed as ae650f3 - popup-commands: indent fix
Created attachment 182503 [details] [review] ephy-file-helpers: only one downloads_dir function Remove all the ambiguity, we always throw downloads to the same place or the Desktop. Bug #618443
Created attachment 182504 [details] [review] ephy-file-helpers: allow desktop in ephy_file_browse_to Nautilus doesn't draw the desktop as always anymore. It's not visible unless you browse there, so the reason to not allow browse_to desktop is not valid anymore. Bug #618443
Created attachment 182505 [details] [review] ephy-download: add the new EphyDownload object EphyDownload is a wrapper object around WebKitDownload that handles common behavior in downloads: auto-destination, default action for the MIME type. It can be used to wrap a WebKitDownload coming from a WebKitView or to download a url: ephy_download_new_for_uri and ephy_download_new_for_download are provided. Its lifetime is not automagic like EphyEmbedPersist, so you have to unref it when you no longer need it. This new object replaces EphyEmbedPersist and enables us to use a single codepath for downloads in all Epiphany. Bug #618443
Created attachment 182506 [details] [review] ephy-download-widget: new widget A widget showing the progress of an EphyDownload and offering the default set of actions to take on it: Open, Browse to, Cancel. It keeps a ref to the EphyDownload its showing. Bug #618443
Created attachment 182507 [details] [review] ephy-window: add new downloads UI Downloads are shown per-window, imitating Chromium's bottom bar. If the window being closed has active downloads, a warning dialog will be shown just as when forms have been modified but not sent. An active download is any download that is not yet 100% complete. Bug #618443
Review of attachment 182505 [details] [review]: Looks sane to me with those changes. ::: embed/ephy-download.c @@ +385,3 @@ + g_return_if_fail (EPHY_IS_DOWNLOAD (download)); +/* vim: set sw=2 ts=2 sts=2 et: */ + download->priv->window = g_object_ref (window); Leaking the window. @@ +402,3 @@ + g_return_if_fail (EPHY_IS_DOWNLOAD (download)); + + download->priv->widget = widget; should do the same than with window. @@ +631,3 @@ + + if (priv->window) { + g_object_unref (priv->window); Need to set window to NULL, dispose can run more than once.
Review of attachment 182506 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +62,3 @@ + gint error_detail, + char *reason, + EphyDownloadWidget *widget); Can you reorder the code to get rid of those? @@ +127,3 @@ + + if (widget->priv->download) + g_object_unref (widget->priv->download); Have to set to NULL. @@ +138,3 @@ + + G_OBJECT_CLASS (ephy_download_widget_parent_class)->finalize (object); +} Useless finalize is useless.
Review of attachment 182507 [details] [review]: Looks sane.
Created attachment 182515 [details] New downloads UI screenshot This is how it looks.
Created attachment 182516 [details] Warning when closing a window with ongoing downloads When closing a window with *ongoing* downloads, you'll get this.
Created attachment 182528 [details] [review] ephy-download: add the new EphyDownload object EphyDownload is a wrapper object around WebKitDownload that handles common behavior in downloads: auto-destination, default action for the MIME type. It can be used to wrap a WebKitDownload coming from a WebKitView or to download a url: ephy_download_new_for_uri and ephy_download_new_for_download are provided. Its lifetime is not automagic like EphyEmbedPersist, so you have to unref it when you no longer need it. This new object replaces EphyEmbedPersist and enables us to use a single codepath for downloads in all Epiphany. Bug #618443
Created attachment 182529 [details] [review] ephy-download-widget: new widget A widget showing the progress of an EphyDownload and offering the default set of actions to take on it: Open, Browse to, Cancel. It keeps a ref to the EphyDownload its showing. Bug #618443
Created attachment 182530 [details] [review] ephy-window: add new downloads UI Downloads are shown per-window, imitating Chromium's bottom bar. If the window being closed has active downloads, a warning dialog will be shown just as when forms have been modified but not sent. An active download is any download that is not yet 100% complete. Bug #618443
Created attachment 182753 [details] [review] ephy-download: add the new EphyDownload object EphyDownload is a wrapper object around WebKitDownload that handles common behavior in downloads: auto-destination, default action for the MIME type. It can be used to wrap a WebKitDownload coming from a WebKitView or to download a url: ephy_download_new_for_uri and ephy_download_new_for_download are provided. Its lifetime is not automagic like EphyEmbedPersist, so you have to unref it when you no longer need it. This new object replaces EphyEmbedPersist and enables us to use a single codepath for downloads in all Epiphany. Bug #618443
Created attachment 182754 [details] [review] ephy-download-widget: new widget A widget showing the progress of an EphyDownload and offering the default set of actions to take on it: Open, Browse to, Cancel. It keeps a ref to the EphyDownload its showing. Bug #618443
Created attachment 182755 [details] [review] ephy-window: add new downloads UI Downloads are shown per-window, imitating Chromium's bottom bar. If the window being closed has active downloads, a warning dialog will be shown just as when forms have been modified but not sent. An active download is any download that is not yet 100% complete. Bug #618443
Review of attachment 182753 [details] [review]: Let's commit this already! ::: embed/ephy-download.c @@ +410,3 @@ + g_object_unref (download->priv->widget); + + if (widget != NULL) { Really need to assign widget always, even if it's NULL. Same for set_window.
Review of attachment 182754 [details] [review]: Let's go.
Review of attachment 182755 [details] [review]: OK.
Boy, am I happy to close this... Thanks for the reviews Xan :) Attachment 182503 [details] pushed as fdafabd - ephy-file-helpers: only one downloads_dir function Attachment 182504 [details] pushed as 7f7826a - ephy-file-helpers: allow desktop in ephy_file_browse_to Attachment 182753 [details] pushed as b9f9bf1 - ephy-download: add the new EphyDownload object Attachment 182754 [details] pushed as 1463005 - ephy-download-widget: new widget Attachment 182755 [details] pushed as 7401732 - ephy-window: add new downloads UI