Merge lp:~evfool/midori/lp792536 into lp:midori

[Robert Roth]

Change tooltips of Reload and ReloadStop actions while shift modifier is pressed

Proposing for merge this simple fix to change the ReloadStop toolbar buttons tooltip in case Shift is pressed, to do a full page reload without caching. The tooltip text is the same as used for the action assigned to Ctrl+Shift+R, so no new string.

[gue5t gue5t (gue5t)]

This looks good. I'll have to double-check that IS_SHIFT doesn't make too many assumptions about e.g. modifier mappings (compared to other code), and after I'll r+ assuming it works right in testing.

[gue5t gue5t (gue5t)]

This should apply to the Reload action as well as the StopReload one.

Avoid // comments in favor of /**/ ones.

Just checking GDK_KEY_Shift_R and GDK_KEY_Shift_L is not sufficient to determine if the key last pressed placed the keyboard into a "shift" state, due to the complexities of X modifier mapping. For example, by running "xmodmap -e 'add shift = 0x0064'", the letter d key acts like a shift key. I'm not sure how to best do this--one could remember the state from the last keypress, but that leads to the question of whether GTK gets a key release event if the user presses shift, leaves the window, and then releases shift.

Thanks for your work, though! This is a nice bit of polish that helps expose functionality that's even less discoverable right now.

review: Needs Fixing

[Robert Roth (evfool)]

I have updated the branch with
* setting the tooltip on the Reload action too
* modified the shift handling logic to be fully based on Gdk instead of conditions wizardry. Simply query the current modifier mask on key press/release, and if it contains GDK_SHIFT_MASK, then act accordingly.
* // comments have been removed being unnecessary as this solution is much easier to read
What do you think about this version?

[gue5t gue5t (gue5t)]

There's one unintended change in behavior with this patch: before the patch, pressing any key with a tooltip open hides the tooltip. After the patch, the tooltip is reshown. This applies to all tooltips, not just the one on the affected actions whose tooltip strings change. It might be a good idea to only set tooltips if the new tooltip is different from what it was previously (i.e. maintain state as mentioned, maybe with g_object_get_data/set_data of a bool on the action, or use the existing state by comparing tooltips before setting).

That's a tiny concern and I'm fine with committing as-is, but I just wanted the change noted nonetheless.

review: Approve

[Robert Roth (evfool)]

I have updated the code to only set the tooltip if it is different from what it was.
I have also tested the behaviour with tooltip hiding/reappearing on keypress, and in my tests only Shift is causing the tooltip to reappear, other keys (e.g. Ctrl) are only hiding the tooltip.
In the case of reload and reloadstop hiding and reappearing tooltips makes sense, as if we would hide the tooltip on Shift keypress, then there would be no sense in setting the new tooltip :) However I would like to stop the hide/appear on other toolitems, but haven't found a way to do that, only setting different tooltips does not help.

[gue5t gue5t (gue5t)]

That's all we can do without messing about in GTK internals, I think. Thanks for the tweak!

[gue5t gue5t (gue5t)]

review: Approve

[RabbitBot (rabbitbot-a)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Robert Roth (evfool)]

@gue5t gue5t:It seems to me that you've reviewed each revision I have committed, but the RabbitBot doesn't agree, could you please take another quick peek at the branch and Approve again and Merge if you think it's OK?

[gue5t gue5t (gue5t)]

review: Approve

=== modified file 'midori/midori-browser.c'
--- midori/midori-browser.c	2014-01-06 23:05:10 +0000
+++ midori/midori-browser.c	2014-02-12 22:56:44 +0000
@@ -1925,6 +1925,53 @@
     /* Nothing to do */
 }
 
+static void
+_update_tooltip_if_changed (GtkAction* action,
+                            const gchar* text)
+{
+    gchar *old;
+    g_object_get (action, "tooltip", &old, NULL);
+    if (g_strcmp0(old, text)) {
+        g_object_set (action,
+                      "tooltip", text, NULL);
+    }
+    g_free (old);
+}
+
+static void 
+_update_reload_tooltip (GtkWidget*   widget,
+                        GdkEventKey* event,
+                        gboolean released)
+{
+    MidoriBrowser* browser = MIDORI_BROWSER (widget);
+
+    /* Update the reload/stop tooltip in case we are holding the hard refresh modifiers*/
+    GtkAction *reload_stop = _action_by_name (browser, "ReloadStop");
+    GtkAction *reload = _action_by_name (browser, "Reload");
+    GdkModifierType mask;
+    gdk_window_get_pointer (gtk_widget_get_window (widget), NULL, NULL, &mask);
+    const gchar *target;
+    
+    if ( mask & GDK_SHIFT_MASK)
+    {
+        target = _("Reload page without caching");
+    }
+    else 
+    {
+        target = _("Reload the current page");
+    }
+    _update_tooltip_if_changed (reload_stop, target);
+    _update_tooltip_if_changed (reload, target);
+}
+
+static gboolean
+midori_browser_key_release_event (GtkWidget*   widget,
+                                  GdkEventKey* event)
+{
+    _update_reload_tooltip (widget, event, TRUE);
+    return FALSE;
+}
+
 static gboolean
 midori_browser_key_press_event (GtkWidget*   widget,
                                 GdkEventKey* event)
@@ -1934,6 +1981,7 @@
     GtkWidgetClass* widget_class;
     guint clean_state;
 
+    _update_reload_tooltip(widget, event, FALSE);
     /* Interpret Ctrl(+Shift)+Tab as tab switching for compatibility */
     if (midori_browser_get_nth_tab (browser, 1) != NULL
      && event->keyval == GDK_KEY_Tab
@@ -2259,6 +2307,7 @@
 
     gtkwidget_class = GTK_WIDGET_CLASS (class);
     gtkwidget_class->key_press_event = midori_browser_key_press_event;
+    gtkwidget_class->key_release_event = midori_browser_key_release_event;
 
     gobject_class = G_OBJECT_CLASS (class);
     gobject_class->dispose = midori_browser_dispose;