Code review comment for lp:~alexeftimie/software-center/fix-704719-briefly-wrong-display

Hi Gary,

I am confident that this (second) patch does what it should and
doesn't have side effects.

As one can see, the cur_app property of an NavigationItem object is
used in navigate_to, with a direct call to show_app; for an
NavigationItem created in applist view, it doesn't make sense to show
the current app; changing the actual view is managed by the update_cb,
which is added on idle gtk time. I don't see a case when some other
component will rely on cur_app _from_ NavigationItem. Setting it to
None just prevents the flick.

I have a question though: why just the callback is queued for
on-gobject-idle, and show_app(cur_app) is not?

Alex

On 3/31/11, Gary Lasker <email address hidden> wrote:
> Hey Alex, thanks for the patch! Looking at the code, it seems a reasonable
> fix and I don't see anything particularly that I'd expect to cause a
> problem.
>
> I verified that this change does indeed fix the problem described in bug
> 704719. Finally, I was not able to find any regressions when I tested
> various combinations of navigations back and forth.
>
> Alex, how about your experience with it? Do you feel pretty confident in
> your testing that this change doesn't cause any regressions in the
> navigation?
>
> Thanks!
> Gary
> --
> https://code.launchpad.net/~alexeftimie/software-center/fix-704719-briefly-wrong-display/+merge/55332
> You are the owner of
> lp:~alexeftimie/software-center/fix-704719-briefly-wrong-display.
>

--
Alex Eftimie