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

Proposed by Andrea Cimitan
Status: Merged
Approved by: Ted Gould
Approved revision: 250
Merged at revision: 248
Proposed branch: lp:~cimi/overlay-scrollbar/immediate-pager-set-state
Merge into: lp:overlay-scrollbar
Diff against target: 231 lines (+68/-23)
4 files modified
os/os-animation.c (+18/-0)
os/os-pager.c (+17/-8)
os/os-private.h (+4/-1)
os/os-scrollbar.c (+29/-14)
To merge this branch: bzr merge lp:~cimi/overlay-scrollbar/immediate-pager-set-state
Reviewer Review Type Date Requested Status
Ted Gould (community) Approve
Review via email: mp+64147@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ted Gould (ted) wrote :

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
Revision history for this message
Andrea Cimitan (cimi) wrote :

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

Revision history for this message
Ted Gould (ted) wrote :

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.

249. By Andrea Cimitan

Replaced goto with code pasted

250. By Andrea Cimitan

Improved if statements following ted's suggestions

Revision history for this message
Ted Gould (ted) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'os/os-animation.c'
2--- os/os-animation.c 2011-06-09 14:52:24 +0000
3+++ os/os-animation.c 2011-06-12 23:22:28 +0000
4@@ -133,6 +133,24 @@
5 }
6
7 /**
8+ * os_animation_is_running:
9+ * @animation: a #OsAnimation
10+ *
11+ * Returns TRUE if the animation is running
12+ **/
13+gboolean
14+os_animation_is_running (OsAnimation *animation)
15+{
16+ OsAnimationPrivate *priv;
17+
18+ g_return_if_fail (animation != NULL);
19+
20+ priv = animation->priv;
21+
22+ return priv->source_id != 0;
23+}
24+
25+/**
26 * os_animation_set_duration:
27 * @animation: a #OsAnimation
28 * @duration: the new duration
29
30=== modified file 'os/os-pager.c'
31--- os/os-pager.c 2011-06-09 14:47:37 +0000
32+++ os/os-pager.c 2011-06-12 23:22:28 +0000
33@@ -434,12 +434,14 @@
34 * os_pager_set_active:
35 * @pager: a #OsPager
36 * @active: whether is active or not
37+ * @animation: whether animate it or not
38 *
39 * Changes the activity state of @pager.
40 **/
41 void
42 os_pager_set_active (OsPager *pager,
43- gboolean active)
44+ gboolean active,
45+ gboolean animate)
46 {
47 OsPagerPrivate *priv;
48
49@@ -447,24 +449,31 @@
50
51 priv = pager->priv;
52
53- if (priv->active != active)
54+ /* set the state and draw even if there's an animation running, that is
55+ * (!animate && os_animation_is_running (priv->animation)). */
56+ if ((priv->active != active) ||
57+ (!animate && os_animation_is_running (priv->animation)))
58 {
59+ gboolean visible;
60+
61 priv->active = active;
62
63 if (priv->parent == NULL)
64 return;
65
66- /* only start the animation if the pager is visible. */
67- if (gdk_window_is_visible (priv->pager_window))
68+ visible = gdk_window_is_visible (priv->pager_window);
69+
70+ if (visible)
71+ os_animation_stop (priv->animation, NULL);
72+
73+ if (visible && animate)
74 {
75- os_animation_stop (priv->animation, NULL);
76-
77 os_animation_set_duration (priv->animation, priv->active ? DURATION_FADE_IN :
78 DURATION_FADE_OUT);
79-
80 os_animation_start (priv->animation);
81 }
82- else
83+
84+ if (!visible || !animate)
85 {
86 priv->weight = 1.0f;
87
88
89=== modified file 'os/os-private.h'
90--- os/os-private.h 2011-06-09 14:52:24 +0000
91+++ os/os-private.h 2011-06-12 23:22:28 +0000
92@@ -133,6 +133,8 @@
93 OsAnimationEndFunc end_func,
94 gpointer user_data);
95
96+gboolean os_animation_is_running (OsAnimation *animation);
97+
98 void os_animation_set_duration (OsAnimation *animation,
99 gint32 duration);
100
101@@ -217,7 +219,8 @@
102 GdkRectangle mask);
103
104 void os_pager_set_active (OsPager *overlay,
105- gboolean active);
106+ gboolean active,
107+ gboolean animate);
108
109 void os_pager_set_detached (OsPager *overlay,
110 gboolean detached);
111
112=== modified file 'os/os-scrollbar.c'
113--- os/os-scrollbar.c 2011-06-09 17:12:44 +0000
114+++ os/os-scrollbar.c 2011-06-12 23:22:28 +0000
115@@ -279,7 +279,7 @@
116 priv = scrollbar->priv;
117
118 if (priv->pager != NULL && priv->can_deactivate_pager)
119- os_pager_set_active (OS_PAGER (priv->pager), FALSE);
120+ os_pager_set_active (OS_PAGER (priv->pager), FALSE, TRUE);
121 }
122
123 /* timeout before deactivating the pager */
124@@ -750,12 +750,12 @@
125 (y > allocation.y && y < allocation.y + allocation.height))
126 {
127 priv->can_deactivate_pager = FALSE;
128- os_pager_set_active (OS_PAGER (priv->pager), TRUE);
129+ os_pager_set_active (OS_PAGER (priv->pager), TRUE, TRUE);
130 }
131 else
132 {
133 priv->can_deactivate_pager = TRUE;
134- os_pager_set_active (OS_PAGER (priv->pager), FALSE);
135+ os_pager_set_active (OS_PAGER (priv->pager), FALSE, TRUE);
136 }
137 }
138
139@@ -789,7 +789,7 @@
140 priv->active_window = TRUE;
141
142 priv->can_deactivate_pager = FALSE;
143- os_pager_set_active (OS_PAGER (priv->pager), TRUE);
144+ os_pager_set_active (OS_PAGER (priv->pager), TRUE, TRUE);
145 }
146 else if (priv->active_window)
147 {
148@@ -828,7 +828,7 @@
149 {
150 /* if the pointer is outside of the window, set it inactive. */
151 priv->can_deactivate_pager = TRUE;
152- os_pager_set_active (OS_PAGER (priv->pager), FALSE);
153+ os_pager_set_active (OS_PAGER (priv->pager), FALSE, TRUE);
154 }
155
156 if ((current_time > end_time) && priv->thumb != NULL)
157@@ -1069,12 +1069,18 @@
158 {
159 Display *display;
160 OsScrollbar *scrollbar;
161+ OsScrollbarPrivate *priv;
162 XWindowChanges changes;
163 guint32 xid, xid_parent;
164 unsigned int value_mask = CWSibling | CWStackMode;
165 int res;
166
167 scrollbar = OS_SCROLLBAR (user_data);
168+ priv = scrollbar->priv;
169+
170+ /* immediately set the pager to be active. */
171+ priv->can_deactivate_pager = FALSE;
172+ os_pager_set_active (OS_PAGER (priv->pager), TRUE, FALSE);
173
174 xid = GDK_WINDOW_XID (gtk_widget_get_window (widget));
175 xid_parent = GDK_WINDOW_XID (gtk_widget_get_window (GTK_WIDGET (scrollbar)));
176@@ -1438,7 +1444,7 @@
177 else
178 {
179 priv->can_deactivate_pager = FALSE;
180- os_pager_set_active (OS_PAGER (priv->pager), TRUE);
181+ os_pager_set_active (OS_PAGER (priv->pager), TRUE, TRUE);
182 }
183 }
184
185@@ -1569,12 +1575,16 @@
186 * this call checks the pointer after the scroll-event,
187 * since it enters the window,
188 * then sets the state accordingly. */
189- if (!priv->active_window && xiev->evtype == XI_Enter)
190- {
191- XIEnterEvent *xiee = xev->xcookie.data;
192
193- pager_set_state_from_pointer (scrollbar, xiee->event_x, xiee->event_y);
194- }
195+ /* FIXME(Cimi) code commented out until I find what and where is wrong. */
196+// if (!priv->active_window && xiev->evtype == XI_Enter)
197+// {
198+// XIEnterEvent *xiee = xev->xcookie.data;
199+//
200+// /* if the thumb is mapped, the pager should be active and should remain active. */
201+// if (gtk_widget_get_mapped (priv->thumb))
202+// pager_set_state_from_pointer (scrollbar, xiee->event_x, xiee->event_y);
203+// }
204
205 if (xiev->evtype == XI_Leave)
206 {
207@@ -1813,8 +1823,13 @@
208 * this call checks the pointer after the scroll-event,
209 * since it enters the window,
210 * then sets the state accordingly. */
211- if (!priv->active_window && xev->type == EnterNotify)
212- pager_set_state_from_pointer (scrollbar, xev->xcrossing.x, xev->xcrossing.y);
213+ /* if the thumb is mapped, the pager should be active and should remain active. */
214+
215+ /* FIXME(Cimi) code commented out until I find what and where is wrong. */
216+// if (!priv->active_window &&
217+// xev->type == EnterNotify &&
218+// gtk_widget_get_mapped (priv->thumb))
219+// pager_set_state_from_pointer (scrollbar, xev->xcrossing.x, xev->xcrossing.y);
220
221 if (xev->type == LeaveNotify)
222 {
223@@ -2202,7 +2217,7 @@
224 /* on map-event of an active window,
225 * the pager should be active. */
226 priv->can_deactivate_pager = FALSE;
227- os_pager_set_active (OS_PAGER (priv->pager), TRUE);
228+ os_pager_set_active (OS_PAGER (priv->pager), TRUE, FALSE);
229 }
230
231 if (priv->fullsize == FALSE)

Subscribers

People subscribed via source and target branches