Merge lp:~junrrein/midori/granite-overlaybar into lp:midori

[Julián Unrrein]

Use Granite's Overlay Bar when Granite is available for the floating status bar.

Use Granite's Overlay Bar when Granite is available for the floating status bar.

The current floating status bar has an usability issue. When it is hovering over a link, it disappears so the user is able to click it, but the link info is not available anymore -> http://videobin.org/+6tu/8b5.html

Granite's overlay bar behaves similarly as Firefox or Chrome's counterparts. When hovering over them with the mouse pointer, it moves horizontally to the other side -> http://videobin.org/+6tu/8b6.html

This widget can be styled by the theme, but it has a sane fallback theme (which is the one being shown in the second video, when using the elementary theme).

It doesn't return to the original position automatically, though this behavior may be introduced by upstream in the future.

Another thing to test is that conditional compilation without Granite works as expected.

[David Gomes (davidgomes)]

Approving as a community member.

review: Approve

[David Gomes (davidgomes)]

(by that I mean it still needs approve from a Midori dev, not just an ~elementary-apps member)

[Cris Dywan (kalikiana)]

I'm a little concerned here because this is a very fundamental problem which a) will only be solved for Granite builds b) is only half solved wrt positioning as of now.

From a quick look at lp:granite the enter_notify_callback flipping alignment is what gets the positioning right, all the rest is theming - I'm not sure why the sizing is needed.

Is it possible to avoid the custom drawing and just have a class name in Midori to allow nicer theming?

review: Needs Information

[Julián Unrrein (junrrein)]

Hi kalikiana. I'm afraid to say that I didn't understand very well what you said, so I'm asking you to clarify a couple of points:

- About "a)", I guess the only other option if we want to replicate Granite's overlay bar, is to copy it over. I'm not really sure that's a good thing.
Getting a Granite-only fix should be better than no-fix at all. When an actual fix for Midori comes around, this can be discarded.

- About "b)", what would be the full solution?

- About your concern with Granite's overlay bar being bloated, I did some preliminary testing, and it seems the custom sizing isn't needed indeed. I will look into that.

- You can theme this widget if you want, the class name is ".granite-overlay-bar".

[Cris Dywan (kalikiana)]

This fix has a high impact on usability, not just visuals. We need a fix for pure GTK+3. And first getting Granite-specific code in will make a follow up harder.

My point really is: I would prefer a solution that doesn't rely on Granite. And if the theming doable with an ordinary label is not good enough, I'd like to know what's needed there.

review: Needs Information

[Julián Unrrein (junrrein)]

I know that this has a high impact on usability, but I don't understand why this problem is "only half solved with positioning". What would be the ideal solution?

I don't understand what the issue with theming is.

If you don't want this to be Granite-specific, would copying the widget over to Midori's codebase be a bad idea?

[Cris Dywan (kalikiana)]

Wrt half solved: see David's comment above. It doesn't jump back automatically.

Wrt theming & copying: I asked to find out why the widget in Granite is more than an ordinary label, which is what Midori uses currently. It's not obvious from the code.

review: Needs Information

[Julián Unrrein (junrrein)]

The widget in Granite is little more than an ordinary label inside an eventbox. It just moves out of the way, and it ensures the "margin" propriety in the css is applied correctly.

About theming: what Midori is currently theming is the frame that holds the label, not the label itself.

[Cris Dywan (kalikiana)]

I will provide a generic fix first - as I said, I think this is rather crucial. We can look at nice theming afterwards.

review: Disapprove

[Julián Unrrein (junrrein)]

I understand. Thanks for taking the time to review this.

Unmerged revisions

6358. By Julián Unrrein on 2013-08-22

Use Granite's Overlay Bar when Granite is available for the floating status bar.

=== modified file 'data/gtk3.css'
--- data/gtk3.css	2012-08-19 12:22:38 +0000
+++ data/gtk3.css	2013-08-22 20:25:16 +0000
@@ -7,7 +7,7 @@
     padding: 0;
 }
 
-GtkOverlay > * {
+GtkOverlay > .frame {
     padding: 4px;
     border-style: solid;
     border-radius: 0 5px 0 0;

=== modified file 'midori/midori-view.c'
--- midori/midori-view.c	2013-08-15 11:55:34 +0000
+++ midori/midori-view.c	2013-08-22 20:25:16 +0000
@@ -3565,6 +3565,13 @@
     gtk_box_pack_start (GTK_BOX (view), view->overlay, TRUE, TRUE, 0);
 
     /* Overlays must be created before showing GtkOverlay as of GTK+ 3.2 */
+    #ifdef HAVE_GRANITE
+    {
+    view->overlay_label = GTK_WIDGET (granite_widgets_overlay_bar_new (GTK_OVERLAY (view->overlay)));
+    gtk_widget_set_no_show_all (view->overlay_label, TRUE);
+    gtk_widget_set_halign (view->overlay_label, GTK_ALIGN_START);
+    }
+    #else
     {
     GtkWidget* frame = gtk_frame_new (NULL);
     gtk_widget_set_no_show_all (frame, TRUE);
@@ -3575,6 +3582,7 @@
     gtk_widget_set_valign (frame, GTK_ALIGN_END);
     gtk_overlay_add_overlay (GTK_OVERLAY (view->overlay), frame);
     }
+    #endif
     view->overlay_find = g_object_new (MIDORI_TYPE_FINDBAR, NULL);
     gtk_widget_set_halign (view->overlay_find, GTK_ALIGN_END);
     gtk_widget_set_valign (view->overlay_find, GTK_ALIGN_START);
@@ -4092,6 +4100,16 @@
     g_return_if_fail (MIDORI_IS_VIEW (view));
 
     #if GTK_CHECK_VERSION (3, 2, 0)
+    #ifdef HAVE_GRANITE
+    if (text == NULL)
+        gtk_widget_hide (view->overlay_label);
+    else
+    {
+        granite_widgets_overlay_bar_set_status (GRANITE_WIDGETS_OVERLAY_BAR (view->overlay_label),
+                                                text);
+        gtk_widget_show (view->overlay_label);
+    }
+    #else /* HAVE_GRANITE */
     if (text == NULL)
         gtk_widget_hide (gtk_widget_get_parent (view->overlay_label));
     else
@@ -4100,6 +4118,7 @@
         gtk_widget_show (gtk_widget_get_parent (view->overlay_label));
     }
     #endif
+    #endif
 }
 
 /**