Skip to content

Commit

Permalink
Fix invalid neutral element handling in View comparators
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@stejbac
stejbac committed Sep 17, 2019
1 parent 10bed7a commit a392081
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 162 deletions.
8 changes: 2 additions & 6 deletions desktop/src/main/java/bisq/desktop/main/funds/locked/LockedView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down
8 changes: 2 additions & 6 deletions desktop/src/main/java/bisq/desktop/main/funds/reserved/ReservedView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down
23 changes: 6 additions & 17 deletions desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()));

Expand Down
122 changes: 35 additions & 87 deletions desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -78,6 +74,7 @@
import javafx.util.Callback;

import java.util.Comparator;
import java.util.function.Function;

@FxmlView
public class ClosedTradesView extends ActivatableViewAndModel<VBox, ClosedTradesViewModel> {
Expand Down Expand Up @@ -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);
Expand All @@ -261,6 +195,20 @@ public void initialize() {
HBox.setMargin(exportButton, new Insets(0, 10, 0, 0));
}

private static <T extends Comparable<T>> Comparator<ClosedTradableListItem> nullsFirstComparing(Function<Tradable, T> keyExtractor) {
return Comparator.comparing(
o -> o.getTradable() != null ? keyExtractor.apply(o.getTradable()) : null,
Comparator.nullsFirst(Comparator.naturalOrder())
);
}

private static <T extends Comparable<T>> Comparator<ClosedTradableListItem> nullsFirstComparingAsTrade(Function<Trade, T> 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());
Expand Down
15 changes: 2 additions & 13 deletions desktop/src/main/java/bisq/desktop/main/portfolio/failedtrades/FailedTradesView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
14 changes: 2 additions & 12 deletions desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
30 changes: 10 additions & 20 deletions desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
2 changes: 1 addition & 1 deletion desktop/src/main/java/bisq/desktop/main/support/dispute/DisputeView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ public void initialize() {
});
List<List<Dispute>> 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
Expand Down

0 comments on commit a392081

Please sign in to comment.