Merge lp:~alexeftimie/software-center/fix-704719-briefly-wrong-display into lp:software-center

Proposed by Alex Eftimie
Status: Merged
Merged at revision: 1656
Proposed branch: lp:~alexeftimie/software-center/fix-704719-briefly-wrong-display
Merge into: lp:software-center
Diff against target: 15 lines (+4/-1)
1 file modified
softwarecenter/view/navhistory.py (+4/-1)
To merge this branch: bzr merge lp:~alexeftimie/software-center/fix-704719-briefly-wrong-display
Reviewer Review Type Date Requested Status
Michael Vogt Approve
Review via email: mp+55332@code.launchpad.net

Description of the change

Change the NavigationItem constructor, so that the current_app isn't set when viewing a list.

This will fix a briefly wrong display when navigating backwards (see bug: #704719)

To post a comment you must log in.
Revision history for this message
Gary Lasker (gary-lasker) 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

Revision history for this message
Alex Eftimie (alexeftimie) wrote :

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

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for this branch. The fix looks great!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/view/navhistory.py'
2--- softwarecenter/view/navhistory.py 2011-02-11 15:26:41 +0000
3+++ softwarecenter/view/navhistory.py 2011-03-29 13:14:23 +0000
4@@ -130,7 +130,10 @@
5 self.apps_category = available_pane.apps_category
6 self.apps_subcategory = available_pane.apps_subcategory
7 self.apps_search_term = available_pane.apps_search_term
8- self.current_app = available_pane.get_current_app()
9+ if available_pane.is_applist_view_showing():
10+ self.current_app = None
11+ else:
12+ self.current_app = available_pane.get_current_app()
13 self.parts = self.available_pane.navigation_bar.get_parts()
14
15 def navigate_to(self):