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.

Sign up for GitHub

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

Jump to bottom

Resolve bug with sorting of table columns with mixed value types #1278

Merged
merged 1 commit into from
May 30, 2024
Merged

Resolve bug with sorting of table columns with mixed value types #1278

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions activity_browser/ui/tables/models/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,12 +414,24 @@ def setup_model_data(self) -> None:
def sync(self, *args, **kwargs) -> None:
pass


class ABSortProxyModel(QSortFilterProxyModel):
"""Reimplementation to allow for sorting on the actual data in cells instead of the visible data.

See this for context: https://github.com/LCA-ActivityBrowser/activity-browser/pull/1151
"""
def lessThan(self, left, right):
def lessThan(self, left, right) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about importing QModelIndex for the left and right arguments so that we can have better type hinting support?

Try this:

from PySide2.QtCore import QModelIndex```


Then update the function signature to this:

def lessThan(self, left: QModelIndex, right: QModelIndex) -> bool:```

If we try to be consistent with type hinting, then we will be able to detect errors by using linters like mypy.

"""Override to sort actual data, expects `left` and `right` are comparable.

If `left` and `right` are not the same type, we check if numerical and empty string are compared, if that is the
case, we assume empty string == 0.
Added this case for: https://github.com/LCA-ActivityBrowser/activity-browser/issues/1215"""
left_data = self.sourceModel().data(left, 'sorting')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have an inconsistent usage of single quotes versus double quotes. This is not a technical issue but consistency across the codebase will allow new people to easily come in and understand what's going on.

right_data = self.sourceModel().data(right, 'sorting')
return left_data < right_data
if type(left_data) is type(right_data):
return left_data < right_data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to use isinstance() instead of type(). I believe type() is faster, but isinstance looks cleaner to me.

This could be rewritten as:
elif type(left_data) in (int, float) and isinstance(right_data, str) and not right_data

If right_data is a string, not right_data would be falsy. We can do the same on line 435.

This would just make the code look a bit cleaner in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To go a step further, I think it would be cleaner to try left_data < right_data directly. They don't need to have the same type to compare the two objects.

And isinstance is exactly the right answer here.

elif type(left_data) in (int, float) and type(right_data) is str and right_data == "":
return left_data < 0
elif type(right_data) in (int, float) and type(left_data) is str and left_data == "":
return 0 < right_data
raise ValueError(f"Cannot compare {left_data} and {right_data}, incompatible types.")
Loading