Merge lp:~cimi/overlay-scrollbar/fix-754927 into lp:overlay-scrollbar

Proposed by Andrea Cimitan
Status: Merged
Merged at revision: 209
Proposed branch: lp:~cimi/overlay-scrollbar/fix-754927
Merge into: lp:overlay-scrollbar
Diff against target: 37 lines (+11/-2)
1 file modified
os/os-pager.c (+11/-2)
To merge this branch: bzr merge lp:~cimi/overlay-scrollbar/fix-754927
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Loïc Molinari (community) Approve
David Barth (community) Needs Information
Review via email: mp+57452@code.launchpad.net

Description of the change

simply add parent =! NULL check

To post a comment you must log in.
208. By Andrea Cimitan

check if priv->animation != NULL

Revision history for this message
David Barth (dbarth) wrote :

The changes are ok here, but I think there's more to the bug.

1. the user_data may contain a dead pointer, so even getting access to ->priv may fail
2. in order to avoid dead pointers still reaching the callback, shouldn't you disconnect the animation signal as well on dispose?

review: Needs Information
Revision history for this message
Andrea Cimitan (cimi) wrote :

1. when user_data is NULL then the animation should be already stopped, in fact in (...)
2. (...) dispose I am running at first g_object_unref (priv->animation), and os-animation on dispose signal is taking care or stopping the animation itself.

A quick opinion of loic could be enough, though I think this should be fine

Revision history for this message
Andrea Cimitan (cimi) wrote :

I merged by mistake (I backported that in a branch I was testing, and I pushed that branch with the changes inside).
I could continue developing, though

Revision history for this message
Loïc Molinari (loic.molinari) wrote :

1) The design ensures the pager is alive all along the animation lifetime.

2) The pager destroys the animation object when it's disposed and I confirm the animation object takes care of removing the source from the mainloop when it's disposed.

review: Approve
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

 review approve

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'os/os-pager.c'
2--- os/os-pager.c 2011-04-08 21:08:32 +0000
3+++ os/os-pager.c 2011-04-13 09:56:17 +0000
4@@ -128,6 +128,9 @@
5
6 priv->weight = weight;
7
8+ if (priv->parent == NULL)
9+ return;
10+
11 os_pager_draw (pager);
12 }
13
14@@ -196,8 +199,10 @@
15 pager = OS_PAGER (user_data);
16 priv = pager->priv;
17
18- if (priv->parent != NULL && priv->pager_window != NULL)
19- os_pager_draw (pager);
20+ if (priv->parent == NULL || priv->pager_window == NULL)
21+ return;
22+
23+ os_pager_draw (pager);
24 }
25
26 /* Type definition. */
27@@ -411,6 +416,10 @@
28
29 priv = pager->priv;
30
31+ /* stop currently running animation. */
32+ if (priv->animation != NULL)
33+ os_animation_stop (priv->animation);
34+
35 if (priv->parent != NULL)
36 {
37 g_object_unref (priv->parent);

Subscribers

People subscribed via source and target branches