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

Proposed by Gary Lasker on 2012-04-11
Status: Merged
Merged at revision: 2964
Proposed branch: lp:~gary-lasker/software-center/sorting-fix-lp969215
Merge into: lp:software-center
Diff against target: 117 lines (+32/-11)
4 files modified
softwarecenter/ui/gtk3/panes/availablepane.py (+4/-0)
softwarecenter/ui/gtk3/panes/softwarepane.py (+3/-0)
softwarecenter/ui/gtk3/views/appview.py (+20/-11)
test/gtk3/test_app_view.py (+5/-0)
To merge this branch: bzr merge lp:~gary-lasker/software-center/sorting-fix-lp969215
Reviewer Review Type Date Requested Status
Michael Vogt 2012-04-11 Approve on 2012-04-12
Review via email: mp+101633@code.launchpad.net

Description of the change

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).

To post a comment you must log in.
2964. By Gary Lasker on 2012-04-11

fix unit test test_app_view.py for new code

Michael Vogt (mvo) wrote :

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) wrote :

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) 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.

Thanks again!

Michael Vogt (mvo) wrote :

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

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-04-10 16:28:10 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-11 19:26:23 +0000
@@ -531,6 +531,10 @@
531 """callback when the search entry widget changes"""531 """callback when the search entry widget changes"""
532 LOG.debug("on_search_terms_changed: %s" % new_text)532 LOG.debug("on_search_terms_changed: %s" % new_text)
533533
534 # reset the flag in the app_view because each new search should
535 # reset the sort criteria
536 self.app_view.user_defined_search_sort_method = False
537
534 self.state.search_term = new_text538 self.state.search_term = new_text
535539
536 # do not hide technical items for a custom list search540 # do not hide technical items for a custom list search
537541
=== 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 @@
447 @wait_for_apt_cache_ready447 @wait_for_apt_cache_ready
448 def _refresh_apps_with_apt_cache(self, query):448 def _refresh_apps_with_apt_cache(self, query):
449 LOG.debug("softwarepane query: %s" % query)449 LOG.debug("softwarepane query: %s" % query)
450
451 self.app_view.configure_sort_method(self._is_in_search_mode())
452
450 # a nonblocking query calls on_query_complete once finished453 # a nonblocking query calls on_query_complete once finished
451 with ExecutionTime("enquirer.set_query()"):454 with ExecutionTime("enquirer.set_query()"):
452 self.enquirer.set_query(455 self.enquirer.set_query(
453456
=== 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 @@
94 self.appcount = None94 self.appcount = None
95 self.vadj = 0.095 self.vadj = 0.0
9696
97 # list view sorting stuff
97 self.user_defined_sort_method = False98 self.user_defined_sort_method = False
99 self.user_defined_search_sort_method = False
98 self._handler = self.sort_methods_combobox.connect(100 self._handler = self.sort_methods_combobox.connect(
99 "changed",101 "changed",
100 self.on_sort_method_changed)102 self.on_sort_method_changed)
@@ -189,17 +191,6 @@
189 LOG.debug("display_matches called on AppTreeStore, ignoring")191 LOG.debug("display_matches called on AppTreeStore, ignoring")
190 return192 return
191193
192 sort_by_relevance = is_search and not self.user_defined_sort_method
193 if sort_by_relevance:
194 self._use_combobox_with_sort_by_search_ranking()
195 self.set_sort_method_with_no_signal(self._SORT_BY_SEARCH_RANKING)
196 else:
197 if not is_search:
198 self._use_combobox_without_sort_by_search_ranking()
199 if (self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING and \
200 not self.user_defined_sort_method):
201 self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
202
203 model = self.get_model()194 model = self.get_model()
204 # disconnect the model from the view before running195 # disconnect the model from the view before running
205 # set_from_matches to ensure that the _cell_data_func_cb is not196 # set_from_matches to ensure that the _cell_data_func_cb is not
@@ -214,6 +205,24 @@
214 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)205 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)
215 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)206 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)
216207
208 def configure_sort_method(self, is_search=False):
209 """ configures the sort method UI appropriately based on current
210 conditions, including whether a search is in progress
211 """
212 if is_search and not self.user_defined_search_sort_method:
213 # the first results for any new search should always be returned
214 # sorted by relevance
215 self._use_combobox_with_sort_by_search_ranking()
216 self.set_sort_method_with_no_signal(
217 self._SORT_BY_SEARCH_RANKING)
218 self.user_defined_search_sort_method = True
219 else:
220 if not is_search:
221 self._use_combobox_without_sort_by_search_ranking()
222 if ((self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING) and
223 not self.user_defined_sort_method):
224 self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
225
217 def clear_model(self):226 def clear_model(self):
218 return self.tree_view.clear_model()227 return self.tree_view.clear_model()
219228
220229
=== 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 @@
71 # test normal window (no search)71 # test normal window (no search)
72 matches = get_test_enquirer_matches(appview.helper.db)72 matches = get_test_enquirer_matches(appview.helper.db)
73 appview.display_matches(matches, is_search=False)73 appview.display_matches(matches, is_search=False)
74 appview.configure_sort_method(is_search=False)
74 do_events()75 do_events()
75 in_model = []76 in_model = []
76 for item in model:77 for item in model:
@@ -79,7 +80,9 @@
7980
80 # now repeat and simulate a search81 # now repeat and simulate a search
81 matches = get_test_enquirer_matches(appview.helper.db)82 matches = get_test_enquirer_matches(appview.helper.db)
83 appview.user_defined_search_sort_method = False
82 appview.display_matches(matches, is_search=True)84 appview.display_matches(matches, is_search=True)
85 appview.configure_sort_method(is_search=True)
83 do_events()86 do_events()
84 in_model = []87 in_model = []
85 for item in model:88 for item in model:
@@ -88,7 +91,9 @@
8891
89 # and back again to no search92 # and back again to no search
90 matches = get_test_enquirer_matches(appview.helper.db)93 matches = get_test_enquirer_matches(appview.helper.db)
94 appview.user_defined_search_sort_method = False
91 appview.display_matches(matches, is_search=False)95 appview.display_matches(matches, is_search=False)
96 appview.configure_sort_method(is_search=False)
92 do_events()97 do_events()
93 # collect items in the model98 # collect items in the model
94 in_model = []99 in_model = []

Subscribers

People subscribed via source and target branches