Sort tab completion properly with more than 9 tabs.

[toofar]

Don't alphabetically sort tab completion.

ListCategory was changed to sort completions (on the first column) by default in #3237, most callers that didn't want to be sorted were addressed but the tab completion missed out. This change address that and adds a test to ensure the completion stays in insertion order.

The test tests the case of where you have 11 tabs and if the model was sorted the tabs with index 10 and 11 would be sorted before the one with index 2.

The random.choices bit for the tab url and title is to also make sure the model isn't being sorted on those columns, whithout haveng to write and all ten lines.

---

This change is 

[qutebrowser-bot]

Thanks for the pull request! Note that reviews currently can take a bit longer, as @The-Compiler is busy with exams until September.

(Powered by GitMate.io)

[rcorre]

Looks good, but please add a test in test_models.py to validate the sorting order.

An alternate approach that comes to mind is changing ListCategory._sort from a bool to a lambda, so a custom sorting function could be provided:

diff --git a/qutebrowser/completion/models/listcategory.py b/qutebrowser/completion/models/listcategory.py
index 13bc1e6b2..7db659886 100644
--- a/qutebrowser/completion/models/listcategory.py
+++ b/qutebrowser/completion/models/listcategory.py
@@ -88,6 +88,6 @@ class ListCategory(QSortFilterProxyModel):
         elif rightstart and not leftstart:
             return False
         elif self._sort:
-            return left < right
+            return self._sort(left < right)
         else:
             return False

That might be something worth considering, as it should allow you to solve the issue with > 10 windows.

That being said, I wouldn't worry too much about sorting windows, as there's no clear order to them like there is with tabs.

[rcorre]

nit: I don't think if num_tabs else 0 is necessary, since the following loop won't run if num_tabs == 0

[jgkamat]

This is needed because log10(0) will throw an error, but I agree it's a little weird...

[The-Compiler]

Could just do len(str(num_tabs)) instead?

[jgkamat]

LGTM overall, could you add a small test for the sorting though if possible (maybe one could go in completion.feature)

[The-Compiler]

Wait, I have a deja-vu... didn't @rcorre already fix this (#2883) with #2888?

[The-Compiler]

Yep, this works on commit b7a296c, and then broke again on 47447c0 for #3156.

[The-Compiler]

Seems like all we really need is this and a test:

diff --git a/qutebrowser/completion/models/miscmodels.py b/qutebrowser/completion/models/miscmodels.py
index 35beb24de..bf8de194b 100644
--- a/qutebrowser/completion/models/miscmodels.py
+++ b/qutebrowser/completion/models/miscmodels.py
@@ -123,7 +123,7 @@ def _buffer(skip_win_id=None):
                          tab.url().toDisplayString(),
                          tabbed_browser.widget.page_title(idx)))
         cat = listcategory.ListCategory("{}".format(win_id), tabs,
-                                        delete_func=delete_buffer)
+                                        delete_func=delete_buffer, sort=False)
         model.add_category(cat)
 
     return model

[The-Compiler]

also, whoever is committing this, while you're at it: Change that "{}".format(win_id) to a str(win_id) 😉

[jgkamat]

LGTM :)

[The-Compiler]

Thanks!