Created attachment 266689 [details] [review]
proposed patch

Hi, I'm new to Epiphany development. I proposed this patch yesterday, but I found some things that must be fixed (style issues and message must be free'd). I will submit an updated patch later.

Created attachment 266736 [details] [review]
updated patch

Does filename not need to be freed as well?

Created attachment 266754 [details] [review]
third version

yup, you're right. Fixed :)

Review of attachment 266754 [details] [review]:

I think this should be using GNotification.

Created attachment 268077 [details] [review]
patch

Now using GNotification :)

Review of attachment 268077 [details] [review]:

Patch looks fine but needs a bit of work. Any comments on the UI-side, Jon?

::: lib/widgets/ephy-download-widget.c
@@ +288,3 @@
 widget_finished_cb (WebKitDownload *download,
                     EphyDownloadWidget *widget)
 {

Considering that this method is rather compact I'd vouch for doing this in a separate method instead of cluttering this one.

@@ +291,3 @@
+  GNotification *notification;
+  GtkWindow *toplevel;
+  GtkApplication *application;

Use GApplication instead.

@@ +297,3 @@
+
+  toplevel = GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (widget)));
+  application = GTK_APPLICATION (gtk_window_get_application (toplevel));

This casting seems unneeded. You could actually cast to a GApplication here and remove the cast below.

Created attachment 268637 [details] [review]
new patch

Okay, done :)

Review of attachment 268637 [details] [review]:

The patch looks correct to me, but I'm wondering whether it's enough. Jon, what do you think?

Review of attachment 268637 [details] [review]:

I see it's using a placeholder light bulb icon.  Should probably be an Epiphany icon instead. (I'm referring to the icon displayed in the body of the notification, not the program icon that's displayed in the notification area.)

P.S. This might be a good item for a feature freeze break request: small, self-contained, and darn useful.

Still waiting for Jon's comments :)

I'm currently traveling and only have a netbook available which does not have the resources to jhbuild epiphany or webkit. So if somebody wants to include this right now, please use my code and add an icon :) Otherwise I will add a new patch in April.

Review of attachment 268637 [details] [review]:

Which April? :)

::: lib/widgets/ephy-download-widget.c
@@ +287,2 @@
 static void
+display_download_finished_notification (WebKitDownload *download,

This feels like functionality that belongs in the EphyDownload class instead.

@@ +294,3 @@
+  char *message;
+  char *filename;
+  const char *dest;

Half of these variables should be declared in conditional below, since they're not needed outside the conditional.

@@ +302,3 @@
+  if (!gtk_window_is_active (toplevel) && dest != NULL) {
+    filename = g_filename_display_basename (dest);
+    message = g_strdup_printf (_("Finished downloading %s"), filename);

Please add a one-line comment above both translatable strings, for the translators.

@@ +308,3 @@
+    g_free (message);
+    g_application_send_notification (application, "download-finished", notification);
+    g_object_unref (notification);

This would be easier to read if you put the g_free and g_object_unref all at the bottom of the conditional, separated from the rest of the code by a blank line.

Hey, saw this bug in a blog post quite some time ago. I would like to start contributing to GNOME, and this looks like quite a good place to start. I'm not new to programming, I have experience in C#, Javascript and some Python, but I'm eager to learn C, which hopefully won't be hard.

With your permission, I'd like to finish and send in the patch for review. If you have any tips for a newbie like me, chuck 'em here, it'll be greatly appreciated!

Hi Juraj, if you know so many other languages, I think you won't find C to be hard... though it's probably not as fun as those languages. I'm eagerly awaiting your patch. Thanks!

OK, so I'm starting to get my hands dirty. I still haven't quite got the hang of C yet, like what are all those asterisks doing all over the place or how you access stuff from different files, but I'm slowly getting the hang of it.

(In reply to Michael Catanzaro from comment #14)
> ::: lib/widgets/ephy-download-widget.c
> @@ +287,2 @@
>  static void
> +display_download_finished_notification (WebKitDownload *download,
> 
> This feels like functionality that belongs in the EphyDownload class instead.

Should I move it there, or just rename it?
 
> @@ +302,3 @@
> +  if (!gtk_window_is_active (toplevel) && dest != NULL) {
> +    filename = g_filename_display_basename (dest);
> +    message = g_strdup_printf (_("Finished downloading %s"), filename);
> 
> Please add a one-line comment above both translatable strings, for the
> translators.

I can't find any example of this. What should the comment look like?

Thanks.

(In reply to Juraj Fiala from comment #17)
> Should I move it there, or just rename it?

You'll want to move it. Add the new function in embed/ephy-download.c and call it from download_finished_cb. You won't have access to an EphyDownloadWidget from there, so the function should take only the WebKitDownload parameter. To get the GApplication without the EphyDownloadWidget, you can remove these lines:

toplevel = GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (widget)));
application = G_APPLICATION (gtk_window_get_application (toplevel));

And replace them with this:

application = G_APPLICATION (ephy_embed_shell_get_default ());
toplevel = gtk_application_get_active_window (GTK_APPLICATION (application));

The casts are how we switch between different levels of inheritance.

> I can't find any example of this. What should the comment look like?

Something like this:

+  if (!gtk_window_is_active (toplevel) && dest != NULL) {
+    filename = g_filename_display_basename (dest);
+    /* Translators: a desktop notification when a download finishes. */
+    message = g_strdup_printf (_("Finished downloading %s"), filename);

Created attachment 312208 [details] [review]
Quick patch before trying to build the project and sending in a proper one

Hey, so I made all of the proposed changes. I haven't built epiphany with this patch yet, and I wanted to send it in before I try building it (which will probably take several hours) for any possible style mistakes, reorderings, blank lines needed, etc.

Review of attachment 312208 [details] [review]:

Thanks for your patch.

It might take a few hours to build all the dependencies of Epiphany (in particular, WebKit), but Epiphany itself should only take a minute or two to build. Then if the build fails, you won't have to rebuild the portion that succeeded already, so it's always best to build your patch before submitting it.

Take a look at https://wiki.gnome.org/Newcomers/BuildGnome for instructions on how to build. Basically, you want to install jhbuild following the instructions on that page, then run:

jhbuild sysdeps --install epiphany
jhbuild build epiphany

After you've built all the dependencies once, you'll usually want to rebuild without building dependencies or pulling newer code:

jhbuild buildone -n epiphany

::: embed/ephy-download.c
@@ +533,3 @@
+    char *filename;
+    char *message;
+    filename = g_filename_display_basename (dest);

A style nit: it's preferred to leave a blank line between the last declaration and the first line of executable code, e.g.:

char *message;

filename = g_filename_display_basename (dest);

@@ +535,3 @@
+    filename = g_filename_display_basename (dest);
+    message = g_strdup_printf (_("Finished downloading %s"), filename);
+    /* Translators: a desktop notification when a download finishes. */

Move the translator comment up above the string to be translated (i.e. one line higher).

@@ +536,3 @@
+    message = g_strdup_printf (_("Finished downloading %s"), filename);
+    /* Translators: a desktop notification when a download finishes. */
+    g_free (filename);

A style nit: free this down below, before message is freed.

::: lib/widgets/ephy-download-widget.c
@@ +301,3 @@
                     EphyDownloadWidget *widget)
 {
+  display_download_finished_notification (download, widget);

This won't build; you'll need to call this from download_finished_cb in ephy-download.c. display_download_finished_notification is marked static, which means it's only available to the source file it's declared in. If you want to use it from another file, you should declare it in a header file instead. (But in this case, it is only needed in ephy-download.c.)

Note also that you changed the function to take only one parameter, so you have to call it accordingly:

display_download_finished_notification (wk_download);

(In reply to Michael Catanzaro from comment #20)
> Take a look at https://wiki.gnome.org/Newcomers/BuildGnome for instructions
> on how to build. Basically, you want to install jhbuild following the
> instructions on that page, then run:
> 
> jhbuild sysdeps --install epiphany
> jhbuild build epiphany

Already read the article and done this, currently at component 44 out of 49, with webkit yet to come.

> ::: embed/ephy-download.c
> @@ +533,3 @@
> +    char *filename;
> +    char *message;
> +    filename = g_filename_display_basename (dest);
> 
> A style nit: it's preferred to leave a blank line between the last
> declaration and the first line of executable code, e.g.:
> 
> char *message;
> 
> filename = g_filename_display_basename (dest);

Was looking exactly for these types of comments, thanks!

> @@ +535,3 @@
> +    filename = g_filename_display_basename (dest);
> +    message = g_strdup_printf (_("Finished downloading %s"), filename);
> +    /* Translators: a desktop notification when a download finishes. */
> 
> Move the translator comment up above the string to be translated (i.e. one
> line higher).

I guess the "check thrice" rule applies perfectly here.

> ::: lib/widgets/ephy-download-widget.c
> @@ +301,3 @@
>                      EphyDownloadWidget *widget)
>  {
> +  display_download_finished_notification (download, widget);
> 
> This won't build; you'll need to call this from download_finished_cb in
> ephy-download.c. display_download_finished_notification is marked static,
> which means it's only available to the source file it's declared in. If you
> want to use it from another file, you should declare it in a header file
> instead. (But in this case, it is only needed in ephy-download.c.)

Whoops, didn't notice download_finished_cb wasn't the original method. I need to pay more attention to this kind of stuff. Also, thought static made it available from every file. Noted. Thanks.

> Note also that you changed the function to take only one parameter, so you
> have to call it accordingly:
> 
> display_download_finished_notification (wk_download);

Gosh, what a stupid mistake. I guess I'm too used to source analysis, heh.

Thanks, will post a proper, tested patch next time.

Created attachment 312235 [details] [review]
Added notification to indicate finished download

If the toplevel window is not active, display a notification with the
file name included.

OK, I must be missing the obvious in this patch. I trust the code before to work correctly, so I must have made some kind of mistake. No errors are thrown, but no notification is thrown either. I'll try to look into it later.

Review of attachment 312235 [details] [review]:

I think it's because our desktop file name does not match our GtkApplication ID. I'll work on trying to fix that; we were going to have to do so eventually regardless....

::: embed/ephy-download.c
@@ +520,2 @@
 static void
+display_download_finished_notification (WebKitDownload *download)

I would move this entire function down below, to just above download_finished_cb. It's better style to clump related functions together. In particular, it doesn't make much sense to put any function in between acquire_session_inhibitor() and release_session_inhibitor().

(In reply to Michael Catanzaro from comment #24)
> I think it's because our desktop file name does not match our GtkApplication
> ID. I'll work on trying to fix that; we were going to have to do so
> eventually regardless....

Did you test the patch? I think I messed up compiling my environment. I can't get atk to compile, no matter what I do, keeps throwing a KeyError exception, looks like a programming error. And when I fire up Epiphany with jhbuild I keep getting

Gtk-Message: Failed to load module "canberra-gtk-module"

errors, like twenty or so of them. And I even tried removing the whole "if" part so it went directly for the notification and nothing.

> ::: embed/ephy-download.c
> @@ +520,2 @@
>  static void
> +display_download_finished_notification (WebKitDownload *download)
> 
> I would move this entire function down below, to just above
> download_finished_cb. It's better style to clump related functions together.
> In particular, it doesn't make much sense to put any function in between
> acquire_session_inhibitor() and release_session_inhibitor().

Put it there because I thought they were sorted alphabetically, didn't notice I put it between two related functions. Makes sense. On it.

Created attachment 312242 [details] [review]
Added notification to indicate finished download

If the toplevel window is not active, display a notification with the
file name included.

Sorry, learning the hard way.

Created attachment 312243 [details] [review]
Added notification to indicate finished download

If the toplevel window is not active, display a notification with the
file name included.

(In reply to Juraj Fiala from comment #25)
> Did you test the patch?

Yeah, it didn't work, because GNotification requires the GtkApplication ID to match the desktop file name. I am going to try renaming the desktop file to org.gnome.epiphany.desktop.

> I think I messed up compiling my environment. I
> can't get atk to compile, no matter what I do, keeps throwing a KeyError
> exception, looks like a programming error.

I'm working on fixing this now. It's caused by the latest commit in gobject-introspection.

> And when I fire up Epiphany with
> jhbuild I keep getting
> 
> Gtk-Message: Failed to load module "canberra-gtk-module"
> 
> errors, like twenty or so of them. And I even tried removing the whole "if"
> part so it went directly for the notification and nothing.

You can ignore those warnings, or 'jhbuild build libcanberra' to get rid of them. The warning means applications you run in jhbuild won't be able to play sounds.

(In reply to Michael Catanzaro from comment #29)
> I'm working on fixing this now. It's caused by the latest commit in
> gobject-introspection.

I couldn't figure out how to fix it so I just reverted the commit. The problem should go away after you 'jhbuild buildone gobject-introspection'.

(In reply to Michael Catanzaro from comment #30)
> (In reply to Michael Catanzaro from comment #29)
> > I'm working on fixing this now. It's caused by the latest commit in
> > gobject-introspection.
> 
> I couldn't figure out how to fix it so I just reverted the commit. The
> problem should go away after you 'jhbuild buildone gobject-introspection'.

Thanks.

Btw, until you figure out what's wrong with the desktop file, could you point me to another bug I can try to fix? I don't mind reading documentation or stuff,  I'm quite eager to continue contributing to GNOME, and I don't want to pointlessly spend my train trips looking out of the window ;)

Maybe bug #109343?

Thanks, will try. Ooh, 12 years old. And I though this one was old, haha.

Created attachment 312248 [details] [review]
Rename the desktop file

This allows us to use GNotification. It is also step #1 to D-Bus
activation.

Still have not managed to get a notification to actually appear. I am thinking we should maybe give up and use libnotify, since that actually works....

I dunno, my young, idealogical and energetic self is telling me that we should keep hacking on this and do it proper, but if that's the better and easier way to do it then so be it.

So? Should I revert to libnotify?

Let me first CC a couple people who might know what's up with the GNotification. I don't care to track down the issue through all the D-Bus calls, so if we can't figure out how to make the GNotification work, switching to libnotify is fine by me. We already depend on libnotify through WebKit anyway, so there's not really any significant value in trying to avoid it here....

So the problem here is that we don't know whether these patches work or not. It may be that GApplication notifications just do not work in jhbuild, or there may be an error in the patches. I will try to build a updated Fedora package to test with. If it doesn't work, we will just have to add an explicit dependency on libnotify, as GApplication notifications are not usable if nobody can figure out how to work them. We already depend on libnotify through WebKit anyway.

Did you manage to get it to work?

Nope, will try to find time for this next cycle....

(In reply to Michael Catanzaro from comment #40)
> Nope, will try to find time for this next cycle....

(Back on my radar.)

Review of attachment 312243 [details] [review]:

This patch is going to need rebased at this point.

Created attachment 366415 [details] [review]
Add download finished notification

So, two years after I’ve piped up the patch is finally working and ready.

Hurray?

Yeah, definitely hurray! This is my first real code patch for GNOME, of course it’s hurray! Sorry for the wait though.

Looks good, thanks!

Attachment 366415 [details] pushed as 954a14d - Add download finished notification

Ah, there's one problem: we need to find a way to not show the notification when downloading adblock filters! (bug #776682)

The following fix has been pushed:
f267318 Don't notify after downloading adblock filters

Created attachment 366440 [details] [review]
Don't notify after downloading adblock filters

"Finished downloading 17ba74de8f13543dbff29e37b3ce125d! Yippee!"
  -- Every user ever

Oh man, there’s always something :D

Just checking, you fixed it with the above commit, right?

Yup, we're good now!