[bisq-network/bisq] Improve chat functionality of mediation/arbitration (#5207)

[Christoph Atteneder]

@ripcurlx requested changes on this pull request.

NACK - Some naming suggestions and comments I would leave out of this PR.

Also I tested the functionality and found following UX issue:

When a new mediation case is opened it looks like that to the user:
![Bildschirmfoto 2021-03-15 um 11 39 47](https://user-images.githubusercontent.com/170962/111140918-2ef5c200-8583-11eb-82e4-f23e03c80245.png)

It is not immediately clear to me what the action I need to do to get rid of the `New` badge (I need to open the chat). For this initial system message I think the chat icon should have a (1) badge already, so users are clicking on it. I also think it would be great to open the chat window on double click of the dispute item as the default action. WDYT? I'm continuing to test the remaining functionality and will add my comments below. But besides that - great improvements already 👍 


> @@ -104,6 +104,7 @@
     private final TradeWalletService tradeWalletService;
     private final BtcWalletService btcWalletService;
     private final TxFeeEstimationService txFeeEstimationService;
+    // PR5160 will add private final MempoolService mempoolService;

Why do you want to add this comment?

> @@ -141,6 +143,7 @@ public DisputeSummaryWindow(@Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormat
                                 TradeWalletService tradeWalletService,
                                 BtcWalletService btcWalletService,
                                 TxFeeEstimationService txFeeEstimationService,
+                                // PR5160 will add MempoolService mempoolService,

Same here: why add this comment?

> @@ -149,6 +152,7 @@ public DisputeSummaryWindow(@Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormat
         this.tradeWalletService = tradeWalletService;
         this.btcWalletService = btcWalletService;
         this.txFeeEstimationService = txFeeEstimationService;
+        // PR5160 will add this.mempoolService = mempoolService;

and same here

> @@ -314,6 +303,26 @@ private void addInfoPane() {
                 " " +
                 formatter.formatCoinWithCode(contract.getOfferPayload().getSellerSecurityDeposit());
         addConfirmationLabelLabel(gridPane, ++rowIndex, Res.get("shared.securityDeposit"), securityDeposit);
+
+        boolean isMediationDispute = getDisputeManager(dispute) instanceof MediationManager;
+        if (isMediationDispute) {
+            if (dispute.getTradePeriodEnd().getTime() > 0) {
+                String status = DisplayUtils.formatDateTime(dispute.getTradePeriodEnd());
+                Label tpe = addConfirmationLabelLabel(gridPane, ++rowIndex, Res.get("disputeSummaryWindow.tradePeriodEnd"), status).second;

It might be obvious to others. What does `tpe` stand for?

> +/*
+            mempoolService.checkTxIsConfirmed(dispute.getDelayedPayoutTxId(), (status -> {
+                log.warn("Mempool check confirmation status of DelayedPayoutTxId returned: [{}]", status);
+                dispute.setPayoutTxConfirms(status);
+                displayPayoutStatus(status);
+            }));
+*/

I don't think this comment should be checked in.

>          this.supportManager = supportManager;
         this.formatter = formatter;
+        this.cptyName = cptyName;

`cpty` stands for?

>      // We do not persist uid, it is only used by dispute agents to guarantee an uid.
     @Setter
     @Nullable
     private transient String uid;
+    @Setter
+    private transient long payoutTxConfirms = -1;
+
+    private transient final BooleanProperty isClosedProperty = new SimpleBooleanProperty();
+    private transient final IntegerProperty alertCountProperty = new SimpleIntegerProperty();

🤔 not sure if alert count is the perfect naming. Maybe notificationCount or badgeCount would be more common for this?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/5207#pullrequestreview-612003611
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20210315/e40ca558/attachment.htm>