Merge lp:~gary-lasker/software-center/remember-sort-preference-lp966878 into lp:software-center

Proposed by Gary Lasker on 2012-04-09
Status: Merged
Merged at revision: 2954
Proposed branch: lp:~gary-lasker/software-center/remember-sort-preference-lp966878
Merge into: lp:software-center
Diff against target: 41 lines (+5/-4)
3 files modified
softwarecenter/ui/gtk3/panes/availablepane.py (+1/-1)
softwarecenter/ui/gtk3/panes/softwarepane.py (+4/-2)
softwarecenter/ui/gtk3/views/appview.py (+0/-1)
To merge this branch: bzr merge lp:~gary-lasker/software-center/remember-sort-preference-lp966878
Reviewer Review Type Date Requested Status
Michael Vogt 2012-04-09 Approve on 2012-04-10
Review via email: mp+101306@code.launchpad.net

Description of the change

This is a fix for bug 966878. A simple fix to not reset the user's sorting preference if they have set one. We should respect the setting when moving around the UI, as we already do when searching in the categories screens. Please check bug 966878 for details and steps to reproduce.

Thanks!

To post a comment you must log in.
Michael Vogt (mvo) wrote :

Thanks for your branch! I like the idea of going for a simple fix, unfortunately this seems to only
fix parts of the issue as e.g.:
- searching for "foo"
- change "by name"
- go back
- go a a category like "accessories" will sort by name, but the combo will show "by top rated"
(note that this is not a regression, but a different incarnation of the bug or a different bug that is very similar to this one).

As for the bit:
...
8 + sortmode=self.get_sort_mode(),
...
I am not sure that this is needed, the name of the function is certainly very misleading, my suggestion would
be to do something like (suggestions for better names welcome of course):
=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
--- softwarecenter/ui/gtk3/panes/availablepane.py 2012-03-30 09:46:20 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-10 09:37:54 +0000
@@ -618,7 +618,7 @@
             self.refresh_apps()

         query = self.get_query()
- n_matches = self.quick_query(query)
+ n_matches = self.quick_query_len(query)
         self.subcategories_view.set_subcategory(category, n_matches)

         self.action_bar.clear()

=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
--- softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-04 14:40:09 +0000
+++ softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-10 09:34:25 +0000
@@ -431,7 +431,10 @@
         self.show_appview_spinner()
         self._refresh_apps_with_apt_cache(query)

- def quick_query(self, query):
+ def quick_query_len(self, query):
+ """ do a blocking query that only returns the amount of
+ matches from this query
+ """
         # a blocking query and does not emit "query-complete"
         with ExecutionTime("enquirer.set_query() quick query"):
             self.enquirer.set_query(

To make it clearer what its actually doing. So unless I miss something, the sort order should not
influence the amount of items returned, so it should be ok to omit it.

Michael Vogt (mvo) wrote :

Actually I just noticed (or remembered) that the first bit I described above is probably very similar (or the same?) as bug #969215 so this part may not be relevant, if so, sorry for the noise.

Gary Lasker (gary-lasker) wrote :

Hi Michael, and thanks for your comments! Yes, I believe that the other case you describe is a symptom of the separate fix needed for bug 969215 (working on that now).

> As for the bit:
> ...
> 8 + sortmode=self.get_sort_mode(),

Indeed, that was just something that I tried quickly early on and then inadvertently left in the branch. This can go (and the method renamed for clarity is a good idea as well).

Gary Lasker (gary-lasker) wrote :

I've update my branch for the above. Thanks Michael!

2948. By Gary Lasker on 2012-04-10

updated per comments from mvo in the merge proposal, thanks Michael

Michael Vogt (mvo) wrote :

Thanks for this update. I'm happy to merge it, the only downside I see is that once the sort method
was changed it will always stay changed. I.e. never jump back to the "default" of the page. I wonder
if its maybe worthwhile to add this to the view_state we keep? I will merge the branch as it is and
investigate that other option a bit.

review: Approve
Gary Lasker (gary-lasker) wrote :

It actually does switch back to the default of the page when it is needed, afaict, such as when viewing the lists from the "More" buttons in the lobby view. I could not find a place where I didn't want my selected to be simply remembered. But, it's worth more investigation certaintly.

I find not resetting it to default constantly to be a big improvement, personally. :)

Michael Vogt (mvo) wrote :

On Tue, Apr 10, 2012 at 04:21:23PM -0000, Gary Lasker wrote:
> It actually does switch back to the default of the page when it is needed, afaict, such as when viewing the lists from the "More" buttons in the lobby view. I could not find a place where I didn't want my selected to be simply remembered. But, it's worth more investigation certaintly.
>
> I find not resetting it to default constantly to be a big improvement, personally. :)

Thanks, you are right :) I don't quite understand this in the code, but
the effect is really nice. When switching back and forth I get my sort
method preserved, but when switching to categories or similar I get
the default. Its a bit of a mystery why this is given that I can't see
any bit in the code that resets user_definied_sort_method once its
changed, but I'm too tired today to think about this more.

Thanks again!
 Michael

Gary Lasker (gary-lasker) wrote :

Indeed, the code needs some cleanup for sure, but at this late stage in the cycle I'm not sure we want to rework these pieces. The reason categories act differently is because the code path is separate and does not use user_defined_sort_method. That's part of what I think we want to fix, we actually do not want to even have to use this variable imo.

So, I think we just need to define clearly what we want sorting to do in each view (the spec probably says this already), and then do that with the simplest implementation possible. As a certain wise man I know has said on many an occasion, it's just a simple matter of programming!

:)

Michael Vogt (mvo) wrote :

Thanks! I agree with that.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
--- softwarecenter/ui/gtk3/panes/availablepane.py 2012-03-30 09:46:20 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-10 16:09:21 +0000
@@ -618,7 +618,7 @@
618 self.refresh_apps()618 self.refresh_apps()
619619
620 query = self.get_query()620 query = self.get_query()
621 n_matches = self.quick_query(query)621 n_matches = self.quick_query_len(query)
622 self.subcategories_view.set_subcategory(category, n_matches)622 self.subcategories_view.set_subcategory(category, n_matches)
623623
624 self.action_bar.clear()624 self.action_bar.clear()
625625
=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
--- softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-04 14:40:09 +0000
+++ softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-10 16:09:21 +0000
@@ -431,8 +431,10 @@
431 self.show_appview_spinner()431 self.show_appview_spinner()
432 self._refresh_apps_with_apt_cache(query)432 self._refresh_apps_with_apt_cache(query)
433433
434 def quick_query(self, query):434 def quick_query_len(self, query):
435 # a blocking query and does not emit "query-complete"435 """ do a blocking query that only returns the amount of
436 matches from this query
437 """
436 with ExecutionTime("enquirer.set_query() quick query"):438 with ExecutionTime("enquirer.set_query() quick query"):
437 self.enquirer.set_query(439 self.enquirer.set_query(
438 query,440 query,
439441
=== modified file 'softwarecenter/ui/gtk3/views/appview.py'
--- softwarecenter/ui/gtk3/views/appview.py 2012-04-10 05:05:22 +0000
+++ softwarecenter/ui/gtk3/views/appview.py 2012-04-10 16:09:21 +0000
@@ -210,7 +210,6 @@
210 if model:210 if model:
211 model.set_from_matches(matches)211 model.set_from_matches(matches)
212 self.set_model(model)212 self.set_model(model)
213 self.user_defined_sort_method = False
214213
215 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)214 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)
216 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)215 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)

Subscribers

People subscribed via source and target branches