GNOME Bugzilla – Bug 720941
A few fixes for the download bar
Last modified: 2014-01-13 23:11:55 UTC
Here are a few fixes for the download bar.
Created attachment 264762 [details] [review] Refactor download widget to split create and populate
Created attachment 264763 [details] [review] Improve the display of remaining time
Created attachment 264764 [details] [review] Separate the icon from the label in the download item
Created attachment 264765 [details] [review] Make download label smaller
Created attachment 264766 [details] [review] Fix download bar close button
Created attachment 264767 [details] [review] Use a menu button for the download item
Review of attachment 264762 [details] [review]: Rest looks fine. ::: lib/widgets/ephy-download-widget.c @@ +404,3 @@ + return; + + if (widget->priv->download != NULL) { You don't need to do this, the download property is construct-only. I'd either remove the construct-only from the property definition, or do this in the same patch that would remove the construct-only flag. @@ +593,3 @@ } + Whitespace change.
Review of attachment 264763 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +109,2 @@ if (hours > 0) { + return g_strdup_printf (ngettext ("%u hour left", "%u hours left", hours), hours); I don't think this is good enough. You'd probably want to round the hours (eg. 2 hours 45 minutes left should read "more than 2 hours left" or "3 hours left". @@ +111,3 @@ } else { if (mins > 0) + return g_strdup_printf (ngettext ("%u minute left", "%u minutes left", mins), mins); Ditto. I think splitting up this code into a cut'n'paste lib would also be useful to share with other programs that can deal with downloads (say, Totem if we implement offline viewing).
Review of attachment 264764 [details] [review]: Looks fine.
Review of attachment 264765 [details] [review]: Looks fine. Would be good to have screenshots to compare before and after all those changes.
Review of attachment 264766 [details] [review]: Looks fine otherwise. ::: src/ephy-window.c @@ +2939,3 @@ gtk_button_set_relief (GTK_BUTTON (close_button), GTK_RELIEF_NONE); + image = gtk_image_new_from_icon_name ("window-close-symbolic", GTK_ICON_SIZE_MENU); Maybe use g_object_new() to do all this in one go?
Review of attachment 264767 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +189,2 @@ download = ephy_download_get_webkit_download (widget->priv->download); + dest = webkit_download_get_destination (download); In which cases would that happen? Is it possible for widget->priv->download to be NULL as well? @@ +309,2 @@ download = ephy_download_get_webkit_download (widget->priv->download); + dest = webkit_download_get_destination (download); Ditto. @@ +537,3 @@ button = totem_glow_button_new (); + menu_button = gtk_menu_button_new (); + gtk_menu_button_set_direction (GTK_MENU_BUTTON (menu_button), GTK_ARROW_UP); Why up, rather than down? Even if the popup will usually be above the button (if the browser is fullscreen, there wouldn't be space to show it below).
Review of attachment 264762 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +404,3 @@ + return; + + if (widget->priv->download != NULL) { I really don't like functions to make assumptions about how they are called, or have to look at other parts of the code to read the function correctly, and it is too easy to change one part of the code and have unintended consequences.
Review of attachment 264763 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +111,3 @@ } else { if (mins > 0) + return g_strdup_printf (ngettext ("%u minute left", "%u minutes left", mins), mins); https://git.gnome.org/browse/telepathy-account-widgets/tree/tp-account-widgets/tpaw-time.c#n82
Review of attachment 264767 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +189,2 @@ download = ephy_download_get_webkit_download (widget->priv->download); + dest = webkit_download_get_destination (download); I think it is null before the destination file is selected or something. NULL is a valid result. As suggested by the docs and the webkit tests. And it happens in practice. It is not possible for download to be null here because this function is only used while the download in in progress.
Created attachment 265625 [details] [review] Refactor download widget to split create and populate
Created attachment 265626 [details] [review] Improve the display of remaining time
Created attachment 265628 [details] screenshot (before)
Created attachment 265629 [details] screenshot (after)
Review of attachment 264764 [details] [review]: OK
Review of attachment 264765 [details] [review]: OK
Review of attachment 264766 [details] [review]: ::: src/ephy-window.c @@ +2939,3 @@ gtk_button_set_relief (GTK_BUTTON (close_button), GTK_RELIEF_NONE); + image = gtk_image_new_from_icon_name ("window-close-symbolic", GTK_ICON_SIZE_MENU); Yes, you could do this if you want.
Attachment 264764 [details] pushed as 6b7999b - Separate the icon from the label in the download item Attachment 264765 [details] pushed as a2cb59a - Make download label smaller Attachment 264766 [details] pushed as e0bf405 - Fix download bar close button Attachment 264767 [details] pushed as f24d612 - Use a menu button for the download item Attachment 265625 [details] pushed as b390094 - Refactor download widget to split create and populate Attachment 265626 [details] pushed as 08aa34f - Improve the display of remaining time
*** Bug 720895 has been marked as a duplicate of this bug. ***