Merge lp:~gue5t/midori/restore-middle-btn-up-2 into lp:midori

[gue5t gue5t]

Rework mouse button handling in KatzeArrayAction

This changes the semantics of the "activate-item" and "activate-item-alt" signals, and documents their behavior more clearly than previously. The former is used for regular activations, based on keyboard interaction, mouse clicks, or other input methods, and is triggered by the normal GTK "activate" signal, but carries information about whether the event requested a new tab. The latter is used to open popup menus or operate on items within menus without properly activating them.

The keyboard now works to activate items in the bookmarks/tabs/trash menus, and clicking on these items behaves the same as normal items. Right-clicking bookmarks and bookmark folders still works as expected.

[Cris Dywan (kalikiana)]

Did you test with global menubars?

review: Needs Information

[gue5t gue5t (gue5t)]

The global menubars only emit the "activate" event, but I have not. I'll trek on over to the nearest Ubuntu box and see if I can test.

[Cris Dywan (kalikiana)]

Testing I get this while using menus

gtk_widget_get_preferred_height_for_width: assertion 'width >= 0' failed

review: Needs Fixing

[gue5t gue5t (gue5t)]

Does the gtk_widget_get_preferred_height_for_width fail with the Ubuntu global menubar, or with a certain GTK3 theme? I can't seem to reproduce it locally. The assertion sounds like maybe a widget hasn't been realized yet at some point, but it's difficult to figure out realization ordering issues just by looking at code.

[Cris Dywan (kalikiana)]

Hrm something more fundamental must have broken I can't get the menubar to appear in the global panel at all even if I override UBUNTU_MENUPROXY. Though no errors either. But the only change due to Midori assuming there is a global bar I can see is the menu item [x] Menubar disappearing from the menus.

[Cris Dywan (kalikiana)]

review: Approve

[RabbitBot (rabbitbot-a)]

Attempt to merge into lp:midori failed due to conflicts:

text conflict in katze/katze-arrayaction.c

[gue5t gue5t (gue5t)]

I've rebased this and changed the approach; it now makes a lot of the code and behavior more sane. Please rereview when you get a chance.

[Cris Dywan (kalikiana)]

I'm afraid changing the signature of an existing signal is a bad idea, it will crash any existing extensions that use it.
We could consider this in the coming new trunk which breaks compat anyway - where there's the whole new question of porting to glib menu API.

review: Needs Fixing

[lp:~gue5t/midori/restore-middle-btn-up-2]

6899. By gue5t <email address hidden> on 2015-03-22

Rename activate-item signal to activate-item-event to carry event and avoid compatibility worries

6900. By gue5t <email address hidden> on 2015-03-23

Restore full backwards compatibility by reverting signal names and adding activate-item-new-tab signal to KatzeArrayAction

6901. By gue5t <email address hidden> on 2015-03-23

Fix leak of GdkEvent

[Cris Dywan (kalikiana)]

Looks nice now, I appreciate your updates despite the confusion whilst discussing this.

review: Approve

=== modified file 'katze/katze-arrayaction.c'
--- katze/katze-arrayaction.c	2014-07-24 06:01:27 +0000
+++ katze/katze-arrayaction.c	2015-03-23 20:01:00 +0000
@@ -12,6 +12,7 @@
 
 #include "katze-arrayaction.h"
 
+#include "midori/midori-platform.h"
 #include "katze-utils.h"
 #include "marshal.h"
 
@@ -51,6 +52,7 @@
     POPULATE_POPUP,
     POPULATE_FOLDER,
     ACTIVATE_ITEM,
+    ACTIVATE_ITEM_NEW_TAB,
     ACTIVATE_ITEM_ALT,
     LAST_SIGNAL
 };
@@ -115,7 +117,7 @@
      * @menu: the menu shell being opened
      * @folder: the folder being opened
      *
-     * A context menu is going to be opened for @folder,
+     * A submenu is going to be opened for @folder,
      * the provided @menu can be populated accordingly.
      *
      * Unlike "populate-popup" this signal is emitted for
@@ -141,20 +143,39 @@
      * KatzeArrayAction::activate-item:
      * @array: the object on which the signal is emitted
      * @item: the item being activated
-     *
-     * An item was clicked with the first button.
-     *
-     * Deprecated: 0.2.8: Use "activate-item-alt" instead.
+     * @event: (allow-none): the event that caused the activation
+     *
+     * An item was activated.
      **/
     signals[ACTIVATE_ITEM] = g_signal_new ("activate-item",
-                                       G_TYPE_FROM_CLASS (class),
-                                       (GSignalFlags) (G_SIGNAL_RUN_LAST),
-                                       0,
-                                       0,
-                                       NULL,
-                                       g_cclosure_marshal_VOID__OBJECT,
-                                       G_TYPE_NONE, 1,
-                                       KATZE_TYPE_ITEM);
+                                           G_TYPE_FROM_CLASS (class),
+                                           (GSignalFlags) (G_SIGNAL_RUN_LAST),
+                                           0,
+                                           0,
+                                           NULL,
+                                           g_cclosure_marshal_VOID__OBJECT,
+                                           G_TYPE_NONE, 1,
+                                           KATZE_TYPE_ITEM);
+
+    /**
+     * KatzeArrayAction::activate-item-new-tab:
+     * @array: the object on which the signal is emitted
+     * @item: the item being activated
+     * @event: (allow-none): the event that caused the activation
+     *
+     * An item was activated and should be opened in a new tab.
+     *
+     * Since: 0.6.0
+     **/
+    signals[ACTIVATE_ITEM_NEW_TAB] = g_signal_new ("activate-item-new-tab",
+                                                   G_TYPE_FROM_CLASS (class),
+                                                   (GSignalFlags) (G_SIGNAL_RUN_LAST),
+                                                   0,
+                                                   0,
+                                                   NULL,
+                                                   g_cclosure_marshal_VOID__OBJECT,
+                                                   G_TYPE_NONE, 1,
+                                                   KATZE_TYPE_ITEM);
 
     /**
      * KatzeArrayAction::activate-item-alt:
@@ -163,10 +184,9 @@
      * @item: the item being activated
      * @event: the mouse button pressed event
      *
-     * An item was clicked, with the specified @button.
+     * The specified @button has been pressed (but not yet released) on the item.
      *
-     * Return value: %TRUE if the event was handled. If %FALSE is returned,
-     *               the default "activate-item" signal is emitted.
+     * Return value: %TRUE if the event was handled.
      **/
     signals[ACTIVATE_ITEM_ALT] = g_signal_new ("activate-item-alt",
                                        G_TYPE_FROM_CLASS (class),
@@ -291,6 +311,13 @@
     g_signal_emit (action, signals[ACTIVATE_ITEM], 0, item);
 }
 
+static void
+katze_array_action_activate_item_new_tab (KatzeArrayAction* action,
+                                          KatzeItem*        item)
+{
+    g_signal_emit (action, signals[ACTIVATE_ITEM_NEW_TAB], 0, item);
+}
+
 static gboolean
 katze_array_action_activate_item_alt (KatzeArrayAction* action,
                                       KatzeItem*        item,
@@ -309,9 +336,6 @@
     g_signal_emit (action, signals[ACTIVATE_ITEM_ALT], 0, item,
                        proxy, event, &handled);
 
-    if (!handled)
-        katze_array_action_activate_item (action, item);
-
     return handled;
 }
 
@@ -321,7 +345,15 @@
 {
     KatzeItem* item = g_object_get_data (G_OBJECT (proxy), "KatzeItem");
 
-    katze_array_action_activate_item (array_action, item);
+    GdkEvent* event = gtk_get_current_event();
+
+    if (event && MIDORI_EVENT_NEW_TAB (event))
+        katze_array_action_activate_item_new_tab (array_action, item);
+    else
+        katze_array_action_activate_item (array_action, item);
+
+    if (event)
+        gdk_event_free (event);
 }
 
 static gboolean
@@ -339,8 +371,8 @@
                                          GdkEventButton*   event,
                                          KatzeArrayAction* array_action)
 {
-    /* Take precedence over menu button-press-event handling to avoid
-       menu item activation and menu disparition for popup opening
+    /* Override the button-press handler for menus which would
+       open a submenu to instead show a popup menu
     */
 
     return katze_array_action_menu_item_button_press_cb (gtk_get_event_widget ((GdkEvent *) event), event, array_action);
@@ -354,7 +386,7 @@
     GtkWidget* toolitem = gtk_widget_get_parent (proxy);
     KatzeItem* item = g_object_get_data (G_OBJECT (toolitem), "KatzeItem");
 
-    /* let the 'clicked' signal be processed normally */
+    /* Left button clicks (e.g. for activation) should not be handled until button release */
     if (event->button == 1)
         return FALSE;
 
@@ -383,6 +415,7 @@
     {
         GtkWidget* submenu = gtk_menu_new ();
         gtk_menu_item_set_submenu (GTK_MENU_ITEM (menuitem), submenu);
+        /* Handle popup menus on menuitems which have open submenus */
         g_signal_connect (submenu, "button-press-event",
             G_CALLBACK (katze_array_action_menu_button_press_cb), array_action);
         g_signal_connect (menuitem, "select",
@@ -392,11 +425,14 @@
     }
     else
     {
-        /* we need the 'activate' signal as well for keyboard events */
+        /* We need the 'activate' signal for actual item activation */
         g_signal_connect (menuitem, "activate",
             G_CALLBACK (katze_array_action_menu_activate_cb), array_action);
     }
 
+    /* Connect to button-press-event to catch right-clicks to open popup menus.
+       Note that this is also necessary for menuitems which have submenus
+       but whose submenus are not open at present! */
     g_signal_connect (menuitem, "button-press-event",
         G_CALLBACK (katze_array_action_menu_item_button_press_cb), array_action);
 
@@ -550,12 +586,6 @@
         return;
     }
 
-    if (KATZE_IS_ITEM (array) && katze_item_get_uri ((KatzeItem*)array))
-    {
-        katze_array_action_activate_item (array_action, KATZE_ITEM (array));
-        return;
-    }
-
     menu = gtk_menu_new ();
     gtk_menu_attach_to_widget (GTK_MENU (menu), proxy, NULL);
 

=== modified file 'midori/midori-browser.c'
--- midori/midori-browser.c	2015-03-15 18:00:54 +0000
+++ midori/midori-browser.c	2015-03-23 20:01:00 +0000
@@ -3139,12 +3139,21 @@
                                KatzeItem*      item,
                                MidoriBrowser*  browser);
 
-static gboolean
-midori_bookmarkbar_activate_item (GtkAction* action,
-                                  KatzeItem* item,
+static void
+midori_bookmarkbar_activate_item (GtkAction*     action,
+                                  KatzeItem*     item,
                                   MidoriBrowser* browser)
 {
-    return midori_browser_open_bookmark (browser, item);;
+    midori_browser_open_bookmark (browser, item);
+}
+
+static void
+midori_bookmarkbar_activate_item_new_tab (GtkAction*     action,
+                                          KatzeItem*     item,
+                                          MidoriBrowser* browser)
+{
+    GtkWidget* view = midori_browser_add_item (browser, item);
+    midori_browser_set_current_tab_smartly (browser, view);
 }
 
 static gboolean
@@ -3156,19 +3165,10 @@
 {
     g_assert (event);
 
-    if (MIDORI_EVENT_NEW_TAB (event))
-    {
-        GtkWidget* view = midori_browser_add_item (browser, item);
-        midori_browser_set_current_tab_smartly (browser, view);
-    }
-    else if (MIDORI_EVENT_CONTEXT_MENU (event))
+    if (MIDORI_EVENT_CONTEXT_MENU (event))
     {
         midori_browser_bookmark_popup (proxy, NULL, item, browser);
     }
-    else if (event->button == 1)
-    {
-        midori_bookmarkbar_activate_item (action, item, browser);
-    }
 
     return TRUE;
 }
@@ -3203,27 +3203,22 @@
     return view;
 }
 
-static gboolean
-_action_trash_activate_item_alt (GtkAction*      action,
-                                 KatzeItem*      item,
-                                 GtkWidget*      proxy,
-                                 GdkEventButton* event,
-                                 MidoriBrowser*  browser)
-{
-    g_assert (event);
-
-    if (MIDORI_EVENT_NEW_TAB (event))
-    {
-        midori_browser_set_current_tab_smartly (browser,
-            midori_browser_restore_tab (browser, item));
-    }
-    else if (event->button == 1)
-    {
-        midori_browser_set_current_tab (browser,
-            midori_browser_restore_tab (browser, item));
-    }
-
-    return TRUE;
+static void
+_action_trash_activate_item (GtkAction*     action,
+                             KatzeItem*     item,
+                             MidoriBrowser* browser)
+{
+    midori_browser_set_current_tab (browser,
+        midori_browser_restore_tab (browser, item));
+}
+
+static void
+_action_trash_activate_item_new_tab (GtkAction*     action,
+                                     KatzeItem*     item,
+                                     MidoriBrowser* browser)
+{
+    midori_browser_set_current_tab_smartly (browser,
+        midori_browser_restore_tab (browser, item));
 }
 
 /* static */ gboolean
@@ -3339,11 +3334,9 @@
 }
 
 static void
-_action_window_activate_item_alt (GtkAction*      action,
-                                  KatzeItem*      item,
-                                  GtkWidget*      proxy,
-                                  GdkEventButton* event,
-                                  MidoriBrowser*  browser)
+_action_window_activate_item (GtkAction*     action,
+                              KatzeItem*     item,
+                              MidoriBrowser* browser)
 {
     midori_browser_set_current_item (browser, item);
 }
@@ -6038,8 +6031,10 @@
     g_object_connect (action,
                       "signal::populate-popup",
                       _action_trash_populate_popup, browser,
-                      "signal::activate-item-alt",
-                      _action_trash_activate_item_alt, browser,
+                      "signal::activate-item",
+                      _action_trash_activate_item, browser,
+                      "signal::activate-item-new-tab",
+                      _action_trash_activate_item_new_tab, browser,
                       NULL);
     gtk_action_group_add_action_with_accel (browser->action_group, action, "");
     g_object_unref (action);
@@ -6060,6 +6055,8 @@
                       midori_bookmarkbar_activate_item_alt, browser,
                       "signal::activate-item",
                       midori_bookmarkbar_activate_item, browser,
+                      "signal::activate-item-new-tab",
+                      midori_bookmarkbar_activate_item_new_tab, browser,
                       NULL);
     gtk_action_group_add_action_with_accel (browser->action_group, action, "");
     g_object_unref (action);
@@ -6091,8 +6088,11 @@
     g_object_connect (action,
                       "signal::populate-popup",
                       _action_window_populate_popup, browser,
-                      "signal::activate-item-alt",
-                      _action_window_activate_item_alt, browser,
+                      "signal::activate-item",
+                      _action_window_activate_item, browser,
+                      /* middle-clicks on tab menu entries behave like left-clicks */
+                      "signal::activate-item-new-tab",
+                      _action_window_activate_item, browser,
                       NULL);
     gtk_action_group_add_action_with_accel (browser->action_group, action, "");
     g_object_unref (action);