Merge lp:~cimi/overlay-scrollbar/immediate-pager-set-state into lp:overlay-scrollbar

[Andrea Cimitan]

[Ted Gould (ted)]

The "goto draw" there is really bad. You should really redo the logic so that it's contained in the if statement above.

review: Needs Fixing

[Andrea Cimitan (cimi)]

a goto is not "really bad".
I can simply copy & paste the code then, instead writing an enormous if statement

[Ted Gould (ted)]

On Fri, 2011-06-10 at 15:26 +0000, Andrea Cimitan wrote:
> a goto is not "really bad".
> I can simply copy & paste the code then, instead writing
> an enormous if statement

Yes, it is :-) There are some cases where the increased complexity does
factor into whether it makes sense, but this isn't one of those cases.

Here's what we're starting with:

      if (gdk_window_is_visible (priv->pager_window))
        {
          os_animation_stop (priv->animation);

          os_animation_set_duration (priv->animation, priv->active ?
DURATION_FADE_IN :

DURATION_FADE_OUT);

          os_animation_start (priv->animation);
        }
      else
        {
          priv->weight = 1.0f;

          os_pager_draw (pager);
        }

And where we want to go is to have three different sections instead of
two. The easiest way to get there (and the compiler will optimize well
with) is to cache the visible property.

gboolean visible = gdk_window_is_visible (priv->pager_window);

if (visible) {
    os_animation_stop (priv->animation);

    os_animation_set_duration (priv->animation, priv->active ?
                                                DURATION_FADE_IN :
                                                DURATION_FADE_OUT);
}

if (animate && visible) {
    os_animation_start (priv->animation);
}

if (!visible || !animate) {
    priv->weight = 1.0f;

    os_pager_draw (pager);
}

We didn't have to increase the if depth at all and made the code pretty
easy to read as we can see the conditions where each function is called.

[lp:~cimi/overlay-scrollbar/immediate-pager-set-state]

249. By Andrea Cimitan on 2011-06-10

Replaced goto with code pasted

250. By Andrea Cimitan on 2011-06-12

Improved if statements following ted's suggestions

[Ted Gould (ted)]

review: Approve

=== modified file 'os/os-animation.c'
--- os/os-animation.c	2011-06-09 14:52:24 +0000
+++ os/os-animation.c	2011-06-12 23:22:28 +0000
@@ -133,6 +133,24 @@
 }
 
 /**
+ * os_animation_is_running:
+ * @animation: a #OsAnimation
+ *
+ * Returns TRUE if the animation is running
+ **/
+gboolean
+os_animation_is_running (OsAnimation *animation)
+{
+  OsAnimationPrivate *priv;
+
+  g_return_if_fail (animation != NULL);
+
+  priv = animation->priv;
+
+  return priv->source_id != 0;
+}
+
+/**
  * os_animation_set_duration:
  * @animation: a #OsAnimation
  * @duration: the new duration

=== modified file 'os/os-pager.c'
--- os/os-pager.c	2011-06-09 14:47:37 +0000
+++ os/os-pager.c	2011-06-12 23:22:28 +0000
@@ -434,12 +434,14 @@
  * os_pager_set_active:
  * @pager: a #OsPager
  * @active: whether is active or not
+ * @animation: whether animate it or not
  *
  * Changes the activity state of @pager.
  **/
 void
 os_pager_set_active (OsPager *pager,
-                     gboolean active)
+                     gboolean active,
+                     gboolean animate)
 {
   OsPagerPrivate *priv;
 
@@ -447,24 +449,31 @@
 
   priv = pager->priv;
 
-  if (priv->active != active)
+  /* set the state and draw even if there's an animation running, that is
+   * (!animate && os_animation_is_running (priv->animation)). */
+  if ((priv->active != active) ||
+      (!animate && os_animation_is_running (priv->animation)))
     {
+      gboolean visible;
+
       priv->active = active;
 
       if (priv->parent == NULL)
         return;
 
-      /* only start the animation if the pager is visible. */
-      if (gdk_window_is_visible (priv->pager_window))
+      visible = gdk_window_is_visible (priv->pager_window);
+
+      if (visible)
+        os_animation_stop (priv->animation, NULL);
+
+      if (visible && animate)
         {
-          os_animation_stop (priv->animation, NULL);
-
           os_animation_set_duration (priv->animation, priv->active ? DURATION_FADE_IN :
                                                                      DURATION_FADE_OUT);
-
           os_animation_start (priv->animation);
         }
-      else
+
+      if (!visible || !animate)
         {
           priv->weight = 1.0f;
 

=== modified file 'os/os-private.h'
--- os/os-private.h	2011-06-09 14:52:24 +0000
+++ os/os-private.h	2011-06-12 23:22:28 +0000
@@ -133,6 +133,8 @@
                                         OsAnimationEndFunc    end_func,
                                         gpointer              user_data);
 
+gboolean     os_animation_is_running   (OsAnimation *animation);
+
 void         os_animation_set_duration (OsAnimation *animation,
                                         gint32       duration);
 
@@ -217,7 +219,8 @@
                                  GdkRectangle mask);
 
 void     os_pager_set_active    (OsPager *overlay,
-                                 gboolean active);
+                                 gboolean active,
+                                 gboolean animate);
 
 void     os_pager_set_detached  (OsPager *overlay,
                                  gboolean detached);

=== modified file 'os/os-scrollbar.c'
--- os/os-scrollbar.c	2011-06-09 17:12:44 +0000
+++ os/os-scrollbar.c	2011-06-12 23:22:28 +0000
@@ -279,7 +279,7 @@
   priv = scrollbar->priv;
 
   if (priv->pager != NULL && priv->can_deactivate_pager)
-    os_pager_set_active (OS_PAGER (priv->pager), FALSE);
+    os_pager_set_active (OS_PAGER (priv->pager), FALSE, TRUE);
 }
 
 /* timeout before deactivating the pager */
@@ -750,12 +750,12 @@
       (y > allocation.y && y < allocation.y + allocation.height))
     {
       priv->can_deactivate_pager = FALSE;
-      os_pager_set_active (OS_PAGER (priv->pager), TRUE);
+      os_pager_set_active (OS_PAGER (priv->pager), TRUE, TRUE);
     }
   else
     {
       priv->can_deactivate_pager = TRUE;
-      os_pager_set_active (OS_PAGER (priv->pager), FALSE);
+      os_pager_set_active (OS_PAGER (priv->pager), FALSE, TRUE);
     }
 }
 
@@ -789,7 +789,7 @@
           priv->active_window = TRUE;
 
           priv->can_deactivate_pager = FALSE;
-          os_pager_set_active (OS_PAGER (priv->pager), TRUE);
+          os_pager_set_active (OS_PAGER (priv->pager), TRUE, TRUE);
         }
       else if (priv->active_window)
         {
@@ -828,7 +828,7 @@
             {
               /* if the pointer is outside of the window, set it inactive. */
               priv->can_deactivate_pager = TRUE;
-              os_pager_set_active (OS_PAGER (priv->pager), FALSE);
+              os_pager_set_active (OS_PAGER (priv->pager), FALSE, TRUE);
             }
 
           if ((current_time > end_time) && priv->thumb != NULL)
@@ -1069,12 +1069,18 @@
 {
   Display *display;
   OsScrollbar *scrollbar;
+  OsScrollbarPrivate *priv;
   XWindowChanges changes;
   guint32 xid, xid_parent;
   unsigned int value_mask = CWSibling | CWStackMode;
   int res;
 
   scrollbar = OS_SCROLLBAR (user_data);
+  priv = scrollbar->priv;
+
+  /* immediately set the pager to be active. */
+  priv->can_deactivate_pager = FALSE;
+  os_pager_set_active (OS_PAGER (priv->pager), TRUE, FALSE);
 
   xid = GDK_WINDOW_XID (gtk_widget_get_window (widget));
   xid_parent = GDK_WINDOW_XID (gtk_widget_get_window (GTK_WIDGET (scrollbar)));
@@ -1438,7 +1444,7 @@
       else
         {
           priv->can_deactivate_pager = FALSE;
-          os_pager_set_active (OS_PAGER (priv->pager), TRUE);
+          os_pager_set_active (OS_PAGER (priv->pager), TRUE, TRUE);
         }
     }
 
@@ -1569,12 +1575,16 @@
            * this call checks the pointer after the scroll-event,
            * since it enters the window,
            * then sets the state accordingly. */
-          if (!priv->active_window && xiev->evtype == XI_Enter)
-            {
-              XIEnterEvent *xiee = xev->xcookie.data;
 
-              pager_set_state_from_pointer (scrollbar, xiee->event_x, xiee->event_y);
-            }
+          /* FIXME(Cimi) code commented out until I find what and where is wrong. */
+//          if (!priv->active_window && xiev->evtype == XI_Enter)
+//            {
+//              XIEnterEvent *xiee = xev->xcookie.data;
+//
+//              /* if the thumb is mapped, the pager should be active and should remain active. */
+//              if (gtk_widget_get_mapped (priv->thumb))
+//                pager_set_state_from_pointer (scrollbar, xiee->event_x, xiee->event_y);
+//            }
 
           if (xiev->evtype == XI_Leave)
             {
@@ -1813,8 +1823,13 @@
        * this call checks the pointer after the scroll-event,
        * since it enters the window,
        * then sets the state accordingly. */
-      if (!priv->active_window && xev->type == EnterNotify)
-        pager_set_state_from_pointer (scrollbar, xev->xcrossing.x, xev->xcrossing.y);
+       /* if the thumb is mapped, the pager should be active and should remain active. */
+
+  /* FIXME(Cimi) code commented out until I find what and where is wrong. */
+//      if (!priv->active_window &&
+//          xev->type == EnterNotify &&
+//          gtk_widget_get_mapped (priv->thumb))
+//        pager_set_state_from_pointer (scrollbar, xev->xcrossing.x, xev->xcrossing.y);
 
       if (xev->type == LeaveNotify)
         {
@@ -2202,7 +2217,7 @@
       /* on map-event of an active window,
        * the pager should be active. */
       priv->can_deactivate_pager = FALSE;
-      os_pager_set_active (OS_PAGER (priv->pager), TRUE);
+      os_pager_set_active (OS_PAGER (priv->pager), TRUE, FALSE);
     }
 
   if (priv->fullsize == FALSE)