Created attachment 257477 [details] [review]
Use GtkMenuTracker for Quartz backend

Review of attachment 257477 [details] [review]:

::: gtk/gtkicontheme.h
@@ -268,2 +268,4 @@
 GDK_AVAILABLE_IN_ALL
 GdkPixbuf *           gtk_icon_info_get_builtin_pixbuf (GtkIconInfo   *icon_info);
+GDK_AVAILABLE_IN_3_12
+gboolean              gtk_icon_info_is_symbolic        (GtkIconInfo   *icon_info);

No clue about the OSX stuff, but this public API should be introduced in its own commit

Review of attachment 257477 [details] [review]:

I agree with pbor about the icon info api.

I also wonder what the gtkapplicationwindow.c changes are about - can you explain that ?

Created attachment 264316 [details] [review]
Fix GtkApplicationWindow action group implementation

GtkApplicationWindow frees its internal action group on dispose for the
usual reasons: to avoid the possibility of reference cycles caused by
actions referring back to the window again.

Unfortunately, if it happens to be inside of a GtkActionMuxer at the
time that it is disposed, it will (eventually) be removed from the muxer
after it has been disposed.  Removing an action group from a muxer
involves a call to g_action_group_list_actions() which will crash
because the internal action group to which we normally delegate the call
has been freed.

A future patch that reworks the quartz menu code will introduce a use of
GtkActionMuxer in a way that causes exactly this problem.

We can guard against the problem in a number of ways.

First, we can avoid the entire situation by ensuring that we are removed
from the muxer before we destroy the action group.  To this end, we
delay destruction of the action group until after the chain-up to the
dispose of GtkWindow (which is where the window is removed from the
GtkApplication).

Secondly, we can add checks to each of our GActionGroup and GActionMap
implementation functions to check that the internal action group is
still alive before we attempt to delegate to it.

We have to be careful, though: because our _list_actions() call will
suddenly be returning an empty list, people watching the group from
outside will have expected to see "action-removed" calls for the
now-missing items.  Make sure we send those. but only if someone is
watching.

Created attachment 264317 [details] [review]
GtkIconInfo: add gtk_icon_info_is_symbol()

Created attachment 264318 [details] [review]
Use GtkMenuTracker for Quartz backend

I've broken this up into 3 patches to address the concerns in the bug, along with providing a better explanation (and alternate approach) for the magic in gtkapplicationwindow.c.

The patches have been rebased on top of the GtkApplication refactor in bug 720550 and modified somewhat as a result.

Review of attachment 264316 [details] [review]:

::: gtk/gtkapplicationwindow.c
@@ +814,3 @@
+
+          g_strfreev (action_names);
+        }

I'm not a fan of this - doint lots of extra work in dispose is not great. *If* somebody is listening for these signals, they are probably going to do still more pointless work.

I don't see GSimpleActionGroup emit any remove-action signals in finalize, either.

Review of attachment 264317 [details] [review]:

::: gtk/gtkicontheme.c
@@ +3572,3 @@
+  return is_symbolic;
+}
+

This function really shows how shaky our definition and handling of symbolic icons really is - its all in a filename. At the very least, the docs should explain that event if this function returns TRUE, the rendered icon may still not appear 'symbolic' (which is never defined anywhere, btw), in the case of fallback.

Review of attachment 264318 [details] [review]:

Overall, looks like an improvement to me.

::: gtk/gtkapplication-quartz-menu.c
@@ +246,3 @@
+
+  if (error != NULL)
+    g_error_free (error);

Why pass a non-NULL GError in the first place, if we're throwing it away here ?

@@ +267,3 @@
+    [item reset];
+
+  if (error != NULL)

Same question here

@@ +287,3 @@
+
+  if (error != NULL)
+    g_error_free (error);

And here

@@ +329,3 @@
+    [item didChangeToggled];
+  else if (name == accel)
+    [item didChangeAccel];

Why the statics ? Seems like an entirely unnecessary optimization to me. I'd rather write

if (name == I_("label"))
  ...
else if (name == I_("icon"))
  ...

::: gtk/gtkapplication-quartz.c
@@ +162,3 @@
+      /* We must preserve the rule that index 0 is the app menu */
+      empty = g_menu_new ();
+      g_menu_prepend_submenu (quartz->combined, "`gtk-private-appname`", G_MENU_MODEL (empty));

The gtk-private-appname hack is worth a comment here.

@@ +175,3 @@
+  /* If we have the menubar, it is a section at index '1' */
+  if (g_menu_model_get_n_items (G_MENU_MODEL (quartz->combined)) > 1)
+    g_menu_remove (quartz->combined, 1);

Its a bit awkward having to add comments about index 0 and index 1 throughout the source.
How about 

#define APP_MENU_INDEX 0
#define MENUBAR_INDEX  1

?

(In reply to comment #8)
> I don't see GSimpleActionGroup emit any remove-action signals in finalize,
> either.

finalize and dispose are two different things, though.  By the time finalize is called -- by definition -- nobody cares about it.  With dispose, on the other hand, there may still well be people holding refs and signal connections (like action muxers).  They have no idea that the object was just disposed, nor can they know (because "dispose" is not a signal on GObject).  To these outsider observers, the interface has just been suddenly violated for no good reason.

I think it's quite rare that we will actually hit that block of code, and indeed I reorganised that function specifically so that we would not hit it in the muxer case.  I added a g_assert_not_reached() here and it didn't hit until I added a separate watch.  If someone else is watching the action group, though, I believe this to be necessary to be strictly correct.

The only other option would be to keep the actions around as Will did in his patch (which would still be technically incorrect unless we also kept their states and enable flags around as well) or to preserve the action group, which runs the risk of creating reference cycles.

All that said, we could just drop this and as long as the action group is disposed after the chain-up we won't notice any problems inside of Gtk itself...  I'd like to think that we're implementing the interface correctly for the sake of outside observers as well, though.

(In reply to comment #9)
> This function really shows how shaky our definition and handling of symbolic
> icons really is - its all in a filename. At the very least, the docs should
> explain that event if this function returns TRUE, the rendered icon may still
> not appear 'symbolic' (which is never defined anywhere, btw), in the case of
> fallback.

No comment on this.  I don't know anything about symbolic icons.

(In reply to comment #10)
> Why pass a non-NULL GError in the first place, if we're throwing it away here > Same question here
> And here

I agree.  I also find that to be strange.

> 
> @@ +329,3 @@
> +    [item didChangeToggled];
> +  else if (name == accel)
> +    [item didChangeAccel];
> 
> Why the statics ? Seems like an entirely unnecessary optimization to me. I'd
> rather write
> 
> if (name == I_("label"))
>   ...
> else if (name == I_("icon"))
>   ...

This is definitely not a good approach.  If we wanted to avoid the statics, a simple string compare would be much faster than acquiring a lock then hashing and attempting to intern the string each time.

I don't have a strong opinion between the current code and just using g_str_equal(), but I guess I'd lean toward the g_str_equal().

> ::: gtk/gtkapplication-quartz.c
> @@ +162,3 @@
> +      /* We must preserve the rule that index 0 is the app menu */
> +      empty = g_menu_new ();
> +      g_menu_prepend_submenu (quartz->combined, "`gtk-private-appname`",
> G_MENU_MODEL (empty));
> 
> The gtk-private-appname hack is worth a comment here.

This made it into this patchset by accident.  This should be part of bug 720552, but the real truth is that it doesn't seem to matter what we put here at all.  The OS shell always changes the name of the fist menu to be the application name.

> 
> @@ +175,3 @@
> +  /* If we have the menubar, it is a section at index '1' */
> +  if (g_menu_model_get_n_items (G_MENU_MODEL (quartz->combined)) > 1)
> +    g_menu_remove (quartz->combined, 1);
> 
> Its a bit awkward having to add comments about index 0 and index 1 throughout
> the source.
> How about 
> 
> #define APP_MENU_INDEX 0
> #define MENUBAR_INDEX  1
> 
> ?

Maybe.  It's slightly complicated by the fact that, at first, there are no items in the menu and then after that there are either 0 or 1, depending on if there is a menubar or not.

I'll give Will a chance to comment on some of the above issues before proceeding.

(In reply to comment #11)

[...]

> All that said, we could just drop this and as long as the action group is
> disposed after the chain-up we won't notice any problems inside of Gtk
> itself...  I'd like to think that we're implementing the interface correctly
> for the sake of outside observers as well, though.

I understand the point about the interface contract and outside observers.
But: if the code is basically never getting hit in real life, it is basically dead code - we would need a testcase to exercise it to ensure it stays working.

(In reply to comment #12)
> I understand the point about the interface contract and outside observers.
> But: if the code is basically never getting hit in real life, it is basically
> dead code - we would need a testcase to exercise it to ensure it stays working.

I agree.  I'll include one in the next round of patches.

Created attachment 265501 [details] [review]
Add gtk_quartz_g_icon_get_ns_image_{async,finish}.

Created attachment 265503 [details] [review]
Use GtkMenuTracker for Quartz backend.

Created attachment 265622 [details] [review]
app window: test actiongroup across destroy

Make sure that we don't violate the interface contract of GActionGroup
just because gtk_widget_destroy() was called.

Review of attachment 265501 [details] [review]:

::: gtk/gtkquartz.c
@@ +24,3 @@
 #include <gdk/quartz/gdkquartz.h>
 
+#define BUFFER_SIZE 1024

Isn't this a bit tiny by todays standards ?
I think we should read at least a page worth of data at a time.
gdk-pixbuf is using 64k buffers (on the stack) in some places.

Review of attachment 265503 [details] [review]:

::: gtk/gtkapplication-quartz-menu.c
@@ +125,3 @@
+  if ((self = [super initWithTitle:@""
+                            action:@selector(didSelectItem:)
+                     keyEquivalent:@""]) != nil)

This looks very hard to read to me.
Is this an assignment hidden inside a condition ?
I generally prefer those to be out in the open.

@@ +179,3 @@
+          c = [label characterAtIndex:i];
+        }
+

Looks like this is trying to do the same job as _gtk_toolbar_elide_underscores - only not quite as well. the toolbar function also deals with the trailing (_F) convention that is used for some non-latin languages. Might be better to just use that function here.

@@ +205,3 @@
+                                            cancellable,
+                                            icon_loaded,
+                                            self);

Looking at the line lengths in the rest of the file, I would have probably just tried to cram this all on one line.

::: gtk/gtkquartz.c
@@ +35,3 @@
+ */
+unichar
+gtk_quartz_get_key_equivalent (guint key)

Wouldn't this be better off as a utility function in gdk/quartz/gdkquartz.h ?

Review of attachment 265622 [details] [review]:

this seems to be missing gtkapplicationwindow.c

Created attachment 265623 [details] [review]
app window: test actiongroup across destroy

Oops :)

Review of attachment 265623 [details] [review]:

looks good

Attachment 264316 [details] pushed as 2fd307f - Fix GtkApplicationWindow action group implementation
Attachment 265623 [details] pushed as 99ebb1c - app window: test actiongroup across destroy

Created attachment 265666 [details] [review]
GtkIconInfo: add gtk_icon_info_is_symbolic()

Created attachment 265667 [details] [review]
Add gtk_quartz_g_icon_get_ns_image_{async,finish}.

Created attachment 265668 [details] [review]
Use GtkMenuTracker for Quartz backend.

Created attachment 265669 [details] [review]
Add icon menu to bloatpad.

Created attachment 265670 [details] [review]
Add icon menu to bloatpad.

Removed adding GEmblem directly as an icon (because that doesn't make
sense).

Review of attachment 265667 [details] [review]:

::: gtk/gtkquartz.c
@@ +390,3 @@
+  while (G_IS_EMBLEM (icon) || G_IS_EMBLEMED_ICON (icon))
+    {
+      if (G_IS_EMBLEM (icon))

you shouldn't attempt to deal with GEmblem here... it's not meant to be an icon in its own right... I have no idea why it implements GIcon to begin with...

Just do

  while (G_IS_EMBLEMED_ICON (icon))
    icon = g_emblemed_icon_get_icon (G_EMBLEMED_ICON (icon));

@@ +398,3 @@
+    }
+
+  if (G_IS_FILE_ICON (icon))

you should have a special case for GBytesIcon.  People are likely to get these from resources, so it'll be a common case.  Having to pull that, asynchronously through the GLoadableIcon interface would be extremely painful.

Attachment 265666 [details] pushed as fd13713 - GtkIconInfo: add gtk_icon_info_is_symbolic()
Attachment 265670 [details] pushed as 6b865d5 - Add icon menu to bloatpad.

Review of attachment 265668 [details] [review]:

This no longer cleanly applies after bug 720551.

Also: Matthias's comments above still apply.

Review of attachment 265668 [details] [review]:

Also: it might be interesting to use  menuNeedsUpdate and menuDidClose from the NSMenuDelegate protocol to call gtk_menu_tracker_item_request_submenu_shown():

https://developer.apple.com/library/mac/documentation/cocoa/reference/NSMenuDelegate_Protocol/Reference/Reference.html

this could potentially interact badly with the problems we've been talking about in bug 704374 so we might want to add idle dispatches in some places.... and that could have interesting interactions with the menuNeedsUpdate stuff I just mentioned...

Created attachment 265742 [details] [review]
Add gtk_quartz_g_icon_get_ns_image_{async,finish}.

Created attachment 265743 [details] [review]
Move get_key_equivalent() to gdk quartz utils.

Created attachment 265744 [details] [review]
Use GtkMenuTracker for Quartz backend.

Review of attachment 265743 [details] [review]:

This isn't much of a "move"... was it your intention to modify the existing code to use it from its new location, or just to keep two copies in order to simplify things?  Either is fine, but the commit message should probably reflect that.

Review of attachment 265742 [details] [review]:

::: gtk/gtkquartz.c
@@ +186,3 @@
+
+static void
+gtk_quartz_g_input_stream_read_async (GInputStream *stream,

...odd function name

@@ +208,3 @@
+
+static void
+gtk_quartz_g_loadable_icon_get_ns_image_finish (GObject      *object,

see comment below about function name

@@ +219,3 @@
+  g_return_if_fail (G_IS_TASK (task));
+
+  if (!G_IS_LOADABLE_ICON (object))

why would this ever happen?  loadable icons are the only ones we try to async load, no?

@@ +230,3 @@
+  stream = g_loadable_icon_load_finish (icon, result, NULL, &error);
+
+  if (!G_IS_INPUT_STREAM (stream))

use a NULL check here instead.

@@ +242,3 @@
+
+static void
+gtk_quartz_g_loadable_icon_get_ns_image_async (GLoadableIcon *icon,

this should not really be called _async unless it has the common async pattern... (ie: taking a GAsyncReadyCallback, etc.)

@@ +252,3 @@
+  data = g_task_get_task_data (task);
+
+  if (!G_IS_LOADABLE_ICON (icon))

ditto with comment above.  this is dead code.

if you want to do paranoid checking here, use g_return_if_fail() or g_assert()

@@ +260,3 @@
+    }
+
+  if (GDK_IS_PIXBUF (icon))

seems like pixbuf ought to be a toplevel case below instead of a special case of loadable handled here (just like we special-case bytes).  this code here could be split into a separate pixbuf function...

@@ +283,3 @@
+
+static void
+gtk_quartz_gtk_icon_info_get_ns_image_finish (GObject      *object,

calling this _finish is really confusing because we usually reserve that for the _finish functions that go along with the public _async ones.  This is better named _complete or _done or something along those lines...

@@ +311,3 @@
+  pixbuf = gtk_icon_info_load_symbolic_finish (info, result, NULL, &error);
+
+  if (!G_IS_LOADABLE_ICON (pixbuf))

use NULL check here instead and divert to pixbuf-loading branch (mentioned below).

@@ +359,3 @@
+      if (!parsed)
+        {
+          gdk_rgba_parse (&foreground, BLACK);

Add a comment here about how these values were taken to match the ones Adwaita uses and how we don't want to use the theme to decide these values because if we have a dark theme, it won't make sense (with the light menubar of macos).

@@ +457,3 @@
+                      GdkPixbuf *pixbuf = gtk_icon_info_get_builtin_pixbuf (info);
+
+                      if (G_IS_LOADABLE_ICON (pixbuf))

you know that this is a GdkPixbuf... why do you do more RTTI on it?

@@ +458,3 @@
+
+                      if (G_IS_LOADABLE_ICON (pixbuf))
+                        gtk_quartz_g_loadable_icon_get_ns_image_async (G_LOADABLE_ICON (pixbuf), task);

as mentioned above, split out the pixbuf-handling case from the general-purpose loadable case...

Review of attachment 265742 [details] [review]:

On second thought and after a lot of discussion, this is all just too complicated.

It also has a lot of problems: not least of which is what happens if we try to pass a file format understood by GdkPixbuf but not NSImage (like svg...)

I think we decided that it is better to just always load pixbufs via GtkIconInfo and convert those to the NSImage that we need.

Created attachment 265794 [details] [review]
Move get_key_equivalent() to gdk quartz utils.

Created attachment 265795 [details] [review]
Use GtkMenuTracker for Quartz backend.

Review of attachment 265794 [details] [review]:

Looks fine.

Review of attachment 265795 [details] [review]:

Looks good.

There are some small outstanding issues, but they are issues that are already problems before this version, so let's deal with those separately:

 - use idle to deal with activates
 - support submenu-action
 - dispatch the NSApp finished loading call to an idle as well so that it runs after the user's startup() is done

::: gtk/gtkapplication-quartz.c
@@ +133,2 @@
 static void
+gtk_application_impl_quartz_window_added (GtkApplicationImpl *impl,

Not sure why these no-op functions are here.  I'll remove them before committing.

Review of attachment 265795 [details] [review]:

Sorry.  Two small issues in the new part.  Commit when finished with those.

::: gtk/gtkapplication-quartz-menu.c
@@ +105,3 @@
+  GtkIconInfo *info = GTK_ICON_INFO (object);
+  GNSMenuItem *item = user_data;
+  GdkPixbuf *pixbuf = gtk_icon_info_load_symbolic_finish (info, result, NULL, NULL);

You should check if the error is CANCELLED here and avoid touching the item if it is (since it will be on its way to being destroyed, if not already gone).

@@ +203,3 @@
+      scale = roundf ([[NSScreen mainScreen] backingScaleFactor]);
+      info = gtk_icon_theme_lookup_by_gicon_for_scale (theme, icon, ICON_SIZE, scale, GTK_ICON_LOOKUP_USE_BUILTIN);
+      gtk_icon_info_load_symbolic_async (info, &foreground, &success, &warning, &error, cancellable, icon_loaded, self);

Looks like info might be leaking here.  Free it when icon_loaded is done, I guess?

Review of attachment 265795 [details] [review]:

::: gtk/gtkapplication-quartz-menu.c
@@ +105,3 @@
+  GtkIconInfo *info = GTK_ICON_INFO (object);
+  GNSMenuItem *item = user_data;
+  GdkPixbuf *pixbuf = gtk_icon_info_load_symbolic_finish (info, result, NULL, NULL);

Actually, your code is fine here because it doesn't touch anything in the case that the pixbuf is NULL.  Maybe that should set the icon to nil, though...

Created attachment 265797 [details] [review]
Use GtkMenuTracker for Quartz backend.

Attachment 265797 [details] pushed as 649ff84 - Use GtkMenuTracker for Quartz backend.