hmm, it seems it also broke the menu from appearing...
I've reverted that commit and it worked fine.

Cosimo, what was the motivation of that commit? Particularly, you mentioned 'This makes key activation working again.' What is this?

(In reply to comment #1)
> hmm, it seems it also broke the menu from appearing...
> I've reverted that commit and it worked fine.
> 
> Cosimo, what was the motivation of that commit? Particularly, you mentioned
> 'This makes key activation working again.' What is this?

The motivation of that commit is previously we were effectively adding a GtkWindow (GtkDialog is a subclass of GtkWindow) into another GtkWindow (the GtkApplicationWindow the code was creating), which is in general a really bad idea, and makes key activation of default widgets not work correctly.

I would be fine with the patch you propose here, and I don't really know why using a GtkDialog breaks the app menu here - could be a bug in either gnome-shell or GTK.

Another approach is to make screenshot-interactive-dialog.c use the same UI, but with an actual GtkApplicationWindow. You would have to replicate the button/content area/response logics that are used there though, which would be a bit tedious.

Created attachment 248410 [details] [review]
proposed patch

ok. I turned it a full gtkapplication, that mimics the current behavior, i.e., it doesn't have any regressions.

the idea is to backport this to 3.8 as well.

Review of attachment 248410 [details] [review]:

Thanks for the patch! This looks great in general, except for the comment below. Could you also do a similar treatment to the result dialog? (the one in screenshot-dialog.c/screenshot-dialog.ui)

::: src/screenshot-interactive-dialog.c
@@ +483,3 @@
+  data->user_data = user_data;
+  g_signal_connect (button, "clicked", G_CALLBACK (capure_button_clicked_cb), data);
+  gtk_container_add (GTK_CONTAINER (button_box), button);

You want to add

gtk_widget_set_can_default (button, TRUE);
gtk_widget_grab_default (button);

here. Note that it needs to be after the gtk_container_add() call, as gtk_widget_grab_default() requires the default widget to be inside a GtkWindow at the moment of the call.

*** Bug 703232 has been marked as a duplicate of this bug. ***

Created attachment 248617 [details] [review]
proposed patch, v2

this is the first patch with the changes you proposed (to make the button default).

Created attachment 248618 [details] [review]
proposed patch

this is the second patch (I prefer to split them in 2), which covers the screenshot-dialog.[ui,c,h].

I've rewritten the .ui file using gtkgrid instead of obsolete [vh]box. also it's now a gtkwindow instead of a dialog.

I've tested both patches here in master and 3.8, worked fine.

Review of attachment 248617 [details] [review]:

Looks good, thanks.

Review of attachment 248618 [details] [review]:

Looks good to me, with these two smaller details fixed - thanks!

::: src/screenshot-dialog.h
@@ +24,3 @@
 
+typedef enum {
+  ScreenshotResponseSave,

I'd rather these being all capitals, like GtkResponseType.

::: src/screenshot-dialog.ui
@@ +2,3 @@
 <interface>
+  <!-- interface-requires gtk+ 3.8 -->
+  <object class="GtkWindow" id="toplevel">

Can you make this a GtkApplicationWindow instead?

(In reply to comment #9)
> 
> ::: src/screenshot-dialog.ui
> @@ +2,3 @@
>  <interface>
> +  <!-- interface-requires gtk+ 3.8 -->
> +  <object class="GtkWindow" id="toplevel">
> 
> Can you make this a GtkApplicationWindow instead?

hmm, I can edit the file by hand, but glade is unable to read it. (it works fine at runtime however). what do you think?

(In reply to comment #10)

> hmm, I can edit the file by hand, but glade is unable to read it. (it works
> fine at runtime however). what do you think?

Maybe Glade doesn't know about GtkApplicationWindow yet? If it works fine at runtime, please edit it by hand then...

thanks for the review. pushed to master and 3.8.
also filled bug 703801.

btw cosimo, do you think it's worth a 3.8.3 release?

Thanks John. I will probably release a 3.8.3 later this week.