After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 643909 - Floating statusbar should escape from mouse pointer
Floating statusbar should escape from mouse pointer
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
git master
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
: 626773 647080 647997 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-03-04 21:35 UTC by Xan Lopez
Modified: 2012-01-17 10:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Slide out the status overlay when the mouse pointer goes close. (2.59 KB, patch)
2011-03-08 19:06 UTC, Alexandre Mazari
reviewed Details | Review
Slide out the status overlay when the mouse pointer goes close. (16.90 KB, patch)
2011-03-09 11:26 UTC, Alexandre Mazari
needs-work Details | Review
Slide out the status overlay when the mouse pointer goes close by. (15.66 KB, patch)
2011-03-09 16:21 UTC, Alexandre Mazari
none Details | Review
Slide out the status overlay when the mouse pointer goes close by. (16.02 KB, patch)
2011-03-09 17:53 UTC, Alexandre Mazari
needs-work Details | Review
Slide out the status overlay when the mouse pointer goes close by. (17.11 KB, patch)
2011-03-17 10:14 UTC, Alexandre Mazari
none Details | Review
Slide out the status overlay when the mouse pointer goes close by. (19.17 KB, patch)
2011-03-17 16:16 UTC, Alexandre Mazari
none Details | Review
Slide out the status overlay when the mouse pointer goes close by. (17.05 KB, patch)
2011-04-15 14:21 UTC, Alexandre Mazari
none Details | Review
Fix indentation issues according to xan's review. (15.61 KB, patch)
2011-04-19 11:04 UTC, Alexandre Mazari
none Details | Review
Add a constructor using the default distance prop value (currently set to 20 pixels). (15.61 KB, patch)
2011-04-19 11:16 UTC, Alexandre Mazari
needs-work Details | Review
Fixes remaining indentation glitches, modify the child's event window mask in the (15.61 KB, patch)
2011-04-19 13:36 UTC, Alexandre Mazari
none Details | Review
Slide out the status overlay when the mouse pointer goes close by. (16.89 KB, patch)
2011-04-19 13:57 UTC, Alexandre Mazari
needs-work Details | Review

Description Xan Lopez 2011-03-04 21:35:41 UTC
Title says it all; at the moment the statusbar will obscure web content on the bottow left corner, it should move away when the mouse approaches that area.
Comment 1 Alexandre Mazari 2011-03-08 19:06:28 UTC
Created attachment 182851 [details] [review]
Slide out the status overlay when the mouse pointer goes close.
Comment 2 Xan Lopez 2011-03-08 19:48:08 UTC
Review of attachment 182851 [details] [review]:

A few comments:

- For obvious reasons this cannot be done like this, because we don't want to do it always with all children. As suggested on jabber, I think we should subclass GeditOverlayChild and add the logic to "escape" from the pointer there.

- I think it's cleaner if we do this just by messing with the "offset" properties of the child (maybe we'll need horizontal and vertical offset) and queue a resize, rather than directly allocating things.
Comment 3 Xan Lopez 2011-03-08 19:49:35 UTC
CCing Nacho.
Comment 4 Ignacio Casal Quinteiro (nacho) 2011-03-09 10:37:23 UTC
As xan says you must override GeditOverlayChild and I think you should do it in this way
- if the pointer is over the floating widget change the position to the opposite one
- if we are in the opposite one and we are still over it, because the floating widget is too big, we but change it back to the normal position and set an offset further than the pointer or change the allocation to have a width of pointer - allocation.x.

what do you think, this is more or less how it works chrome but I think it would have a better behavior than chrome.
Comment 5 Alexandre Mazari 2011-03-09 11:26:15 UTC
Created attachment 182949 [details] [review]
Slide out the status overlay when the mouse pointer goes close.

Draft of an implementation of an escaping overlay child.
- Makes the child bigger and listen to mouse motion event, and place our widget accordingly.

Need help : make the event window and the GtkBin transparent
Comment 6 Ignacio Casal Quinteiro (nacho) 2011-03-09 13:15:21 UTC
Review of attachment 182949 [details] [review]:

I think it is quite far from finished but here it comes the first comments.

::: embed/ephy-embed.c
@@ +453,2 @@
   gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
+  gedit_overlay_add_escaping (GEDIT_OVERLAY (overlay), frame, GEDIT_OVERLAY_CHILD_POSITION_SOUTH_WEST, 0);

create the escaping child here, and add it with gedit_overlay_add

::: lib/widgets/gedit-overlay-escaping-child.c
@@ +1,2 @@
+/*
+ * gedit-overlay-escaping-child.c - Source for GeditOverlayEscapingChild

let's use a ephy name, not gedit as it is a subclass and this is only for ephy

@@ +25,3 @@
+
+static void
+gedit_overlay_escaping_child_constructed (GObject *object);

move up the functions so you don't need to add the prototypes.

::: lib/widgets/gedit-overlay-escaping-child.h
@@ +27,3 @@
+G_BEGIN_DECLS
+
+#define GEDIT_TYPE_OVERLAY_ESCAPING_CHILD       \

use the right indentation for this like in the other widgets

::: lib/widgets/gedit-overlay.c
@@ +479,3 @@
+ */
+void
+ * @position: a #GeditOverlayChildPosition

this is not needed

::: lib/widgets/gedit-overlay.h
@@ +65,3 @@
 
+void
+gedit_overlay_add_escaping (GeditOverlay             *overlay,

not needed
Comment 7 Xan Lopez 2011-03-09 13:19:48 UTC
I think that unless we can show with actual numbers that listening to motion events in the parent Overlay is too slow we should just do that (this is what Chromium does, AFAIK). This seems too hacky for my taste.
Comment 8 Alexandre Mazari 2011-03-09 16:21:32 UTC
Created attachment 183001 [details] [review]
Slide out the status overlay when the mouse pointer goes close by.

This introduces a GeditOverlayChild subclass, listening to parent
overlay mouse events to define the escaping policy.
The distance from which the widget "escapes" the mouse pointer
can be set at construction time.
Comment 9 Alexandre Mazari 2011-03-09 17:53:07 UTC
Created attachment 183010 [details] [review]
Slide out the status overlay when the mouse pointer goes close by.

This introduces a GeditOverlayChild subclass, listening to parent
overlay mouse events to define the escaping policy.
The distance from which the widget "escapes" the mouse pointer
can be set at construction time.
Comment 10 Frederic Peters 2011-03-11 08:01:15 UTC
Xan, I am resetting the "GNOME target" field, it's used for GNOME wide blocker bugs, it is advised module maintainers use the "Target milestone" field. Thanks.

(and I hope this gets in soon, I have been annoyed a few times by this issue).
Comment 11 Diego Escalante Urrelo (not reading bugmail) 2011-03-12 21:18:47 UTC
Review of attachment 183010 [details] [review]:

::: lib/widgets/gedit-overlay-escaping-child.c
@@ +70,3 @@
+  GeditOverlayEscapingChildPrivate *priv =
+      GEDIT_OVERLAY_ESCAPING_CHILD (widget)->priv;
+  gint distance_x, distance_y;

We usually just use int (see HACKING)

@@ +78,3 @@
+      - priv->escaping_distance || event->x > (alloc.x + alloc.width
+      + gedit_overlay_child_get_offset (GEDIT_OVERLAY_CHILD (widget))
+      + priv->escaping_distance))

Can you use temporal vars? It looks a bit crowded here in this if().

@@ +95,3 @@
+parent_size_allocate (GtkWidget    *widget,
+               GdkRectangle *allocation,
+               GtkWidget      *parent)

Indent? Or perhaps just bugzilla doing something weird...

@@ +194,3 @@
+
+  switch (property_id)
+  {

K&R style for this:

switch () {
    case A:
        break;
    default:
        break;
}

@@ +216,3 @@
+gedit_overlay_escaping_child_new (GtkWidget *widget, guint escaping_distance)
+{
+

A BLANK LINE! This is unacceptable!

::: lib/widgets/gedit-overlay-escaping-child.h
@@ +46,3 @@
+  (G_TYPE_INSTANCE_GET_CLASS ((obj),                                    \
+                              GEDIT_TYPE_OVERLAY_ESCAPING_CHILD,        \
+                              GeditOverlayEscapingChildClass))

Make them one-liners, like in lib/widgets/ephy-download-widget.h (and consider the indent too)

::: lib/widgets/gedit-overlay.c
@@ +470,3 @@
+
+/**
+ * gedit_overlay_add:

Seems you forgot to update the doc string.

::: lib/widgets/gedit-overlay.h
@@ +68,3 @@
+                            GtkWidget                *widget,
+                            GeditOverlayChildPosition position,
+                            guint                     offset);

Update the alignment so the file looks tidy :)
Comment 12 Alexandre Mazari 2011-03-17 10:14:06 UTC
Created attachment 183613 [details] [review]
Slide out the status overlay when the mouse pointer goes close by.

This patch fixes the style/indentation issue reported by descalante.
Also the _add_escaping method was simplified by using the existing facilities.
The inner widget "destroy" signal binding with its containing child was move into EphyOverlayChild instad of being done in the EphyOverlay.

This introduces a GeditOverlayChild subclass, listening to parent
overlay mouse events to define the escaping policy.
The distance from which the widget "escapes" the mouse pointer
can be set at construction time.
Comment 13 Alexandre Mazari 2011-03-17 13:46:27 UTC
(In reply to comment #4)
> As xan says you must override GeditOverlayChild and I think you should do it in
> this way
> - if the pointer is over the floating widget change the position to the opposite
> one
> - if we are in the opposite one and we are still over it, because the floating
> widget is too big, we but change it back to the normal position and set an
> offset further than the pointer or change the allocation to have a width of
> pointer - allocation.x.
Hi nacho, thanks for you review !

> ::: embed/ephy-embed.c
> @@ +453,2 @@
> gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
> +  gedit_overlay_add_escaping (GEDIT_OVERLAY (overlay), frame,
> GEDIT_OVERLAY_CHILD_POSITION_SOUTH_WEST, 0);
> 
> create the escaping child here, and add it with gedit_overlay_add
> 
I think implementation details should be hidden and not leak in client code. The OverlayChild creation is the duty of the Overlay IMHO.

> ::: lib/widgets/gedit-overlay-escaping-child.c
> @@ +1,2 @@
> +/*
> + * gedit-overlay-escaping-child.c - Source for GeditOverlayEscapingChild
> 
> let's use a ephy name, not gedit as it is a subclass and this is only for ephy

A'ght, will fix.
> 
> @@ +25,3 @@
> +
> +static void
> +gedit_overlay_escaping_child_constructed (GObject *object);
> 
> move up the functions so you don't need to add the prototypes.
> 
The _constructed function disapeared in recent patches.

> ::: lib/widgets/gedit-overlay-escaping-child.h
> @@ +27,3 @@
> +G_BEGIN_DECLS
> +
> +#define GEDIT_TYPE_OVERLAY_ESCAPING_CHILD       \
> 
> use the right indentation for this like in the other widgets

Will do
> 
> ::: lib/widgets/gedit-overlay.c
> @@ +479,3 @@
> + */
> +void
> + * @position: a #GeditOverlayChildPosition
> 
> this is not needed
> 
> ::: lib/widgets/gedit-overlay.h
> @@ +65,3 @@
> 
> +void
> +gedit_overlay_add_escaping (GeditOverlay             *overlay,
> 
> not needed
See above
Comment 14 Alexandre Mazari 2011-03-17 16:16:08 UTC
Created attachment 183648 [details] [review]
Slide out the status overlay when the mouse pointer goes close by.

Updated in this patch:
- disconnect signal handlers on the parent at reparenting/destruction time
- precalculate the bounding rectangle of the escaping zone
- handle the case when the pointer hovers the widget
- correct macros indentation

BTW, the more I think about it, the more I think we should listen to an inclosing GtkEventBox instead of litening to the whole parent overlay.
Comment 15 Ignacio Casal Quinteiro (nacho) 2011-03-17 16:44:15 UTC
(In reply to comment #13)
> (In reply to comment #4)
> > As xan says you must override GeditOverlayChild and I think you should do it in
> > this way
> > - if the pointer is over the floating widget change the position to the opposite
> > one
> > - if we are in the opposite one and we are still over it, because the floating
> > widget is too big, we but change it back to the normal position and set an
> > offset further than the pointer or change the allocation to have a width of
> > pointer - allocation.x.
> Hi nacho, thanks for you review !
> 
> > ::: embed/ephy-embed.c
> > @@ +453,2 @@
> > gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
> > +  gedit_overlay_add_escaping (GEDIT_OVERLAY (overlay), frame,
> > GEDIT_OVERLAY_CHILD_POSITION_SOUTH_WEST, 0);
> > 
> > create the escaping child here, and add it with gedit_overlay_add
> > 
> I think implementation details should be hidden and not leak in client code.
> The OverlayChild creation is the duty of the Overlay IMHO.

See that this is a general purpose widget and it will be added to gtk+ so you will not be able to modify it to add your own widgets, this is the reason we allow to inherit from the childoverlay, if not it would be totally private. So what you have to do is to create your own scape child in you widget and add it to the overlay with gedit_overlay_add instead of creating your own gedit_overlay* method for this. See how gedit or nautilus did it in case you have some doubts.

> 
> > ::: lib/widgets/gedit-overlay-escaping-child.c
> > @@ +1,2 @@
> > +/*
> > + * gedit-overlay-escaping-child.c - Source for GeditOverlayEscapingChild
> > 
> > let's use a ephy name, not gedit as it is a subclass and this is only for ephy
> 
> A'ght, will fix.
> > 
> > @@ +25,3 @@
> > +
> > +static void
> > +gedit_overlay_escaping_child_constructed (GObject *object);
> > 
> > move up the functions so you don't need to add the prototypes.
> > 
> The _constructed function disapeared in recent patches.
> 
> > ::: lib/widgets/gedit-overlay-escaping-child.h
> > @@ +27,3 @@
> > +G_BEGIN_DECLS
> > +
> > +#define GEDIT_TYPE_OVERLAY_ESCAPING_CHILD       \
> > 
> > use the right indentation for this like in the other widgets
> 
> Will do
> > 
> > ::: lib/widgets/gedit-overlay.c
> > @@ +479,3 @@
> > + */
> > +void
> > + * @position: a #GeditOverlayChildPosition
> > 
> > this is not needed
> > 
> > ::: lib/widgets/gedit-overlay.h
> > @@ +65,3 @@
> > 
> > +void
> > +gedit_overlay_add_escaping (GeditOverlay             *overlay,
> > 
> > not needed
> See above
Comment 16 Xan Lopez 2011-04-07 18:53:13 UTC
*** Bug 647080 has been marked as a duplicate of this bug. ***
Comment 17 Alexandre Mazari 2011-04-15 14:21:49 UTC
Created attachment 186023 [details] [review]
Slide out the status overlay when the mouse pointer goes close by.

This introduces a GeditOverlayChild subclass, listening to parent
overlay mouse events to define the escaping policy.
The distance from which the widget "escapes" the mouse pointer
can be set at construction time.

Updated since last patch:
- remove _add_escaping API from GeditOverlay
- rename GeditOverlayEscapingChild to EphyOverlayEscapingChild

This patchs tries to limit modifications to the Gedit provided class. Two changes were required and might be concidered for upstreaming:
- add GDK_LEAVE_NOTIFY_MASK to GeditOverlay event window, so we can catch mouse entering/leaving. This is used by EphyOverlayEscapingChild to reset the position when the mouse leaves.
- add GDK_POINTER_MOTION_MASK to GeditOverlayChild event window. This is used by EphyOverlayEscapingChild to hide the status bar on hover.

Another change to gedit code is provided, moving the attachement of gtk_widget_destroy to the OverlayChild "destroy" event from GeditOverlay, to GeditOverlayChild. This change is not required but is a small improvement imho.
Comment 18 Ignacio Casal Quinteiro (nacho) 2011-04-15 14:29:11 UTC
Please update to the latest code of gedit and make a patch for the new one, and attach a patch in a new for gedit.
Comment 19 Xan Lopez 2011-04-16 04:22:47 UTC
Review of attachment 186023 [details] [review]:

::: embed/ephy-embed.c
@@ +455,2 @@
   gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
+  child = ephy_overlay_escaping_child_new(frame, 20);

missing space before (. Shouldn't there be a way of creating the widget without passing an explicit distance? Either two functions or passing '-1' to use whatever the default is.

::: lib/widgets/ephy-overlay-escaping-child.c
@@ +32,3 @@
+                                           guint property_id,
+                                           const GValue *value,
+                                           GParamSpec *pspec);

Reorder the code so that we don't need this.

@@ +39,3 @@
+  PROP_ESCAPING_DISTANCE = 1, LAST_PROPERTY
+};
+

The way this is usually done is PROP_0 at first, and please one enum per line.

@@ +48,3 @@
+
+/* If the pointer leaves the window, restore the widget position
+ */

The style for small coments is /* ... */

@@ +55,3 @@
+{
+  EphyOverlayEscapingChildPrivate *priv =
+      EPHY_OVERLAY_ESCAPING_CHILD (widget)->priv;

Don't break a new a line.

@@ +65,3 @@
+}
+
+/* this should be in Gdk...really */

There is GdkPoint in GdkRegion.

@@ +70,3 @@
+{
+  return point_x >= rectangle.x && point_x < rectangle.x + rectangle.width &&
+          point_y >= rectangle.y && point_y < rectangle.y + rectangle.height;

Indentation is weird here.

@@ +75,3 @@
+/* Keep the widget-pointer distance at at least
+ * EphyOverlayEscapingChildPrivate::escaping_distance by sliding the widget
+ * away if needed

Missing '.' at the end.  If you are using the long format for function doc I guess you need a /** at the beginning.

@@ +80,3 @@
+parent_motion_notify_event (GtkWidget *widget,
+                            GdkEventMotion *event,
+                            GtkWidget *parent)

parent is not used in the whole method.

@@ +83,3 @@
+{
+  EphyOverlayEscapingChildPrivate *priv =
+      EPHY_OVERLAY_ESCAPING_CHILD (widget)->priv;

Don't break a new line.

@@ +90,3 @@
+
+  if (is_point_in_rectangle (event->x, event->y, priv->escaping_area))
+  {

The style for braces is wrong, they go in the previous line. See HACKING.

@@ +92,3 @@
+  {
+    gtk_widget_get_pointer (widget, &distance_x, &distance_y);
+    alloc.y = alloc.y + (priv->escaping_distance + distance_y);

alloc.y +=

@@ +139,3 @@
+static void
+ephy_overlay_escaping_child_parent_set (GtkWidget *widget,
+                                         GtkWidget *previous_parent)

Indentation.

@@ +145,3 @@
+  parent = gtk_widget_get_parent (widget);
+  if (parent == NULL)
+	  return;

This seems wrong? Don't you need to check first if previous_parent is != NULL, and disconnect all methods, and then check if current parent is != NULL, and connect all methods?

@@ +149,3 @@
+  g_signal_connect_swapped (parent,
+                            "motion-notify-event",
+                            G_CALLBACK(parent_motion_notify_event),

Space before (.

@@ +153,3 @@
+  g_signal_connect_swapped (parent,
+                            "leave-notify-event",
+                            G_CALLBACK(parent_leave_notify_event),

Same.

@@ +157,3 @@
+  g_signal_connect_swapped (parent,
+                            "size-allocate",
+                            G_CALLBACK(parent_size_allocate),

Same.

@@ +162,3 @@
+    return;
+
+  g_signal_handlers_disconnect_by_func (parent,

I suppose this should be previous_parent?

@@ +178,3 @@
+static gboolean
+child_motion_notify_event (GtkWidget *widget,
+                            GdkEventMotion *event)

Indent.

@@ +180,3 @@
+                            GdkEventMotion *event)
+{
+  EphyOverlayEscapingChildPrivate *priv = EPHY_OVERLAY_ESCAPING_CHILD(widget)->priv;

space before (

@@ +184,3 @@
+  event->x += priv->initial_allocation.x;
+  event->y += priv->initial_allocation.y;
+  return parent_motion_notify_event(widget, event, gtk_widget_get_parent (widget));

space before (

Hrm, can't we just change the allocation here to be below the event->y? Since we know stuff already interesects and so...

@@ +199,3 @@
+
+  widget_class->parent_set = ephy_overlay_escaping_child_parent_set;
+  widget_class->motion_notify_event = child_motion_notify_event;

Inconsistent naming!

@@ +209,3 @@
+                                                      G_MAXUINT,
+                                                      20,
+                                                      G_PARAM_CONSTRUCT_ONLY

Is there any reason for this to be CONSTRUCT_ONLY, actually?

@@ +211,3 @@
+                                                      G_PARAM_CONSTRUCT_ONLY
+                                                          | G_PARAM_READWRITE
+                                                          | G_PARAM_STATIC_STRINGS));

We usually put this in one line, or otherwise the | goes in the previous line (see gedit-overlay.c)

@@ +221,3 @@
+          EphyOverlayEscapingChildPrivate);
+
+  self->priv = priv;

self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, ...);

@@ +270,3 @@
+ * Creates a new #EphyOverlayEscapingChild object
+ *
+ * Returns: a new #EphyOverlayChild object

#EphyOverlayEscapingChild

@@ +279,3 @@
+                       widget,
+                       "escaping-distance",
+                       escaping_distance,

Group property name and values.

::: lib/widgets/ephy-overlay-escaping-child.h
@@ +1,2 @@
+/*
+ * ephy-overlay-escaping-child.h - Header for EphyOverlayEscapingChild

Uh, we don't really put this in headers.

@@ +23,3 @@
+
+#include "gedit-overlay-child.h"
+

Extra blank line.

@@ +51,3 @@
+GType                      ephy_overlay_escaping_child_get_type (void) G_GNUC_CONST;
+
+EphyOverlayEscapingChild *ephy_overlay_escaping_child_new       (GtkWidget *widget, guint escaping_distance);

Align the prototypes.
Comment 20 Xan Lopez 2011-04-16 04:29:17 UTC
*** Bug 626773 has been marked as a duplicate of this bug. ***
Comment 21 Xan Lopez 2011-04-17 14:47:19 UTC
*** Bug 647997 has been marked as a duplicate of this bug. ***
Comment 22 Alexandre Mazari 2011-04-19 11:04:02 UTC
Created attachment 186269 [details] [review]
Fix indentation issues according to xan's review.
Comment 23 Alexandre Mazari 2011-04-19 11:16:27 UTC
Created attachment 186270 [details] [review]
Add a constructor using the default distance prop value (currently set to 20 pixels).

Update ctors documentation.
Comment 24 Ignacio Casal Quinteiro (nacho) 2011-04-19 11:25:18 UTC
Review of attachment 186270 [details] [review]:

Some more comments.

::: embed/ephy-embed.c
@@ +529,2 @@
   gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
+  escaping_child = ephy_overlay_escaping_child_new (frame, 20);

remove 20 from the constructor?
it will relay on the default value and in case it needs to be changed you can always change it with the property.

@@ +529,3 @@
   gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
+  escaping_child = ephy_overlay_escaping_child_new (frame, 20);
+  gedit_overlay_add (GEDIT_OVERLAY (overlay), escaping_child, GEDIT_OVERLAY_CHILD_POSITION_SOUTH_WEST, 0);

let's break the line here to not have such a big line

::: lib/widgets/ephy-overlay-escaping-child.c
@@ +55,3 @@
+/* this should be in Gdk...really */
+static gboolean
+is_point_in_rectangle (int point_x, int point_y, GdkRectangle rectangle)

break on parameters

@@ +57,3 @@
+is_point_in_rectangle (int point_x, int point_y, GdkRectangle rectangle)
+{
+  return point_x >= rectangle.x && point_x < rectangle.x + rectangle.width 

let's add here some parenthesis to make it more readable

@@ +70,3 @@
+                            GtkWidget *parent)
+{
+  EphyOverlayEscapingChildPrivate *priv =

same line

@@ +104,3 @@
+                      GtkWidget      *parent)
+{
+  EphyOverlayEscapingChildPrivate *priv = EPHY_OVERLAY_ESCAPING_CHILD(widget)->priv;

space before (

@@ +234,3 @@
+                                                      G_MAXUINT,
+                                                      20,
+                                                      G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

break on |

@@ +255,3 @@
+ */
+EphyOverlayEscapingChild *
+ephy_overlay_escaping_child_new (GtkWidget *widget, guint escaping_distance)

break on params

::: lib/widgets/gedit-overlay-child.c
@@ +105,3 @@
 	attributes.window_type = GDK_WINDOW_CHILD;
 	attributes.wclass = GDK_INPUT_OUTPUT;
+	attributes.event_mask = GDK_EXPOSURE_MASK | GDK_POINTER_MOTION_MASK;;

it should go in the inherited class

::: lib/widgets/gedit-overlay.c
@@ +210,3 @@
 	attributes.visual = gtk_widget_get_visual (widget);
 	attributes.event_mask = gtk_widget_get_events (widget);
+	attributes.event_mask |= GDK_EXPOSURE_MASK | GDK_BUTTON_PRESS_MASK | GDK_LEAVE_NOTIFY_MASK;

file a bug in gedit for this
Comment 25 Alexandre Mazari 2011-04-19 11:29:55 UTC
Hi Xan,
Thanks for your review. I hopw the newer patch fixes the style/indentation remarks you had.

> passing an explicit distance? Either two functions or passing '-1' to use
> whatever the default is.
I added a constructor to use the default value.


> There is GdkPoint in GdkRegion.
Indeed, but if i understand right, instanciating a GdkRegion is a bit costly, as it copies the pixels. Also isn't GdkRegion deprecated in favor of Cairo stuff ?


> @@ +80,3 @@
> +parent_motion_notify_event (GtkWidget *widget,
> +                            GdkEventMotion *event,
> +                            GtkWidget *parent)
> 
> parent is not used in the whole method.

Yep, dont need it, really. Still we should be notified of mouse motion event on the parent.

> This seems wrong? Don't you need to check first if previous_parent is != NULL,
> and disconnect all methods, and then check if current parent is != NULL, and
> connect all methods?

Was wrong indeed. Fixed it. See below
> I suppose this should be previous_parent?

Absolutely.

> 
> Hrm, can't we just change the allocation here to be below the event->y? Since
> we know stuff already interesects and so...

Well, that's basically what is hapenning, but i just wanted to reuse the code and codepath for parent's mouse motion event.


> Is there any reason for this to be CONSTRUCT_ONLY, actually?

Good question. What is your opinion ? Do you see a situation where we would change the escaping distance afterwards ?
Comment 26 Alexandre Mazari 2011-04-19 13:36:40 UTC
Created attachment 186284 [details] [review]
Fixes remaining indentation glitches, modify the child's event window mask in the

subclass.
Comment 27 Ignacio Casal Quinteiro (nacho) 2011-04-19 13:42:17 UTC
Review of attachment 186284 [details] [review]:

Wrong patch?
Comment 28 Alexandre Mazari 2011-04-19 13:52:39 UTC
(In reply to comment #24)
> Review of attachment 186270 [details] [review]:
> 
> Some more comments.
> 
> ::: embed/ephy-embed.c
> @@ +529,2 @@
>    gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
> +  escaping_child = ephy_overlay_escaping_child_new (frame, 20);
> 
> remove 20 from the constructor?
> it will relay on the default value and in case it needs to be changed you can
> always change it with the property.
> 
> @@ +529,3 @@
>    gtk_container_add (GTK_CONTAINER (frame), priv->statusbar_label);
> +  escaping_child = ephy_overlay_escaping_child_new (frame, 20);
> +  gedit_overlay_add (GEDIT_OVERLAY (overlay), escaping_child,
> GEDIT_OVERLAY_CHILD_POSITION_SOUTH_WEST, 0);
> 
> let's break the line here to not have such a big line
> 
> ::: lib/widgets/ephy-overlay-escaping-child.c
> @@ +55,3 @@
> +/* this should be in Gdk...really */
> +static gboolean
> +is_point_in_rectangle (int point_x, int point_y, GdkRectangle rectangle)
> 
> break on parameters
> 
> @@ +57,3 @@
> +is_point_in_rectangle (int point_x, int point_y, GdkRectangle rectangle)
> +{
> +  return point_x >= rectangle.x && point_x < rectangle.x + rectangle.width 
> 
> let's add here some parenthesis to make it more readable
> 
> @@ +70,3 @@
> +                            GtkWidget *parent)
> +{
> +  EphyOverlayEscapingChildPrivate *priv =
> 
> same line
> 
> @@ +104,3 @@
> +                      GtkWidget      *parent)
> +{
> +  EphyOverlayEscapingChildPrivate *priv =
> EPHY_OVERLAY_ESCAPING_CHILD(widget)->priv;
> 
> space before (
> 
> @@ +234,3 @@
> +                                                      G_MAXUINT,
> +                                                      20,
> +                                                      G_PARAM_CONSTRUCT_ONLY |
> G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> 
> break on |
Thanks for your review!

> 
> @@ +255,3 @@
> + */
> +EphyOverlayEscapingChild *
> +ephy_overlay_escaping_child_new (GtkWidget *widget, guint escaping_distance)
> 
> break on params
done
> 
> ::: lib/widgets/gedit-overlay-child.c
> @@ +105,3 @@
>      attributes.window_type = GDK_WINDOW_CHILD;
>      attributes.wclass = GDK_INPUT_OUTPUT;
> +    attributes.event_mask = GDK_EXPOSURE_MASK | GDK_POINTER_MOTION_MASK;;
> 
> it should go in the inherited class
> 
done in a override of 'realize'. Is that the right place ?

> ::: lib/widgets/gedit-overlay.c
> @@ +210,3 @@
>      attributes.visual = gtk_widget_get_visual (widget);
>      attributes.event_mask = gtk_widget_get_events (widget);
> +    attributes.event_mask |= GDK_EXPOSURE_MASK | GDK_BUTTON_PRESS_MASK |
> GDK_LEAVE_NOTIFY_MASK;
> 
> file a bug in gedit for this

https://bugzilla.gnome.org/show_bug.cgi?id=648214
Comment 29 Alexandre Mazari 2011-04-19 13:57:37 UTC
Created attachment 186290 [details] [review]
Slide out the status overlay when the mouse pointer goes close by.

Wrong patch indeed, sorry for that.
Fixes remaining indentation glitches, modify the child's event window mask in the
subclass.
Comment 30 Ignacio Casal Quinteiro (nacho) 2011-04-19 14:01:11 UTC
Review of attachment 186290 [details] [review]:

Another comment: align the parameters of the methods.

::: lib/widgets/ephy-overlay-escaping-child.h
@@ +50,3 @@
+
+EphyOverlayEscapingChild  *ephy_overlay_escaping_child_new               (GtkWidget *widget);
+EphyOverlayEscapingChild  *ephy_overlay_escaping_child_new_with_distance (GtkWidget *widget, guint escaping_distance);

break on parameters.
Comment 31 Xan Lopez 2011-04-25 22:07:42 UTC
I'm going to push this for ephy 3.0.1 since I think we really have to have it. I'll leave the bug open in case we want to make more changes to the patch.
Comment 32 Xan Lopez 2011-04-25 22:11:12 UTC
Comment: right now if you hover over a link in the statusbar area we'll briefly show and hide the statusbar. I think it should never be shown, probably.
Comment 33 Xan Lopez 2012-01-17 10:30:08 UTC
So, we use NautilusFloatBar now, which already escapes in a similar way to how Firefox does it. Closing.