Created attachment 144033 [details] [review]
[AppWell] Add Add/Remove from favorites menu, unify lists more

Also have inactive applications pop up a menu.

Add/Remove from favorites is now in the menu.

Concatenate the favorites/not-favorites instead of having a gap only
if you happened to have a not-divisible-by-4 number of favorites.

Created attachment 144034 [details] [review]
[ShellMenu] Allow any button to deactivate menu

For some reason we were limiting popdowns to only button 1 before;
other menus popdown on any button, so we should fit in with the
crowd.

Created attachment 144035 [details] [review]
[AppIcon] Immediately pop up menu on right click

We were actually showing a menu on button 3 before, but only
through a chain of coincidences.  This patch explicitly supports
it and makes sure we show it immediately rather than after
a timeout.

Review of attachment 144033 [details] [review]:

Menu part loosk fine to me. Removal of the "separator" looks right. Think the DND behavior might need a bit more tweeking.

::: js/ui/appDisplay.js
@@ -900,3 @@
 
-        let dropIsFavorite = this._grid.isBeforeSeparator(x - this._grid.actor.x,
-                                                          y - this._grid.actor.y);

It doesn't really make sense to me that dragging a not-favorite app a little bit and releasing would mark it is favorite. I think only the gesture of dragging an app from somwehere else (not the well) should favorite it. To favorite stuff in the well, you need to use the menu or whatever.

(Maybe eventually we can have the kind of rearrangement interface discussed in the meeting yesterday, but then there needs to be an affordance for showing what's going to happen when you drop, like highlighting the part of the app well that is favorite apps and when you start dragging into there showing a tooltip like "Add to favorites")

Review of attachment 144034 [details] [review]:

For click-hold-release, what you need to do here is to make the menu popdown on the release of the button it was opened with.

This doesn't really matter a lot for mice. It's a very big deal for Wacom tablets which have the tip as button 1 and a button on the side of the stylus as button 3. If you have a menu activated with the side button, it must not be deactivated if the user temporarily presses the tip.

On the other hand, if the user has clicked and released, then any click should activate/dismiss the menu.

Review of attachment 144035 [details] [review]:

Looks good. (You'll have to psas in the button to popupMenu to get the behavior I described in the other bug, but that's separate from this.)

(In reply to comment #4)

> It doesn't really make sense to me that dragging a not-favorite app a little
> bit and releasing would mark it is favorite. I think only the gesture of
> dragging an app from somwehere else (not the well) should favorite it. To
> favorite stuff in the well, you need to use the menu or whatever.

I agree and that's what's implemented, note the patch removes the handling for WellDisplayItem:

-        if (source instanceof BaseWellItem) {
-            app = source.appInfo;

(In reply to comment #7)
> (In reply to comment #4)
> 
> > It doesn't really make sense to me that dragging a not-favorite app a little
> > bit and releasing would mark it is favorite. I think only the gesture of
> > dragging an app from somwehere else (not the well) should favorite it. To
> > favorite stuff in the well, you need to use the menu or whatever.
> 
> I agree and that's what's implemented, note the patch removes the handling for
> WellDisplayItem:
> 
> -        if (source instanceof BaseWellItem) {
> -            app = source.appInfo;

Ah, OK

(In reply to comment #5)
> Review of attachment 144034 [details] [review]:
> 
> For click-hold-release, what you need to do here is to make the menu popdown on
> the release of the button it was opened with.
>

Hmm; GtkMenu appears to popdown on any release as well, or at least I just quickly tested with the menu bar in XChat-GNOME.

(In reply to comment #9)
> (In reply to comment #5)
> > Review of attachment 144034 [details] [review] [details]:
> > 
> > For click-hold-release, what you need to do here is to make the menu popdown on
> > the release of the button it was opened with.
> >
> 
> Hmm; GtkMenu appears to popdown on any release as well, or at least I just
> quickly tested with the menu bar in XChat-GNOME.

Hmm, true, but it still doesn't make it right! (one thing to note about the GTK+ behavior is it only activates on the button that it was triggered with - other button releases just dismiss.)

Created attachment 144197 [details] [review]
Immediately pop up menu on right click

We were actually showing a menu on button 3 before, but only
through a chain of coincidences.  This patch explicitly supports
it and makes sure we show it immediately rather than after
a timeout.  In this case, allow deactivation on any button.

Also, in the click+hold case, pass the activating button in so
that we only pop down on that button.

Created attachment 144198 [details] [review]
[ShellMenu] Allow conditional cancellation depending on button

Before we hardcoded popdowns to only button 1 before.  Instead start
respecting the button passed in; if it's 0 we allow popdown on any
button, if specified then we only pop down on that button.

Review of attachment 144197 [details] [review]:

One line that doesn't make sense to me and that the comment doesn't help on, rest looks good.

::: js/ui/appIcon.js
@@ +214,3 @@
+        } else if (button == 3) {
+            // Don't bind to the button for right click
+            this.popupMenu(0);

Why don't bind to the button for the right click?

Review of attachment 144198 [details] [review]:

One fix needed, otherwise looks OK. (The needed fix may be the reason why you had the mysterious exclusion of right-clicks in the other patch)

::: src/shell-menu.c
@@ +125,1 @@
     return FALSE;

After this check, you should set box->priv->activating_button = 0, since once the button has been released once, it no longer matters for subsequent releases.

(In reply to comment #13)
> Review of attachment 144197 [details] [review]:
> 
> One line that doesn't make sense to me and that the comment doesn't help on,
> rest looks good.
> 
> ::: js/ui/appIcon.js
> @@ +214,3 @@
> +        } else if (button == 3) {
> +            // Don't bind to the button for right click
> +            this.popupMenu(0);
> 
> Why don't bind to the button for the right click?

// Allow any button to deactivate

?

Created attachment 144608 [details] [review]
Allow conditional cancellation depending on button

Before we hardcoded popdowns to only button 1 before.  Instead start
respecting the button passed in; if it's 0 we allow popdown on any
button, if specified then we only pop down on that button.

Created attachment 144609 [details] [review]
Immediately pop up menu on right click

We were actually showing a menu on button 3 before, but only
through a chain of coincidences.  This patch explicitly supports
it and makes sure we show it immediately rather than after
a timeout.  In this case, allow deactivation on any button.

Also, in the click+hold case, pass the activating button in so
that we only pop down on that button.

Review of attachment 144608 [details] [review]:

::: src/shell-menu.c
@@ +124,3 @@
+  if (box->priv->activating_button > 0 && box->priv->activating_button != event->button)
+    {
+      box->priv->activating_button = 0;

This is still wrong, the activating_button needs to be cleared when the *activating button* is released, not when other buttons are released.

Review of attachment 144609 [details] [review]:

Still wrong as commented before.

Created attachment 144616 [details] [review]
Ignore releases of buttons other than the activating button

Just a tiny change from your patch - revised the comment
and cleared activating_button when the activating_button was
released not when the other buttons were released.

Created attachment 144617 [details] [review]
Immediately pop up menu on right click

Even tinier change - changed the commit message, and pass in
'button' rather than 0. when button==3.

Attachment 144616 [details] pushed as c5ce405 - Ignore releases of buttons other than the activating button
Attachment 144617 [details] pushed as 3b8d530 - Immediately pop up menu on right click