Merge lp:~gary-lasker/software-center/sorting-fix-lp969215 into lp:software-center

[Gary Lasker]

This fixes bug 969215 and to the best of my testing I get what I expect with sorting options in all cases (searching, not searching, navigating around, etc.). Note in the bug description that I've added a second test case that comes from a comment in a separate merge proposal review by mvo (https://code.launchpad.net/~gary-lasker/software-center/remember-sort-preference-lp966878/+merge/101306).

[lp:~gary-lasker/software-center/sorting-fix-lp969215]

2964. By Gary Lasker on 2012-04-11

fix unit test test_app_view.py for new code

[Michael Vogt (mvo)]

Thanks for your work on this branch! It works fine and fixes the bug nicely.

Looking at the code (and as I understand it, I may be wrong of course) I would
like to suggest a tweak to the variable name "self.app_view.user_defined_search_sort_method".

- its actually not set when the sort method is changed, but unconditionally once the first
  set of results was displayed. Maybe something like:
- it very similar to "self.user_defined_sort_method" that is set when the sort-method changes

        # when a search changes, this is reset and forces the first
        # result set to be always sorted by relevance
        self.force_default_search_sort_method = True

(of course the logic needs to be inverted with that name change)?

But a more creative ideas for a good name is very welcome of course :) I will approve it now, but
I would like to hear your opinion about the variable.

review: Approve

[Michael Vogt (mvo)]

Ups, looks like there was a copy/paste error above, it should read:

Maybe something like:
        # when a search changes, this is reset and forces the first
        # result set to be always sorted by relevance
        self.force_default_search_sort_method = True
?

[Gary Lasker (gary-lasker)]

Hi mvo! Thanks for the review. You make a very good point about that variable name, thanks. It really does not reflect its purpose clearly at all. I'm happy with self.force_default_search_sort_method, but, hmm, when I see "force" it makes me think we are pushing something that shouldn't be pushed. Plus, it sounds like a method, and not a flag. How about "self.default_search_sort_method_needs_reset"? I'll push a branch with that and if it seems fine to you we can merge it.

Thanks again!

[Michael Vogt (mvo)]

On Thu, Apr 12, 2012 at 05:13:26PM -0000, Gary Lasker wrote:

> Hi mvo! Thanks for the review. You make a very good point about that
> variable name, thanks. It really does not reflect its purpose
> clearly at all. I'm happy with
> self.force_default_search_sort_method, but, hmm, when I see "force"
> it makes me think we are pushing something that shouldn't be
> pushed. Plus, it sounds like a method, and not a flag. How about
> "self.default_search_sort_method_needs_reset"? I'll push a branch
> with that and if it seems fine to you we can merge it.

Sure, good point! I don't mind much what name to use, I used a similar
one in the lp:~mvo/software-center/appview-tweaks branch, but I will
be happy to change it to a different name and/or a different method
name.

Cheers,
 Michael

=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
--- softwarecenter/ui/gtk3/panes/availablepane.py	2012-04-10 16:28:10 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py	2012-04-11 19:26:23 +0000
@@ -531,6 +531,10 @@
         """callback when the search entry widget changes"""
         LOG.debug("on_search_terms_changed: %s" % new_text)
 
+        # reset the flag in the app_view because each new search should
+        # reset the sort criteria
+        self.app_view.user_defined_search_sort_method = False
+
         self.state.search_term = new_text
 
         # do not hide technical items for a custom list search
 
=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
--- softwarecenter/ui/gtk3/panes/softwarepane.py	2012-04-10 16:06:40 +0000
+++ softwarecenter/ui/gtk3/panes/softwarepane.py	2012-04-11 19:26:23 +0000
@@ -447,6 +447,9 @@
     @wait_for_apt_cache_ready
     def _refresh_apps_with_apt_cache(self, query):
         LOG.debug("softwarepane query: %s" % query)
+
+        self.app_view.configure_sort_method(self._is_in_search_mode())
+
         # a nonblocking query calls on_query_complete once finished
         with ExecutionTime("enquirer.set_query()"):
             self.enquirer.set_query(
 
=== modified file 'softwarecenter/ui/gtk3/views/appview.py'
--- softwarecenter/ui/gtk3/views/appview.py	2012-04-10 16:17:22 +0000
+++ softwarecenter/ui/gtk3/views/appview.py	2012-04-11 19:26:23 +0000
@@ -94,7 +94,9 @@
         self.appcount = None
         self.vadj = 0.0
 
+        # list view sorting stuff
         self.user_defined_sort_method = False
+        self.user_defined_search_sort_method = False
         self._handler = self.sort_methods_combobox.connect(
                                     "changed",
                                     self.on_sort_method_changed)
@@ -189,17 +191,6 @@
             LOG.debug("display_matches called on AppTreeStore, ignoring")
             return
 
-        sort_by_relevance = is_search and not self.user_defined_sort_method
-        if sort_by_relevance:
-            self._use_combobox_with_sort_by_search_ranking()
-            self.set_sort_method_with_no_signal(self._SORT_BY_SEARCH_RANKING)
-        else:
-            if not is_search:
-                self._use_combobox_without_sort_by_search_ranking()
-            if (self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING and \
-                not self.user_defined_sort_method):
-                self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
-
         model = self.get_model()
         # disconnect the model from the view before running
         # set_from_matches to ensure that the _cell_data_func_cb is not
@@ -214,6 +205,24 @@
         self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)
         self.tree_view_scroll.get_vadjustment().set_value(self.vadj)
 
+    def configure_sort_method(self, is_search=False):
+        """ configures the sort method UI appropriately based on current
+            conditions, including whether a search is in progress
+        """
+        if is_search and not self.user_defined_search_sort_method:
+            # the first results for any new search should always be returned
+            # sorted by relevance
+            self._use_combobox_with_sort_by_search_ranking()
+            self.set_sort_method_with_no_signal(
+                                        self._SORT_BY_SEARCH_RANKING)
+            self.user_defined_search_sort_method = True
+        else:
+            if not is_search:
+                self._use_combobox_without_sort_by_search_ranking()
+            if ((self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING) and
+                not self.user_defined_sort_method):
+                self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
+
     def clear_model(self):
         return self.tree_view.clear_model()
 
 
=== modified file 'test/gtk3/test_app_view.py'
--- test/gtk3/test_app_view.py	2012-03-08 15:22:19 +0000
+++ test/gtk3/test_app_view.py	2012-04-11 19:26:23 +0000
@@ -71,6 +71,7 @@
         # test normal window (no search)
         matches = get_test_enquirer_matches(appview.helper.db)
         appview.display_matches(matches, is_search=False)
+        appview.configure_sort_method(is_search=False)
         do_events()
         in_model = []
         for item in model:
@@ -79,7 +80,9 @@
 
         # now repeat and simulate a search
         matches = get_test_enquirer_matches(appview.helper.db)
+        appview.user_defined_search_sort_method = False
         appview.display_matches(matches, is_search=True)
+        appview.configure_sort_method(is_search=True)
         do_events()
         in_model = []
         for item in model:
@@ -88,7 +91,9 @@
 
         # and back again to no search
         matches = get_test_enquirer_matches(appview.helper.db)
+        appview.user_defined_search_sort_method = False
         appview.display_matches(matches, is_search=False)
+        appview.configure_sort_method(is_search=False)
         do_events()
         # collect items in the model
         in_model = []