Created attachment 49679 [details] [review]
first patch to do this

I haven't added a entry into the preferences dialog for this feature. I can do
it if needed.
So the option must be changed into gconf.

Thanks for the patch!

We're in feature freeze right now so we can't apply this patch at this time.
During the next development cycle we plan to remove the download window into an
external progress notification application; the idea to just show the icon
instead of the window is of course still valid for that.

Maybe we should do this by default, and just draw attention to the tray icon the first time it appears with a balloon tooltip (libnotify) ?

I think if we don't get nautilus integration soon, we should instead use this patch together with libnotify to draw attention to it when downloads are added or finished.

Is this used in Ephy 2.15?
Let's not forget about this one, it's a good idea!

This will be obsolete in 2.17 with nud's work...

Is this still valid/anyone working on this?

I'm all for hiding the downloads window by default. We should add a nice notify in that case. I don't think we are ever getting mathusalem integration...

I assume this doesn't require anything from bug #492351 ?.

Created attachment 100504 [details] [review]
Doesn't open the download window AND shows a nice notification to drag attention about it

This is a first draft of the patch. I tried to implement it so other notifications can be showed (like download complete or sudden failure).

Comments welcome!. I'm looking forward to implement another notification for download complete for bug #492351 (that as far as I see is just another notify call on update_download_row @case EPHY_DOWNLOAD_COMPLETED:)

Oh yes, and I would say that the best behaviour would be to:

(hide = true)
1. Show a notification to drag attention, only once, when the dw is created

and in the other case:

(hide = false)
1. Show the window the first time a download is added, don't steal focus on later additions to it (right now, it always steal focus)

in both cases:
2. If the window is hidden by the user, we should remember this on gconf so next time the dw will start either shown or hidden.

What do you think?

This is really cool!
I think we'll have Mathusalem support, but not too soon...I'm working on it, I've rewritten a saner API, but do not have the time to start coding it yet.
But in the meantime, I agree that this is a good solution.

About the patch:
If it's possible, I think it'd be great if on download complete, if you do not have set to open files automatically, you could pack a button in the notify, asking the user to open it with the default handler application for that file.
Also, I agree with your last comment for the window/notification behaviour.

+1

Created attachment 100549 [details] [review]
Updated patch

This uses a behaviour like the one described on my last comment. It will show a notification window only when dw is set to be hidden. It will act as usual if dw is not set to hidden.
The hidden setting toggles when you manually show or hide the window from the tray.

I also modified show_downloader_cb to use ephy_dialog_* functions, no idea why it was using gtk_* ones.

+++ src/ephy-main.c	(working copy)
@@ -511,6 +511,8 @@ main (int argc,
 
 	g_option_context_set_main_group (option_context, option_group);
 
+	notify_init (PACKAGE);

Can we do this lazily in downloader-view, maybe in its class_init.

+	/* This avoids a "compressed" window when the default was not showing it */
+	gtk_window_set_default_size (GTK_WINDOW (dv->priv->window), 440, 260);

Should be set in the glade file instead.

+					"Cool bubble is here", 
+					"a new download started, but window is hidden.", 

This is not good :)

Created attachment 100559 [details] [review]
Updated again. With better texts and following chpe's comments.

Done!.

Created attachment 100561 [details] [review]
updated quote marks, removed some commented code.

Candidate for commit!

+	notify_uninit ();

Can notify_uninit be called multiple times, and notify used again without notify_init ? Because this is in the finalize handler, with the _init in the class_init.

+	else
+	{
+		return TRUE;
+	}

Just return TRUE, no need for the "else".

+static void
+show_notification_window (DownloaderView *dv)
+{
+	if (gtk_status_icon_is_embedded (dv->priv->status_icon))
+		notify_notification_show (dv->priv->notification, NULL);
+	else
+		g_timeout_add_seconds (1, (GSourceFunc) queue_show_notification, dv);
+}

No timeouts that use |dv| but don't own a reference. Store the returned timeout ID, and remove it if it's still existing in dispose or finalize.

+	/* FIXME: this should be && not ||. It's || for testing the patch only. */
+	if (eel_gconf_get_boolean (CONF_DOWNLOADS_HIDDEN) && !g_value_get_boolean (&visible))

?

+	priv->notification = notify_notification_new ("empty text, ignore", 
+							"empty text, ignore", 

How about NULL (or "" if NULL isn't allowed) ?

Created attachment 100570 [details] [review]
updated

Update following comments.

+		downloading = g_strdup_printf(_("The file “%s” was added to the downloads queue."), ephy_download_get_name (download));

"... has been added..." I think, not "was".

+	priv->notification = notify_notification_new (" ", " ", GTK_STOCK_INFO, NULL);

Does "" or NULL not work here? I think NULL should work.

Once you've fixed these nits, please commit. Thanks!

"" nor NULL work for notify_notification_new():

(epiphany:12736): libnotify-CRITICAL **: notify_notification_update: assertion `summary != NULL && *summary != '\0'' failed

(epiphany:12736): libnotify-CRITICAL **: notify_notification_update: assertion `summary != NULL && *summary != '\0'' failed

Only " " is ok.
Should I report a bug to libnotify?.

Ok to commit only fixing the text?

Yes, I think since one should be able to use NULL or "" here (since this just instantiates the object, and one can set the summary and text later, and hide/show the notification.)

r7802 | diegoe | 2007-12-23 12:36:17 -0500 (Sun, 23 Dec 2007) | 7 lines

download-dialog hidden state is now saved to gconf. New downloads started while 
the dialog is hidden pop-up a notification (as in libnotify).
We now require libnotify to build.

Fixes bug #311209.