Created attachment 144866 [details] [review]
[AppIcon] compute the sorted window list even if not doing the menu

Created attachment 144867 [details] [review]
[AppIcon] redo constructor to take a params object, add "size" param

Add a "size" parameter to allow changing the AppIcon size, and then
simplify the constructor by taking an object with parameters like
gobject-introspection constructors do, rather than taking a large
number of miscellaneous arguments.

Created attachment 144868 [details] [review]
[AppIcon] Improve shell_draw_box_pointer()

Add a new enum type for the pointer direction, rather than abusing
ClutterGravity, and implement the missing directions.

Review of attachment 144866 [details] [review]:

Looks right to me - there was already a bad interaction between menuType and the glow boolean where if menuType was NONE the glow code would have thrown. This does conflict with Colin's patch in bug 597120

Review of attachment 144867 [details] [review]:

Looks good, couple of minor style things.

::: js/ui/appIcon.js
@@ +60,3 @@
+            throw new Error('AppIcon constructor requires "appInfo" param');
+
+        this._menuType = params.menuType || MenuType.NONE;

I don't like this pattern ... it works here only because MenuType.NONE is numerically 0, but if you changed it to:

 params.menuType || MenuType.ON_RIGHT

you'd get a surprise if you tried to specify MenuType.NONE. I think an explicit ternary:

 this._menuType = params.menuType !== undefined ? params.menuType : MenuType.NONE;

is better.

@@ +61,3 @@
+
+        this._menuType = params.menuType || MenuType.NONE;
+        this._iconSize = params.size || APPICON_ICON_SIZE;

maybe this should be changed to APPICON_DEFAULT_SIZE ?

Review of attachment 144868 [details] [review]:

Looks good, just minor style nits.

::: src/shell-drawing.c
@@ +176,3 @@
+      cairo_move_to (cr, 0, 0);
+      cairo_line_to (cr, floor (width * 0.5), height);
+      cairo_line_to (cr, width, 0);

Probably better practice to flip the last two lines so that all the arrows are drawn clockwise; it doesn't actually matter unless you were combining and overlapping arrows within a single path.

@@ +182,3 @@
+      cairo_move_to (cr, width, 0);
+      cairo_line_to (cr, 0, floor (height * 0.5));
+      cairo_line_to (cr, width, height);

And flip these two.

::: src/shell-drawing.h
@@ +20,3 @@
+  SHELL_POINTER_RIGHT
+} ShellPointerDirection;
+void shell_draw_box_pointer (ClutterCairoTexture   *texture,

Looks weird to me without a blank line, even if the enum is single-use.

@@ +21,3 @@
+} ShellPointerDirection;
+void shell_draw_box_pointer (ClutterCairoTexture   *texture,
+                             ShellPointerDirection  pointing_towards,

I don't think you are "pointing towards down", though you might have been 'pointing towards south'

(In reply to comment #5)
> +        this._menuType = params.menuType || MenuType.NONE;
> 
> I don't like this pattern ... it works here only because MenuType.NONE is
> numerically 0, but if you changed it to:
> 
>  params.menuType || MenuType.ON_RIGHT
> 
> you'd get a surprise if you tried to specify MenuType.NONE.

Yeah, I ran into this trying to have glow be default-true (so switched it to default-false). Figured I'd worry about it more when we actually had params-constructor-helper funcs. But I'll change it.

> maybe this should be changed to APPICON_DEFAULT_SIZE ?

yup

(In reply to comment #6)
> I don't think you are "pointing towards down", though you might have been
> 'pointing towards south'

I'd named it that way because it seemed a bit confusing in the context of the window menus that you specify WEST/LEFT when doing MENU_ON_RIGHT and NORTH/UP when doing MENU_BELOW. But I can change it.

(In reply to comment #7)

> (In reply to comment #6)
> > I don't think you are "pointing towards down", though you might have been
> > 'pointing towards south'
> 
> I'd named it that way because it seemed a bit confusing in the context of the
> window menus that you specify WEST/LEFT when doing MENU_ON_RIGHT and NORTH/UP
> when doing MENU_BELOW. But I can change it.

Maybe I'm just too used to things but a "down arrow" or "arrow direction=down" is very obvious to me, while an "arrow pointing_towards=down" I have to stop and think "what does that mean? Oh... must mean a down arrow"

The fact that a menu on the right has a left arrow to point back to the contents seems fairly natural.

Attachment 144866 [details] pushed as e5efecd - [AppIcon] compute the sorted window list even if not doing the menu
Attachment 144867 [details] pushed as 45dd342 - [AppIcon] redo constructor to take a params object, add "size" param
Attachment 144868 [details] pushed as ff4ac0d - [AppIcon] Improve shell_draw_box_pointer()