From 10bed7aafefc548b47f0a09aba564349edeb633f Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 12 Sep 2019 22:25:25 +0100 Subject: [PATCH 1/2] Remove redundant/invalid null handling from PaymentMethod.compareTo 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. --- .../java/bisq/core/payment/payload/PaymentMethod.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/bisq/core/payment/payload/PaymentMethod.java b/core/src/main/java/bisq/core/payment/payload/PaymentMethod.java index 983d0177e62..ff04f767398 100644 --- a/core/src/main/java/bisq/core/payment/payload/PaymentMethod.java +++ b/core/src/main/java/bisq/core/payment/payload/PaymentMethod.java @@ -42,7 +42,7 @@ @EqualsAndHashCode(exclude = {"maxTradePeriod", "maxTradeLimit"}) @ToString @Slf4j -public final class PaymentMethod implements PersistablePayload, Comparable { +public final class PaymentMethod implements PersistablePayload, Comparable { /////////////////////////////////////////////////////////////////////////////////////////// // Static @@ -311,11 +311,8 @@ else if (maxTradeLimit == DEFAULT_TRADE_LIMIT_HIGH_RISK.value) } @Override - public int compareTo(@NotNull Object other) { - if (id != null) - return id.compareTo(((PaymentMethod) other).id); - else - return 0; + public int compareTo(@NotNull PaymentMethod other) { + return id.compareTo(other.id); } public boolean isAsset() { From a392081462995bc02cca75c9bafaa8674254a92c Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 12 Sep 2019 23:38:36 +0100 Subject: [PATCH 2/2] 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