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
1=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
2--- softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-10 16:28:10 +0000
3+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-11 19:26:23 +0000
4@@ -531,6 +531,10 @@
5 """callback when the search entry widget changes"""
6 LOG.debug("on_search_terms_changed: %s" % new_text)
7
8+ # reset the flag in the app_view because each new search should
9+ # reset the sort criteria
10+ self.app_view.user_defined_search_sort_method = False
11+
12 self.state.search_term = new_text
13
14 # do not hide technical items for a custom list search
15
16=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
17--- softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-10 16:06:40 +0000
18+++ softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-11 19:26:23 +0000
19@@ -447,6 +447,9 @@
20 @wait_for_apt_cache_ready
21 def _refresh_apps_with_apt_cache(self, query):
22 LOG.debug("softwarepane query: %s" % query)
23+
24+ self.app_view.configure_sort_method(self._is_in_search_mode())
25+
26 # a nonblocking query calls on_query_complete once finished
27 with ExecutionTime("enquirer.set_query()"):
28 self.enquirer.set_query(
29
30=== modified file 'softwarecenter/ui/gtk3/views/appview.py'
31--- softwarecenter/ui/gtk3/views/appview.py 2012-04-10 16:17:22 +0000
32+++ softwarecenter/ui/gtk3/views/appview.py 2012-04-11 19:26:23 +0000
33@@ -94,7 +94,9 @@
34 self.appcount = None
35 self.vadj = 0.0
36
37+ # list view sorting stuff
38 self.user_defined_sort_method = False
39+ self.user_defined_search_sort_method = False
40 self._handler = self.sort_methods_combobox.connect(
41 "changed",
42 self.on_sort_method_changed)
43@@ -189,17 +191,6 @@
44 LOG.debug("display_matches called on AppTreeStore, ignoring")
45 return
46
47- sort_by_relevance = is_search and not self.user_defined_sort_method
48- if sort_by_relevance:
49- self._use_combobox_with_sort_by_search_ranking()
50- self.set_sort_method_with_no_signal(self._SORT_BY_SEARCH_RANKING)
51- else:
52- if not is_search:
53- self._use_combobox_without_sort_by_search_ranking()
54- if (self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING and \
55- not self.user_defined_sort_method):
56- self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
57-
58 model = self.get_model()
59 # disconnect the model from the view before running
60 # set_from_matches to ensure that the _cell_data_func_cb is not
61@@ -214,6 +205,24 @@
62 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)
63 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)
64
65+ def configure_sort_method(self, is_search=False):
66+ """ configures the sort method UI appropriately based on current
67+ conditions, including whether a search is in progress
68+ """
69+ if is_search and not self.user_defined_search_sort_method:
70+ # the first results for any new search should always be returned
71+ # sorted by relevance
72+ self._use_combobox_with_sort_by_search_ranking()
73+ self.set_sort_method_with_no_signal(
74+ self._SORT_BY_SEARCH_RANKING)
75+ self.user_defined_search_sort_method = True
76+ else:
77+ if not is_search:
78+ self._use_combobox_without_sort_by_search_ranking()
79+ if ((self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING) and
80+ not self.user_defined_sort_method):
81+ self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
82+
83 def clear_model(self):
84 return self.tree_view.clear_model()
85
86
87=== modified file 'test/gtk3/test_app_view.py'
88--- test/gtk3/test_app_view.py 2012-03-08 15:22:19 +0000
89+++ test/gtk3/test_app_view.py 2012-04-11 19:26:23 +0000
90@@ -71,6 +71,7 @@
91 # test normal window (no search)
92 matches = get_test_enquirer_matches(appview.helper.db)
93 appview.display_matches(matches, is_search=False)
94+ appview.configure_sort_method(is_search=False)
95 do_events()
96 in_model = []
97 for item in model:
98@@ -79,7 +80,9 @@
99
100 # now repeat and simulate a search
101 matches = get_test_enquirer_matches(appview.helper.db)
102+ appview.user_defined_search_sort_method = False
103 appview.display_matches(matches, is_search=True)
104+ appview.configure_sort_method(is_search=True)
105 do_events()
106 in_model = []
107 for item in model:
108@@ -88,7 +91,9 @@
109
110 # and back again to no search
111 matches = get_test_enquirer_matches(appview.helper.db)
112+ appview.user_defined_search_sort_method = False
113 appview.display_matches(matches, is_search=False)
114+ appview.configure_sort_method(is_search=False)
115 do_events()
116 # collect items in the model
117 in_model = []

Subscribers

People subscribed via source and target branches