Ubuntu Software Center

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

  1. appview-tweaks
  2. Merge into trunk
Proposed by Michael Vogt
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) Approve
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.
Revision history for this message
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
Revision history for this message
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.

lp:~mvo/software-center/appview-tweaks updated
2969. By Michael Vogt

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
=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
--- softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-11 18:47:43 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-04-13 11:33:23 +0000
@@ -533,7 +533,7 @@
533533
534 # reset the flag in the app_view because each new search should534 # reset the flag in the app_view because each new search should
535 # reset the sort criteria535 # reset the sort criteria
536 self.app_view.user_defined_search_sort_method = False536 self.app_view.reset_default_sort_mode()
537537
538 self.state.search_term = new_text538 self.state.search_term = new_text
539539
540540
=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
--- softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-11 14:38:36 +0000
+++ softwarecenter/ui/gtk3/panes/softwarepane.py 2012-04-13 11:33:23 +0000
@@ -486,11 +486,7 @@
486 if (self.state.category and486 if (self.state.category and
487 self.state.category.sortmode != SortMethods.BY_ALPHABET):487 self.state.category.sortmode != SortMethods.BY_ALPHABET):
488 return self.state.category.sortmode488 return self.state.category.sortmode
489 # searches are always by ranking unless the user decided differently489 # ask the app_view for the sort-mode
490 if (self._is_in_search_mode() and
491 not self.app_view.user_defined_sort_method):
492 return SortMethods.BY_SEARCH_RANKING
493 # use the appview combo
494 return self.app_view.get_sort_mode()490 return self.app_view.get_sort_mode()
495491
496 def on_search_entry_key_press_event(self, event):492 def on_search_entry_key_press_event(self, event):
497493
=== modified file 'softwarecenter/ui/gtk3/views/appview.py'
--- softwarecenter/ui/gtk3/views/appview.py 2012-04-11 18:50:42 +0000
+++ softwarecenter/ui/gtk3/views/appview.py 2012-04-13 11:33:23 +0000
@@ -95,8 +95,7 @@
95 self.vadj = 0.095 self.vadj = 0.0
9696
97 # list view sorting stuff97 # list view sorting stuff
98 self.user_defined_sort_method = False98 self._force_default_sort_method = True
99 self.user_defined_search_sort_method = False
100 self._handler = self.sort_methods_combobox.connect(99 self._handler = self.sort_methods_combobox.connect(
101 "changed",100 "changed",
102 self.on_sort_method_changed)101 self.on_sort_method_changed)
@@ -132,7 +131,6 @@
132 pass131 pass
133132
134 def on_sort_method_changed(self, *args):133 def on_sort_method_changed(self, *args):
135 self.user_defined_sort_method = True
136 self.vadj = 0.0134 self.vadj = 0.0
137 self.emit("sort-method-changed", self.sort_methods_combobox)135 self.emit("sort-method-changed", self.sort_methods_combobox)
138136
@@ -205,23 +203,37 @@
205 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)203 self.tree_view_scroll.get_vadjustment().set_lower(self.vadj)
206 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)204 self.tree_view_scroll.get_vadjustment().set_value(self.vadj)
207205
206 def reset_default_sort_mode(self):
207 """ force the appview to reset to the default sort method without
208 doing a refresh or sending any signals
209 """
210 self._force_default_sort_method = True
211
208 def configure_sort_method(self, is_search=False):212 def configure_sort_method(self, is_search=False):
209 """ configures the sort method UI appropriately based on current213 """ configures the sort method UI appropriately based on current
210 conditions, including whether a search is in progress214 conditions, including whether a search is in progress.
215
216 Note that this will not change the users current sort method,
217 if that is the intention, call reset_default_sort_mode()
211 """218 """
212 if is_search and not self.user_defined_search_sort_method:219 # figure out what combobox we need
213 # the first results for any new search should always be returned220 if is_search:
214 # sorted by relevance
215 self._use_combobox_with_sort_by_search_ranking()221 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:222 else:
220 if not is_search:223 self._use_combobox_without_sort_by_search_ranking()
221 self._use_combobox_without_sort_by_search_ranking()224
222 if ((self.get_sort_mode() == SortMethods.BY_SEARCH_RANKING) and225 # and what sorting
223 not self.user_defined_sort_method):226 if self._force_default_sort_method:
224 self.set_sort_method_with_no_signal(self._SORT_BY_TOP_RATED)227 # always reset this, its the job of the user of the appview
228 # to call reset_default_sort_mode() to reset this
229 self._force_default_sort_method = False
230 # and now set the default sort depending on if its a view or not
231 if is_search:
232 self.set_sort_method_with_no_signal(
233 self._SORT_BY_SEARCH_RANKING)
234 else:
235 self.set_sort_method_with_no_signal(
236 self._SORT_BY_TOP_RATED)
225237
226 def clear_model(self):238 def clear_model(self):
227 return self.tree_view.clear_model()239 return self.tree_view.clear_model()
228240
=== modified file 'test/gtk3/test_app_view.py'
--- test/gtk3/test_app_view.py 2012-04-11 19:23:32 +0000
+++ test/gtk3/test_app_view.py 2012-04-13 11:33:23 +0000
@@ -80,7 +80,6 @@
8080
81 # now repeat and simulate a search81 # now repeat and simulate a search
82 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
84 appview.display_matches(matches, is_search=True)83 appview.display_matches(matches, is_search=True)
85 appview.configure_sort_method(is_search=True)84 appview.configure_sort_method(is_search=True)
86 do_events()85 do_events()
@@ -91,7 +90,6 @@
9190
92 # and back again to no search91 # and back again to no search
93 matches = get_test_enquirer_matches(appview.helper.db)92 matches = get_test_enquirer_matches(appview.helper.db)
94 appview.user_defined_search_sort_method = False
95 appview.display_matches(matches, is_search=False)93 appview.display_matches(matches, is_search=False)
96 appview.configure_sort_method(is_search=False)94 appview.configure_sort_method(is_search=False)
97 do_events()95 do_events()

Subscribers

People subscribed via source and target branches