Code review comment for lp:~gary-lasker/software-center/remember-sort-preference-lp966878

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.

« Back to merge proposal