Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix completion sorting #3237

Merged
merged 2 commits into from Nov 3, 2017
Merged

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Nov 3, 2017

There's also a fix for a histcategory test that was failing locally while working on this -- turns out it was timezone sensitive 😬


This change is Reviewable

I previously removed the sorting logic from SortFilter thinking it was
unnecessary if we construct the model with a sorted list. However, this
only worked when no pattern was set, and the items are misordered as
soon as a pattern is input.

This patch reintroduces alpha-sorting, which can be disabled by passing
sort=False to the ListCategory constructor. The session completion test
had to be tweaked as it simulated the incorrect assumption that the
session list is not alpha-ordered; sessions come out of the
session-manager pre-sorted so we may as well use alpha-sorting in the
session completion model.

Resolves qutebrowser#3156.
We really just need to check that the row exists here, the date doesn't
matter. Checking the date here is actually flaky with regards to time.
When running locally at 11:50 EST, it failed with:

```
assert self._model.data(self._model.index(row, col)) == item
AssertionError: assert '1969-12-31' == '1970-01-01'
- 1969-12-31
+ 1970-01-01
```

It was wrong to assume that an atime of 0 would always format to
1970-01-01.
@The-Compiler
Copy link
Member

Welp, I broke something in master yesterday evening 😉

@The-Compiler
Copy link
Member

Thanks!

@The-Compiler The-Compiler merged commit 0f28804 into qutebrowser:master Nov 3, 2017
@rcorre rcorre deleted the completionsort branch November 4, 2017 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants