From a392081462995bc02cca75c9bafaa8674254a92c Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 12 Sep 2019 23:38:36 +0100 Subject: [PATCH] Fix invalid neutral element handling in View comparators 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. --- .../desktop/main/funds/locked/LockedView.java | 8 +- .../main/funds/reserved/ReservedView.java | 8 +- .../main/offer/offerbook/OfferBookView.java | 23 +--- .../closedtrades/ClosedTradesView.java | 122 +++++------------- .../failedtrades/FailedTradesView.java | 15 +-- .../portfolio/openoffer/OpenOffersView.java | 14 +- .../pendingtrades/PendingTradesView.java | 30 ++--- .../main/support/dispute/DisputeView.java | 2 +- 8 files changed, 60 insertions(+), 162 deletions(-) diff --git a/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedView.java b/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedView.java index 74b87ea4d14..75cb17d1ef7 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedView.java @@ -64,6 +64,7 @@ import javafx.util.Callback; import java.util.Comparator; +import java.util.Date; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -123,12 +124,7 @@ public void initialize() { addressColumn.setComparator(Comparator.comparing(LockedListItem::getAddressString)); detailsColumn.setComparator(Comparator.comparing(o -> o.getTrade().getId())); balanceColumn.setComparator(Comparator.comparing(LockedListItem::getBalance)); - dateColumn.setComparator((o1, o2) -> { - if (getTradable(o1).isPresent() && getTradable(o2).isPresent()) - return getTradable(o2).get().getDate().compareTo(getTradable(o1).get().getDate()); - else - return 0; - }); + dateColumn.setComparator(Comparator.comparing(o -> getTradable(o).map(Tradable::getDate).orElse(new Date(0)))); tableView.getSortOrder().add(dateColumn); dateColumn.setSortType(TableColumn.SortType.DESCENDING); diff --git a/desktop/src/main/java/bisq/desktop/main/funds/reserved/ReservedView.java b/desktop/src/main/java/bisq/desktop/main/funds/reserved/ReservedView.java index 59e782bc167..6285c9ecb87 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/reserved/ReservedView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/reserved/ReservedView.java @@ -64,6 +64,7 @@ import javafx.util.Callback; import java.util.Comparator; +import java.util.Date; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -123,12 +124,7 @@ public void initialize() { addressColumn.setComparator(Comparator.comparing(ReservedListItem::getAddressString)); detailsColumn.setComparator(Comparator.comparing(o -> o.getOpenOffer().getId())); balanceColumn.setComparator(Comparator.comparing(ReservedListItem::getBalance)); - dateColumn.setComparator((o1, o2) -> { - if (getTradable(o1).isPresent() && getTradable(o2).isPresent()) - return getTradable(o2).get().getDate().compareTo(getTradable(o1).get().getDate()); - else - return 0; - }); + dateColumn.setComparator(Comparator.comparing(o -> getTradable(o).map(Tradable::getDate).orElse(new Date(0)))); tableView.getSortOrder().add(dateColumn); dateColumn.setSortType(TableColumn.SortType.DESCENDING); diff --git a/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java b/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java index 26eb7e603b7..ae01ce70260 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java @@ -48,8 +48,6 @@ import bisq.core.locale.FiatCurrency; import bisq.core.locale.Res; import bisq.core.locale.TradeCurrency; -import bisq.core.monetary.Price; -import bisq.core.monetary.Volume; import bisq.core.offer.Offer; import bisq.core.offer.OfferPayload; import bisq.core.payment.PaymentAccount; @@ -226,22 +224,13 @@ public void initialize() { placeholder.setWrapText(true); tableView.setPlaceholder(placeholder); - marketColumn.setComparator((o1, o2) -> { - String str1 = BSFormatter.getCurrencyPair(o1.getOffer().getCurrencyCode()); - String str2 = BSFormatter.getCurrencyPair(o2.getOffer().getCurrencyCode()); - return str1 != null && str2 != null ? str1.compareTo(str2) : 0; - }); - priceColumn.setComparator((o1, o2) -> { - Price price1 = o1.getOffer().getPrice(); - Price price2 = o2.getOffer().getPrice(); - return price1 != null && price2 != null ? price1.compareTo(price2) : 0; - }); + marketColumn.setComparator(Comparator.comparing( + o -> BSFormatter.getCurrencyPair(o.getOffer().getCurrencyCode()), + Comparator.nullsFirst(Comparator.naturalOrder()) + )); + priceColumn.setComparator(Comparator.comparing(o -> o.getOffer().getPrice(), Comparator.nullsFirst(Comparator.naturalOrder()))); amountColumn.setComparator(Comparator.comparing(o -> o.getOffer().getAmount())); - volumeColumn.setComparator((o1, o2) -> { - Volume offerVolume1 = o1.getOffer().getVolume(); - Volume offerVolume2 = o2.getOffer().getVolume(); - return offerVolume1 != null && offerVolume2 != null ? offerVolume1.compareTo(offerVolume2) : 0; - }); + volumeColumn.setComparator(Comparator.comparing(o -> o.getOffer().getVolume(), Comparator.nullsFirst(Comparator.naturalOrder()))); paymentMethodColumn.setComparator(Comparator.comparing(o -> o.getOffer().getPaymentMethod())); avatarColumn.setComparator(Comparator.comparing(o -> o.getOffer().getOwnerNodeAddress().getFullAddress())); diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesView.java b/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesView.java index 7cb3696114a..435b1c0e71d 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesView.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesView.java @@ -32,8 +32,6 @@ import bisq.core.alert.PrivateNotificationManager; import bisq.core.app.AppOptionKeys; import bisq.core.locale.Res; -import bisq.core.monetary.Price; -import bisq.core.monetary.Volume; import bisq.core.offer.Offer; import bisq.core.offer.OpenOffer; import bisq.core.trade.Contract; @@ -44,8 +42,6 @@ import bisq.network.p2p.NodeAddress; -import org.bitcoinj.core.Coin; - import com.googlecode.jcsv.writer.CSVEntryConverter; import com.google.inject.name.Named; @@ -78,6 +74,7 @@ import javafx.util.Callback; import java.util.Comparator; +import java.util.function.Function; @FxmlView public class ClosedTradesView extends ActivatableViewAndModel { @@ -163,89 +160,26 @@ public void initialize() { directionColumn.setComparator(Comparator.comparing(o -> o.getTradable().getOffer().getDirection())); marketColumn.setComparator(Comparator.comparing(model::getMarketLabel)); - priceColumn.setComparator((o1, o2) -> { - final Tradable tradable1 = o1.getTradable(); - final Tradable tradable2 = o2.getTradable(); - Price price1 = null; - Price price2 = null; - if (tradable1 != null) - price1 = tradable1 instanceof Trade ? ((Trade) tradable1).getTradePrice() : tradable1.getOffer().getPrice(); - if (tradable2 != null) - price2 = tradable2 instanceof Trade ? ((Trade) tradable2).getTradePrice() : tradable2.getOffer().getPrice(); - return price1 != null && price2 != null ? price1.compareTo(price2) : 0; - }); - volumeColumn.setComparator((o1, o2) -> { - if (o1.getTradable() instanceof Trade && o2.getTradable() instanceof Trade) { - Volume tradeVolume1 = ((Trade) o1.getTradable()).getTradeVolume(); - Volume tradeVolume2 = ((Trade) o2.getTradable()).getTradeVolume(); - return tradeVolume1 != null && tradeVolume2 != null ? tradeVolume1.compareTo(tradeVolume2) : 0; - } else - return 0; - }); - amountColumn.setComparator((o1, o2) -> { - if (o1.getTradable() instanceof Trade && o2.getTradable() instanceof Trade) { - Coin amount1 = ((Trade) o1.getTradable()).getTradeAmount(); - Coin amount2 = ((Trade) o2.getTradable()).getTradeAmount(); - return amount1 != null && amount2 != null ? amount1.compareTo(amount2) : 0; - } else - return 0; - }); - avatarColumn.setComparator((o1, o2) -> { - if (o1.getTradable() instanceof Trade && o2.getTradable() instanceof Trade) { - NodeAddress tradingPeerNodeAddress1 = ((Trade) o1.getTradable()).getTradingPeerNodeAddress(); - NodeAddress tradingPeerNodeAddress2 = ((Trade) o2.getTradable()).getTradingPeerNodeAddress(); - String address1 = tradingPeerNodeAddress1 != null ? tradingPeerNodeAddress1.getFullAddress() : ""; - String address2 = tradingPeerNodeAddress2 != null ? tradingPeerNodeAddress2.getFullAddress() : ""; - return address1.compareTo(address2); - } else - return 0; - }); - txFeeColumn.setComparator((o1, o2) -> { - final Tradable tradable1 = o1.getTradable(); - final Tradable tradable2 = o2.getTradable(); - Coin txFee1 = null; - Coin txFee2 = null; - if (tradable1 != null) - txFee1 = tradable1 instanceof Trade ? ((Trade) tradable1).getTxFee() : tradable1.getOffer().getTxFee(); - if (tradable2 != null) - txFee2 = tradable2 instanceof Trade ? ((Trade) tradable2).getTxFee() : tradable2.getOffer().getTxFee(); - return txFee1 != null && txFee2 != null ? txFee1.compareTo(txFee2) : 0; - }); - makerFeeColumn.setComparator((o1, o2) -> { - final Tradable tradable1 = o1.getTradable(); - final Tradable tradable2 = o2.getTradable(); - Coin txFee1 = null; - Coin txFee2 = null; - if (tradable1 != null) - txFee1 = tradable1 instanceof Trade ? ((Trade) tradable1).getTakerFee() - : tradable1.getOffer().getMakerFee(); - if (tradable2 != null) - txFee2 = tradable2 instanceof Trade ? ((Trade) tradable2).getTakerFee() - : tradable2.getOffer().getMakerFee(); - return txFee1 != null && txFee2 != null ? txFee1.compareTo(txFee2) : 0; - }); - buyerSecurityDepositColumn.setComparator((o1, o2) -> { - final Tradable tradable1 = o1.getTradable(); - final Tradable tradable2 = o2.getTradable(); - Coin txFee1 = null; - Coin txFee2 = null; - if (tradable1 != null && tradable1.getOffer() != null) - txFee1 = tradable1.getOffer().getBuyerSecurityDeposit(); - if (tradable2 != null && tradable2.getOffer() != null) - txFee2 = tradable2.getOffer().getBuyerSecurityDeposit(); - return txFee1 != null && txFee2 != null ? txFee1.compareTo(txFee2) : 0; - }); - sellerSecurityDepositColumn.setComparator((o1, o2) -> { - final Tradable tradable1 = o1.getTradable(); - final Tradable tradable2 = o2.getTradable(); - Coin txFee1 = null; - Coin txFee2 = null; - if (tradable1 != null && tradable1.getOffer() != null) - txFee1 = tradable1.getOffer().getSellerSecurityDeposit(); - if (tradable2 != null && tradable2.getOffer() != null) - txFee2 = tradable2.getOffer().getSellerSecurityDeposit(); - return txFee1 != null && txFee2 != null ? txFee1.compareTo(txFee2) : 0; - }); + priceColumn.setComparator(nullsFirstComparing(o -> + o instanceof Trade ? ((Trade) o).getTradePrice() : o.getOffer().getPrice() + )); + volumeColumn.setComparator(nullsFirstComparingAsTrade(Trade::getTradeVolume)); + amountColumn.setComparator(nullsFirstComparingAsTrade(Trade::getTradeAmount)); + avatarColumn.setComparator(nullsFirstComparingAsTrade(o -> + o.getTradingPeerNodeAddress() != null ? o.getTradingPeerNodeAddress().getFullAddress() : null + )); + txFeeColumn.setComparator(nullsFirstComparing(o -> + o instanceof Trade ? ((Trade) o).getTxFee() : o.getOffer().getTxFee() + )); + makerFeeColumn.setComparator(nullsFirstComparing(o -> + o instanceof Trade ? ((Trade) o).getTakerFee() : o.getOffer().getMakerFee() + )); + buyerSecurityDepositColumn.setComparator(nullsFirstComparing(o -> + o.getOffer() != null ? o.getOffer().getBuyerSecurityDeposit() : null + )); + sellerSecurityDepositColumn.setComparator(nullsFirstComparing(o -> + o.getOffer() != null ? o.getOffer().getSellerSecurityDeposit() : null + )); stateColumn.setComparator(Comparator.comparing(model::getState)); dateColumn.setSortType(TableColumn.SortType.DESCENDING); @@ -261,6 +195,20 @@ public void initialize() { HBox.setMargin(exportButton, new Insets(0, 10, 0, 0)); } + private static > Comparator nullsFirstComparing(Function keyExtractor) { + return Comparator.comparing( + o -> o.getTradable() != null ? keyExtractor.apply(o.getTradable()) : null, + Comparator.nullsFirst(Comparator.naturalOrder()) + ); + } + + private static > Comparator nullsFirstComparingAsTrade(Function keyExtractor) { + return Comparator.comparing( + o -> o.getTradable() instanceof Trade ? keyExtractor.apply((Trade) o.getTradable()) : null, + Comparator.nullsFirst(Comparator.naturalOrder()) + ); + } + @Override protected void activate() { filteredList = new FilteredList<>(model.getList()); diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/failedtrades/FailedTradesView.java b/desktop/src/main/java/bisq/desktop/main/portfolio/failedtrades/FailedTradesView.java index 3353a728ef8..940feda4c63 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/failedtrades/FailedTradesView.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/failedtrades/FailedTradesView.java @@ -86,19 +86,8 @@ public void initialize() { tradeIdColumn.setComparator(Comparator.comparing(o -> o.getTrade().getId())); dateColumn.setComparator(Comparator.comparing(o -> o.getTrade().getDate())); priceColumn.setComparator(Comparator.comparing(o -> o.getTrade().getTradePrice())); - - volumeColumn.setComparator((o1, o2) -> { - if (o1.getTrade().getTradeVolume() != null && o2.getTrade().getTradeVolume() != null) - return o1.getTrade().getTradeVolume().compareTo(o2.getTrade().getTradeVolume()); - else - return 0; - }); - amountColumn.setComparator((o1, o2) -> { - if (o1.getTrade().getTradeAmount() != null && o2.getTrade().getTradeAmount() != null) - return o1.getTrade().getTradeAmount().compareTo(o2.getTrade().getTradeAmount()); - else - return 0; - }); + volumeColumn.setComparator(Comparator.comparing(o -> o.getTrade().getTradeVolume(), Comparator.nullsFirst(Comparator.naturalOrder()))); + amountColumn.setComparator(Comparator.comparing(o -> o.getTrade().getTradeAmount(), Comparator.nullsFirst(Comparator.naturalOrder()))); stateColumn.setComparator(Comparator.comparing(model::getState)); marketColumn.setComparator(Comparator.comparing(model::getMarketLabel)); diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java b/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java index 5ae125f56d9..cf80312dc0d 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java @@ -31,8 +31,6 @@ import bisq.desktop.main.portfolio.PortfolioView; import bisq.core.locale.Res; -import bisq.core.monetary.Price; -import bisq.core.monetary.Volume; import bisq.core.offer.OpenOffer; import bisq.core.user.DontShowAgainLookup; @@ -115,16 +113,8 @@ public void initialize() { directionColumn.setComparator(Comparator.comparing(o -> o.getOffer().getDirection())); marketColumn.setComparator(Comparator.comparing(model::getMarketLabel)); amountColumn.setComparator(Comparator.comparing(o -> o.getOffer().getAmount())); - priceColumn.setComparator((o1, o2) -> { - Price price1 = o1.getOffer().getPrice(); - Price price2 = o2.getOffer().getPrice(); - return price1 != null && price2 != null ? price1.compareTo(price2) : 0; - }); - volumeColumn.setComparator((o1, o2) -> { - Volume offerVolume1 = o1.getOffer().getVolume(); - Volume offerVolume2 = o2.getOffer().getVolume(); - return offerVolume1 != null && offerVolume2 != null ? offerVolume1.compareTo(offerVolume2) : 0; - }); + priceColumn.setComparator(Comparator.comparing(o -> o.getOffer().getPrice(), Comparator.nullsFirst(Comparator.naturalOrder()))); + volumeColumn.setComparator(Comparator.comparing(o -> o.getOffer().getVolume(), Comparator.nullsFirst(Comparator.naturalOrder()))); dateColumn.setComparator(Comparator.comparing(o -> o.getOffer().getDate())); dateColumn.setSortType(TableColumn.SortType.DESCENDING); diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java index baba88b46a9..7093a68e36a 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java @@ -180,27 +180,17 @@ public void initialize() { tradeIdColumn.setComparator(Comparator.comparing(o -> o.getTrade().getId())); dateColumn.setComparator(Comparator.comparing(o -> o.getTrade().getDate())); - volumeColumn.setComparator((o1, o2) -> { - if (o1.getTrade().getTradeVolume() != null && o2.getTrade().getTradeVolume() != null) - return o1.getTrade().getTradeVolume().compareTo(o2.getTrade().getTradeVolume()); - else - return 0; - }); - amountColumn.setComparator((o1, o2) -> { - if (o1.getTrade().getTradeAmount() != null && o2.getTrade().getTradeAmount() != null) - return o1.getTrade().getTradeAmount().compareTo(o2.getTrade().getTradeAmount()); - else - return 0; - }); + volumeColumn.setComparator(Comparator.comparing(o -> o.getTrade().getTradeVolume(), Comparator.nullsFirst(Comparator.naturalOrder()))); + amountColumn.setComparator(Comparator.comparing(o -> o.getTrade().getTradeAmount(), Comparator.nullsFirst(Comparator.naturalOrder()))); priceColumn.setComparator(Comparator.comparing(PendingTradesListItem::getPrice)); - paymentMethodColumn.setComparator(Comparator.comparing(o -> o.getTrade().getOffer() != null ? - o.getTrade().getOffer().getPaymentMethod().getId() : null)); - avatarColumn.setComparator((o1, o2) -> { - if (o1.getTrade().getTradingPeerNodeAddress() != null && o2.getTrade().getTradingPeerNodeAddress() != null) - return o1.getTrade().getTradingPeerNodeAddress().getFullAddress().compareTo(o2.getTrade().getTradingPeerNodeAddress().getFullAddress()); - else - return 0; - }); + paymentMethodColumn.setComparator(Comparator.comparing( + o -> o.getTrade().getOffer() != null ? o.getTrade().getOffer().getPaymentMethod().getId() : null, + Comparator.nullsFirst(Comparator.naturalOrder()) + )); + avatarColumn.setComparator(Comparator.comparing( + o -> o.getTrade().getTradingPeerNodeAddress() != null ? o.getTrade().getTradingPeerNodeAddress().getFullAddress() : null, + Comparator.nullsFirst(Comparator.naturalOrder()) + )); roleColumn.setComparator(Comparator.comparing(model::getMyRole)); marketColumn.setComparator(Comparator.comparing(model::getMarketLabel)); diff --git a/desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java b/desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java index 6922b560560..d6284969985 100644 --- a/desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java +++ b/desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java @@ -237,7 +237,7 @@ public void initialize() { }); List> disputeGroups = new ArrayList<>(); map.forEach((key, value) -> disputeGroups.add(value)); - disputeGroups.sort((o1, o2) -> !o1.isEmpty() && !o2.isEmpty() ? o1.get(0).getOpeningDate().compareTo(o2.get(0).getOpeningDate()) : 0); + disputeGroups.sort(Comparator.comparing(o -> !o.isEmpty() ? o.get(0).getOpeningDate() : new Date(0))); StringBuilder stringBuilder = new StringBuilder(); // We don't translate that as it is not intended for the public