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

Sorting order of offers appropriately by min/max range #4068

Merged
merged 1 commit into from Mar 18, 2020
Merged

Sorting order of offers appropriately by min/max range #4068

merged 1 commit into from Mar 18, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 16, 2020

Fixes #3818

This change fixes an issue with sorting the offer list when the amount is shown as a range. The sort was hard coded on the top end of the range which does not make sense when sorting in ascending order.

In OfferBookView::activate() we add a listener for the sortTypeProperty on amountColumn and volumeColumn. When the sortType is changed we set the comparator to be the approprate property of the Offer; either getAmount/getMinAmount; getVolume/getMinVolume.

@ghost ghost changed the title fix #3818 .. sorting range amounts appropriately by min/max Sorting order of offers appropriately by min/max range Mar 16, 2020
Comment on lines 297 to 309
if (newValue==TableColumn.SortType.DESCENDING)
amountColumn.setComparator(Comparator.comparing(o -> o.getOffer().getAmount(), Comparator.nullsFirst(Comparator.naturalOrder())));
else
amountColumn.setComparator(Comparator.comparing(o -> o.getOffer().getMinAmount(), Comparator.nullsFirst(Comparator.naturalOrder())));
});
volumeColumn.sortTypeProperty().addListener((observable, oldValue, newValue) -> {
if (newValue==TableColumn.SortType.DESCENDING)
volumeColumn.setComparator(Comparator.comparing(o -> o.getOffer().getVolume(), Comparator.nullsFirst(Comparator.naturalOrder())));
else
volumeColumn.setComparator(Comparator.comparing(o -> o.getOffer().getMinVolume(), Comparator.nullsFirst(Comparator.naturalOrder())));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

As we had issues in the past concerning using no brackets for one line conditionals we decided to use always {}

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK - based on my code format comments

ACK for functionality

@ghost ghost requested a review from ripcurlx March 17, 2020 21:02
ripcurlx
ripcurlx previously approved these changes Mar 18, 2020
@ripcurlx
Copy link
Contributor

@jmacxx I just recognized that you haven't signed your commits.

@ripcurlx
Copy link
Contributor

Also make sure your commits are signed, and that you applied Bisq's code style and formatting.

This change fixes an issue with sorting the offer list when the amount
is shown as a range.  In OfferBookView::activate() we add a listener
for the sortTypeProperty on amountColumn and volumeColumn. When the
sortType is changed we set the comparator to be the approprate property
of the Offer; either getAmount/getMinAmount; getVolume/getMinVolume.

Fixes #3818
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

Testet it on Mainnet.

@ripcurlx ripcurlx merged commit d9b5422 into bisq-network:master Mar 18, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 18, 2020

Awesome work, congrats on your first merged pull request!

@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 1, 2020
@ghost ghost mentioned this pull request Apr 7, 2020
@ghost ghost deleted the fix_column_sort_range branch May 16, 2020 13:00
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.

Sorting order of offers
1 participant