Merge lp:~mvo/software-center/appview-tweaks into lp:software-center

Proposed by Michael Vogt on 2012-04-12
Status: Merged
Merged at revision: 2969
Proposed branch: lp:~mvo/software-center/appview-tweaks
Merge into: lp:software-center
Diff against target: 121 lines (+29/-23)
4 files modified
softwarecenter/ui/gtk3/panes/availablepane.py (+1/-1)
softwarecenter/ui/gtk3/panes/softwarepane.py (+1/-5)
softwarecenter/ui/gtk3/views/appview.py (+27/-15)
test/gtk3/test_app_view.py (+0/-2)
To merge this branch: bzr merge lp:~mvo/software-center/appview-tweaks
Reviewer Review Type Date Requested Status
Gary Lasker (community) 2012-04-12 Approve on 2012-04-13
Review via email: mp+101727@code.launchpad.net

Description of the change

This branch is based on the very nice work in lp:~gary-lasker/software-center/sorting-fix-lp969215

The main reason for it is combine user_defined_search_sort_method and user_defined_sort_method
into the single single variable. Test test cases from #969215 and #966878 are working nicely for me
and the tests work as well.

But given how close we are to the end of the cycle and how delicate this code is, I would appreciate
a extra critical review of this to ensure we do not add regressions.

To post a comment you must log in.
Gary Lasker (gary-lasker) wrote :

Hi Michael. Thanks for this, I like it very much! I just have one small detail that I am not sure that we really want to do. It's the part of softwarecenter/ui/gtk3/panes/availablepane.py where we reset to the default search mode every time we change the category (the change at line 693). I think that it's preferable to just remove this piece, so that even when navigating between categories we retain the search criteria that the user has set.

I personally prefer this and I also think that it is in the spirit of the fix for bug 966878. Note the comment in the bug description "This does not occur when browsing catagories and it seems to remember the setting fine". We will be breaking that expectation if we do this reset.

Please let me know if you agree or not. Other than this one detail to be decided, I am all for merging this branch! I'll set it to approved in any case.

Thanks again, Michael!

review: Approve
Michael Vogt (mvo) wrote :

Thanks for your review. I agree, its indeed better to leave that out and not reset the sorting on category changes. I will revert that change.

2969. By Michael Vogt on 2012-04-13

softwarecenter/ui/gtk3/panes/availablepane.py: do not reseting the sort mode on category change, that is a bit too much, thanks to Gary Lasker for spotting this

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-11 18:47:43 +0000
3+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-13 11:33:23 +0000
4@@ -533,7 +533,7 @@
5
6 # reset the flag in the app_view because each new search should
7 # reset the sort criteria
8- self.app_view.user_defined_search_sort_method = False
9+ self.app_view.reset_default_sort_mode()
10
11 self.state.search_term = new_text
12
13
14=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
15--- softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-11 14:38:36 +0000
16+++ softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-13 11:33:23 +0000
17@@ -486,11 +486,7 @@
18 if (self.state.category and
19 self.state.category.sortmode != SortMethods.BY_ALPHABET):
20 return self.state.category.sortmode
21- # searches are always by ranking unless the user decided differently
22- if (self._is_in_search_mode() and
23- not self.app_view.user_defined_sort_method):
24- return SortMethods.BY_SEARCH_RANKING
25- # use the appview combo
26+ # ask the app_view for the sort-mode
27 return self.app_view.get_sort_mode()
28
29 def on_search_entry_key_press_event(self, event):
30
31=== modified file 'softwarecenter/ui/gtk3/views/appview.py'
32--- softwarecenter/ui/gtk3/views/appview.py 2012-04-11 18:50:42 +0000
33+++ softwarecenter/ui/gtk3/views/appview.py 2012-04-13 11:33:23 +0000
34@@ -95,8 +95,7 @@
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._force_default_sort_method = True
41 self._handler = self.sort_methods_combobox.connect(
42 "changed",
43 self.on_sort_method_changed)
44@@ -132,7 +131,6 @@
45 pass
46
47 def on_sort_method_changed(self, *args):
48- self.user_defined_sort_method = True
49 self.vadj = 0.0
50 self.emit("sort-method-changed", self.sort_methods_combobox)
51
52@@ -205,23 +203,37 @@
53 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)
54 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)
55
56+ def reset_default_sort_mode(self):
57+ """ force the appview to reset to the default sort method without
58+ doing a refresh or sending any signals
59+ """
60+ self._force_default_sort_method = True
61+
62 def configure_sort_method(self, is_search=False):
63 """ configures the sort method UI appropriately based on current
64- conditions, including whether a search is in progress
65+ conditions, including whether a search is in progress.
66+
67+ Note that this will not change the users current sort method,
68+ if that is the intention, call reset_default_sort_mode()
69 """
70- if is_search and not self.user_defined_search_sort_method:
71- # the first results for any new search should always be returned
72- # sorted by relevance
73+ # figure out what combobox we need
74+ if is_search:
75 self._use_combobox_with_sort_by_search_ranking()
76- self.set_sort_method_with_no_signal(
77- self._SORT_BY_SEARCH_RANKING)
78- self.user_defined_search_sort_method = True
79 else:
80- if not is_search:
81- self._use_combobox_without_sort_by_search_ranking()
82- if ((self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING) and
83- not self.user_defined_sort_method):
84- self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)
85+ self._use_combobox_without_sort_by_search_ranking()
86+
87+ # and what sorting
88+ if self._force_default_sort_method:
89+ # always reset this, its the job of the user of the appview
90+ # to call reset_default_sort_mode() to reset this
91+ self._force_default_sort_method = False
92+ # and now set the default sort depending on if its a view or not
93+ if is_search:
94+ self.set_sort_method_with_no_signal(
95+ self._SORT_BY_SEARCH_RANKING)
96+ else:
97+ self.set_sort_method_with_no_signal(
98+ self._SORT_BY_TOP_RATED)
99
100 def clear_model(self):
101 return self.tree_view.clear_model()
102
103=== modified file 'test/gtk3/test_app_view.py'
104--- test/gtk3/test_app_view.py 2012-04-11 19:23:32 +0000
105+++ test/gtk3/test_app_view.py 2012-04-13 11:33:23 +0000
106@@ -80,7 +80,6 @@
107
108 # now repeat and simulate a search
109 matches = get_test_enquirer_matches(appview.helper.db)
110- appview.user_defined_search_sort_method = False
111 appview.display_matches(matches, is_search=True)
112 appview.configure_sort_method(is_search=True)
113 do_events()
114@@ -91,7 +90,6 @@
115
116 # and back again to no search
117 matches = get_test_enquirer_matches(appview.helper.db)
118- appview.user_defined_search_sort_method = False
119 appview.display_matches(matches, is_search=False)
120 appview.configure_sort_method(is_search=False)
121 do_events()

Subscribers

People subscribed via source and target branches