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 invalid comparators #3251

Merged
merged 2 commits into from Nov 1, 2019

Conversation

@stejbac
Copy link
Contributor

stejbac commented Sep 13, 2019

Many of the table column comparators (as well as PaymentMethod.compareTo) are invalid, since they immediately return 0 upon encountering a null/missing field. This violates the Comparator/Comparable contract, which expects a total order on the objects and, in particular, an equivalence relation given by the result of "compare(x, y) == 0".

This PR attemps to fix all the invalid Comparator / Comparable instances (which are almost all table column comparators), by ordering objects with a null/missing (sub)field ahead of the populated ones. In most cases this will probably have no effect, as nulls won't be encountered for that column in practice. However, the current flawed comparators are at least breaking row sorting in the Portfolio/History table, as seen in the following two screenshots:

Closed Trades with Broken Comparators 1
Closed Trades with Broken Comparators 2

The ascending/descending sorting of the "Amount in BTC" field does not make sense, due to a broken comparator passed to the underlying FX library insertion sort. After the fix, the rows appear as follows:

Closed Trades with Fixed Comparators 1
Closed Trades with Fixed Comparators 2

The missing cells are deemed smaller than all the others in the column, which is consistent with TableColumnBase.DEFAULT_COMPARATOR from the openjfx library.

@stejbac stejbac requested review from ripcurlx and sqrrm as code owners Sep 13, 2019
@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Sep 17, 2019

@stejbac Could you please resolve the conflict because of the recently merged release branch.

stejbac added 2 commits Sep 12, 2019
Don't return 0 in the case of a null 'this.id' field, as that violates
the Comparable contract and leads to inconsistent NPE throwing when the
arguments are swapped. Instead, just assume (reasonably) that the ID
field will never be null.
Remove the flawed pattern of returning 0 in a comparator when the sub-
field of one of the two inputs being compared is null or absent. This
violates the Comparator contract, since returning 0 or otherwise should
define an equivalence relation.

Use Comparator.nullsFirst(..) in the table column comparators instead,
to ensure consistent ordering when a cell is missing a value. This fixes
ill-defined and erratic behaviour in the underlying merge/insertion sort
of the table rows done by the FX library.
@stejbac stejbac force-pushed the stejbac:fix-invalid-comparators branch from 2809270 to a392081 Sep 17, 2019
@stejbac

This comment has been minimized.

Copy link
Contributor Author

stejbac commented Sep 17, 2019

I've just rebased the two commits to resolve the conflict, which appears to be simply due to a file rename: TraderDisputeView -> DisputeView.

Copy link
Member

freimair left a comment

Ack

@freimair freimair merged commit 8ee2d53 into bisq-network:master Nov 1, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@freimair freimair referenced this pull request Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.