From 3005374ada236d8b126e2194881663401edb654c Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Wed, 8 Feb 2017 17:21:27 -0500 Subject: [PATCH] Simplify sorting logic in sortfilter. For URL completion, time-based sorting is handled by the SQL model. All the other models use simple alphabetical sorting. This allowed cleaning up some logic in the sortfilter, removing DUMB_SORT, and removing the completion.Role.sort. This also removes the userdata completion field as it was only used in url completion and is no longer necessary with the SQL model. --- qutebrowser/completion/models/base.py | 26 ++-------- qutebrowser/completion/models/configmodel.py | 4 +- qutebrowser/completion/models/miscmodels.py | 1 - qutebrowser/completion/models/sortfilter.py | 28 ++-------- tests/unit/completion/test_completer.py | 1 - tests/unit/completion/test_sortfilter.py | 54 ++++---------------- 6 files changed, 19 insertions(+), 95 deletions(-) diff --git a/qutebrowser/completion/models/base.py b/qutebrowser/completion/models/base.py index 43f3a1b48..1d3ed2be4 100644 --- a/qutebrowser/completion/models/base.py +++ b/qutebrowser/completion/models/base.py @@ -29,10 +29,6 @@ from PyQt5.QtGui import QStandardItemModel, QStandardItem from qutebrowser.utils import usertypes -Role = usertypes.enum('Role', ['sort', 'userdata'], start=Qt.UserRole, - is_int=True) - - class CompletionModel(QStandardItemModel): """A simple QStandardItemModel adopted for completions. @@ -42,37 +38,30 @@ class CompletionModel(QStandardItemModel): Attributes: column_widths: The width percentages of the columns used in the - completion view. - dumb_sort: the dumb sorting used by the model """ - def __init__(self, dumb_sort=None, column_widths=(30, 70, 0), - columns_to_filter=None, delete_cur_item=None, parent=None): + def __init__(self, column_widths=(30, 70, 0), columns_to_filter=None, + delete_cur_item=None, parent=None): super().__init__(parent) self.setColumnCount(3) self.columns_to_filter = columns_to_filter or [0] - self.dumb_sort = dumb_sort self.column_widths = column_widths self.delete_cur_item = delete_cur_item - def new_category(self, name, sort=None): + def new_category(self, name): """Add a new category to the model. Args: name: The name of the category to add. - sort: The value to use for the sort role. Return: The created QStandardItem. """ cat = QStandardItem(name) - if sort is not None: - cat.setData(sort, Role.sort) self.appendRow(cat) return cat - def new_item(self, cat, name, desc='', misc=None, sort=None, - userdata=None): + def new_item(self, cat, name, desc='', misc=None): """Add a new item to a category. Args: @@ -80,8 +69,6 @@ class CompletionModel(QStandardItemModel): name: The name of the item. desc: The description of the item. misc: Misc text to display. - sort: Data for the sort role (int). - userdata: User data to be added for the first column. Return: A (nameitem, descitem, miscitem) tuple. @@ -98,11 +85,6 @@ class CompletionModel(QStandardItemModel): miscitem = QStandardItem(misc) cat.appendRow([nameitem, descitem, miscitem]) - if sort is not None: - nameitem.setData(sort, Role.sort) - if userdata is not None: - nameitem.setData(userdata, Role.userdata) - return nameitem, descitem, miscitem def flags(self, index): """Return the item flags for index. diff --git a/qutebrowser/completion/models/configmodel.py b/qutebrowser/completion/models/configmodel.py index 5ed2a47d0..406d8d572 100644 --- a/qutebrowser/completion/models/configmodel.py +++ b/qutebrowser/completion/models/configmodel.py @@ -67,7 +67,7 @@ def value(sectname, optname): optname: The name of the config option this model shows. """ model = base.CompletionModel(column_widths=(20, 70, 10)) - cur_cat = model.new_category("Current/Default", sort=0) + cur_cat = model.new_category("Current/Default") try: val = config.get(sectname, optname, raw=True) or '""' except (configexc.NoSectionError, configexc.NoOptionError): @@ -85,7 +85,7 @@ def value(sectname, optname): # Different type for each value (KeyValue) vals = configdata.DATA[sectname][optname].typ.complete() if vals is not None: - cat = model.new_category("Completions", sort=1) + cat = model.new_category("Completions") for (val, desc) in vals: model.new_item(cat, val, desc) return model diff --git a/qutebrowser/completion/models/miscmodels.py b/qutebrowser/completion/models/miscmodels.py index bcbb94177..cb87f9d7e 100644 --- a/qutebrowser/completion/models/miscmodels.py +++ b/qutebrowser/completion/models/miscmodels.py @@ -119,7 +119,6 @@ def buffer(): model = base.CompletionModel( column_widths=(6, 40, 54), - dumb_sort=Qt.DescendingOrder, delete_cur_item=delete_buffer, columns_to_filter=[idx_column, url_column, text_column]) diff --git a/qutebrowser/completion/models/sortfilter.py b/qutebrowser/completion/models/sortfilter.py index 92dd1b2a0..e5d42607c 100644 --- a/qutebrowser/completion/models/sortfilter.py +++ b/qutebrowser/completion/models/sortfilter.py @@ -33,14 +33,13 @@ from qutebrowser.completion.models import base as completion class CompletionFilterModel(QSortFilterProxyModel): - """Subclass of QSortFilterProxyModel with custom sorting/filtering. + """Subclass of QSortFilterProxyModel with custom filtering. Attributes: pattern: The pattern to filter with. srcmodel: The current source model. Kept as attribute because calling `sourceModel` takes quite a long time for some reason. - _sort_order: The order to use for sorting if using dumb_sort. """ def __init__(self, source, parent=None): @@ -49,21 +48,12 @@ class CompletionFilterModel(QSortFilterProxyModel): self.srcmodel = source self.pattern = '' self.pattern_re = None - - dumb_sort = self.srcmodel.dumb_sort - if dumb_sort is None: - # pylint: disable=invalid-name - self.lessThan = self.intelligentLessThan - self._sort_order = Qt.AscendingOrder - else: - self.setSortRole(completion.Role.sort) - self._sort_order = dumb_sort + self.lessThan = self.intelligentLessThan + #self._sort_order = self.srcmodel.sort_order or Qt.AscendingOrder def set_pattern(self, val): """Setter for pattern. - Invalidates the filter and re-sorts the model. - Args: val: The value to set. """ @@ -163,12 +153,6 @@ class CompletionFilterModel(QSortFilterProxyModel): qtutils.ensure_valid(lindex) qtutils.ensure_valid(rindex) - left_sort = self.srcmodel.data(lindex, role=completion.Role.sort) - right_sort = self.srcmodel.data(rindex, role=completion.Role.sort) - - if left_sort is not None and right_sort is not None: - return left_sort < right_sort - left = self.srcmodel.data(lindex) right = self.srcmodel.data(rindex) @@ -183,9 +167,3 @@ class CompletionFilterModel(QSortFilterProxyModel): return False else: return left < right - - def sort(self, column, order=None): - """Extend sort to respect self._sort_order if no order was given.""" - if order is None: - order = self._sort_order - super().sort(column, order) diff --git a/tests/unit/completion/test_completer.py b/tests/unit/completion/test_completer.py index 48f619104..34c9a2d19 100644 --- a/tests/unit/completion/test_completer.py +++ b/tests/unit/completion/test_completer.py @@ -37,7 +37,6 @@ class FakeCompletionModel(QStandardItemModel): super().__init__(parent) self.kind = kind self.pos_args = [*pos_args] - self.dumb_sort = None class CompletionWidgetStub(QObject): diff --git a/tests/unit/completion/test_sortfilter.py b/tests/unit/completion/test_sortfilter.py index c972a5ec9..7d84e9489 100644 --- a/tests/unit/completion/test_sortfilter.py +++ b/tests/unit/completion/test_sortfilter.py @@ -151,80 +151,46 @@ def test_count(tree, expected): assert filter_model.count() == expected -@pytest.mark.parametrize('pattern, dumb_sort, filter_cols, before, after', [ - ('foo', None, [0], +@pytest.mark.parametrize('pattern, filter_cols, before, after', [ + ('foo', [0], [[('foo', '', ''), ('bar', '', '')]], [[('foo', '', '')]]), - ('foo', None, [0], + ('foo', [0], [[('foob', '', ''), ('fooc', '', ''), ('fooa', '', '')]], [[('fooa', '', ''), ('foob', '', ''), ('fooc', '', '')]]), - ('foo', None, [0], + ('foo', [0], [[('foo', '', '')], [('bar', '', '')]], [[('foo', '', '')], []]), # prefer foobar as it starts with the pattern - ('foo', None, [0], + ('foo', [0], [[('barfoo', '', ''), ('foobar', '', '')]], [[('foobar', '', ''), ('barfoo', '', '')]]), # however, don't rearrange categories - ('foo', None, [0], + ('foo', [0], [[('barfoo', '', '')], [('foobar', '', '')]], [[('barfoo', '', '')], [('foobar', '', '')]]), - ('foo', None, [1], + ('foo', [1], [[('foo', 'bar', ''), ('bar', 'foo', '')]], [[('bar', 'foo', '')]]), - ('foo', None, [0, 1], + ('foo', [0, 1], [[('foo', 'bar', ''), ('bar', 'foo', ''), ('bar', 'bar', '')]], [[('foo', 'bar', ''), ('bar', 'foo', '')]]), - ('foo', None, [0, 1, 2], + ('foo', [0, 1, 2], [[('foo', '', ''), ('bar', '')]], [[('foo', '', '')]]), - - # the fourth column is the sort role, which overrides data-based sorting - ('', None, [0], - [[('two', '', '', 2), ('one', '', '', 1), ('three', '', '', 3)]], - [[('one', '', ''), ('two', '', ''), ('three', '', '')]]), - - ('', Qt.AscendingOrder, [0], - [[('two', '', '', 2), ('one', '', '', 1), ('three', '', '', 3)]], - [[('one', '', ''), ('two', '', ''), ('three', '', '')]]), - - ('', Qt.DescendingOrder, [0], - [[('two', '', '', 2), ('one', '', '', 1), ('three', '', '', 3)]], - [[('three', '', ''), ('two', '', ''), ('one', '', '')]]), ]) -def test_set_pattern(pattern, dumb_sort, filter_cols, before, after): +def test_set_pattern(pattern, filter_cols, before, after): """Validate the filtering and sorting results of set_pattern.""" model = _create_model(before) - model.dumb_sort = dumb_sort model.columns_to_filter = filter_cols filter_model = sortfilter.CompletionFilterModel(model) filter_model.set_pattern(pattern) actual = _extract_model_data(filter_model) assert actual == after - - -def test_sort(): - """Ensure that a sort argument passed to sort overrides DUMB_SORT. - - While test_set_pattern above covers most of the sorting logic, this - particular case is easier to test separately. - """ - model = _create_model([[('B', '', '', 1), - ('C', '', '', 2), - ('A', '', '', 0)]]) - filter_model = sortfilter.CompletionFilterModel(model) - - filter_model.sort(0, Qt.AscendingOrder) - actual = _extract_model_data(filter_model) - assert actual == [[('A', '', ''), ('B', '', ''), ('C', '', '')]] - - filter_model.sort(0, Qt.DescendingOrder) - actual = _extract_model_data(filter_model) - assert actual == [[('C', '', ''), ('B', '', ''), ('A', '', '')]]