Add missing filtering on lists #5963

ghost · 2022-01-12T15:13:51Z

PR introduce possibility to filtering these lists:

Portfolio -> Open Trades
Funds -> Receive Funds
Funds -> Send Funds
Funds -> Reserved Funds
Funds -> Locked Funds
Funds -> Transactions

Code from Funds -> Transactions was also simplified to be more consistent with whole code base.

chimp1984 · 2022-01-20T17:07:42Z

desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java

+            if (filterString.isEmpty())
+                return true;
+
+            if (item.getBalance().toString().contains(filterString)) {


This does not cover formatted string. E.g. if balance is 0.01 BTC and you type in 0.01 it is not found.
Better create a new string field in the itme (formatter.formatCoin(this.balance) which reflects the displayed string

desktop/src/main/java/bisq/desktop/main/funds/deposit/DepositView.fxml

chimp1984 · 2022-01-20T17:13:21Z

desktop/src/main/java/bisq/desktop/main/funds/locked/LockedView.java

+                boolean matchesBuyersPaymentAccountData = false;
+                boolean matchesSellersPaymentAccountData = false;
+                if (contract != null) {
+                    isBuyerOnion = contract.getBuyerNodeAddress().getFullAddress().contains(filterString);


Those predicates appear several times. Would be good to make it reusable code

chimp1984 · 2022-01-20T17:21:03Z

desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionsView.java

+                                            pubKeyRing
+                                    );
+                                } else {
+                                    return new DummyTransactionAwareTradable(tradable);


I know that is from old code, but as that case never gets executed I think we should return null here and then filter out null in the next step. Tradable is only extended from OpenOffer and TradeModel (one other case is not relevant here). The DummyTransactionAwareTradable can be removed then.

chimp1984 · 2022-01-20T17:26:17Z

I have not looked closely to the other filter predicates. But basically it shoud always take the display string as that is what users would expect to be able to filter. Maybe there are some cases where additional domain data filtering is useful as well, but should be additional to the display string case.
As we have not plenty of filters in the app it would be good to have them re-usable also with reusable predicates where it makes sense.

I am not so 100% sure if we should show the filter fields by default or extend it just when the user demand it. E.g. add a filter icon and when the user opens it show the field. That would help to not overload more the already overloaded UI.
We can persist the selection of hte icon, so users who prefer to have the filter get it automatically displayed at next restart.

I know the earlier filter implementations have been done the same way (by myself), but as we extend it now to more screens I think its worth to put a bit more effort into it.

Beside that great to see that filter added to more screens.

ghost · 2022-01-21T20:23:21Z

@chimp1984 I fixed these small issues you mentioned on particular code lines.

1.

Regarding code reusability I've moved it forward, details:

I created FilteringUtils that supports filtering upon certain entities
I created generic FilterBox component that encapsulates filtering layout, listener and common logic

The main problem I have now is some inconsistency in code - fields are rendered in two ways:
A) with model usage, like here
B) inside item, like here

I think we should unify it (personally I think B approach is better). What do you think?

I'll appreciate your feedback about changes I made so far.

2.

basically it shoud always take the display string as that is what users would expect to be able to filter. Maybe there are some cases where additional domain data filtering is useful as well, but should be additional to the display string case.

Let's polish it after all of the work regarding reusability will be done

3.

I am not so 100% sure if we should show the filter fields by default or extend it just when the user demand it. E.g. add a filter icon and when the user opens it show the field. That would help to not overload more the already overloaded UI.
We can persist the selection of hte icon, so users who prefer to have the filter get it automatically displayed at next restart.

It seems to be quite complex - I prefer not doing it in the scope of this particular pull request.

chimp1984 · 2022-01-21T21:33:26Z

I think we should unify it (personally I think B approach is better). What do you think?

Yes agree. Don't remember the reasons why that was in the ViewModel, maybe because it accessed other ViewDataModel code or as it was already used from earlier code. But I also dislike that.

Rest looks good to me. Not sure if I find time for a full review... If @ripcurlx does not find time ping me again.

Thanks.

ghost · 2022-01-22T16:06:46Z

I think we should unify it (personally I think B approach is better). What do you think?

Yes agree.

This part is finished - FilterBox is now used consistently across all of the lists.

ripcurlx · 2022-01-24T15:36:10Z

I'll try to review it tomorrow. Could you please resolve the conflict of this PR with master which was caused by a merge today. Thanks!

…ring

…rBox

… FilterBox

ghost · 2022-01-24T16:24:12Z

Thanks @ripcurlx. It's rebased.

desktop/src/main/java/bisq/desktop/components/list/FilterBox.java

ripcurlx · 2022-01-25T11:54:11Z

I'm in the middle of an in-depth code review and started now to test it in the client itself.
When entering send funds it fails with following exception:

java.lang.NullPointerException: Cannot invoke "bisq.desktop.components.list.FilterBox.initialize(javafx.collections.transformation.FilteredList, javafx.scene.control.TableView)" because "this.filterBox" is null
	at bisq.desktop.main.funds.withdrawal.WithdrawalView.initialize(WithdrawalView.java:190)
	at bisq.desktop.common.view.InitializableView.initialize(InitializableView.java:41)
	at javafx.fxml.FXMLLoader.loadImpl(FXMLLoader.java:2655)
	at javafx.fxml.FXMLLoader.loadImpl(FXMLLoader.java:2548)
	at javafx.fxml.FXMLLoader.load(FXMLLoader.java:2517)
	at bisq.desktop.common.fxml.FxmlViewLoader.loadFromFxml(FxmlViewLoader.java:107)

ripcurlx · 2022-01-25T11:56:50Z

I think it would be great to ignore lower upper case differentiation in transactions on the Details column as users are expecting it like that IMO

ripcurlx · 2022-01-25T12:05:12Z

I think it would be great to ignore lower upper case differentiation in transactions on the Details column as users are expecting it like that IMO

Similar behavior would be great for:

My Open Offers for Date/Time, Market, payment method and offer type
Open Trades for Date/Time, Market,Payment method and My role
Unconfirmed BSQ Swaps for Date/Time,Market and Offer type (actually not sure why we display the market at all in BSQ Swaps)
History for Date/Time, Market, Offer type and status
Failed for Date/Time, Market, Offer type and status

But great to see this option now for all lists 👍

desktop/src/main/java/bisq/desktop/main/portfolio/bsqswaps/UnconfirmedBsqSwapsView.java

ghost · 2022-01-25T14:13:41Z

@ripcurlx Thanks for review - I either fixed or put additional explanation on comments

ripcurlx

ACK - Tested it on Regtest

ghost · 2022-02-01T16:10:47Z

Column sorting by date is broken in portfolio -> closed trades view. It used to sort by date, now it sorts by date string.

pazza83 · 2022-03-12T23:14:12Z

@xyzmaker123 thanks for this. It is working well for me.

ghost marked this pull request as ready for review January 13, 2022 09:59

ghost mentioned this pull request Jan 15, 2022

For cycle 32 bisq-network/compensation#977

Closed

ripcurlx added this to the v1.8.2 milestone Jan 17, 2022

chimp1984 suggested changes Jan 20, 2022

View reviewed changes

ghost mentioned this pull request Jan 21, 2022

FXML refactoring #5989

Closed

3 tasks

ghost requested a review from chimp1984 January 21, 2022 20:23

xyzmaker123 added 15 commits January 24, 2022 17:19

Add missing filtering on lists

b0f1897

Add missing filtering on lists - use formatted balance string

e3d562b

Add missing filtering on lists - remove DummyTransactionAwareTradable

2c920b4

Add missing filtering on lists - reusable code for contract filtering

135541f

Add missing filtering on lists - reusable code for offer filtering

4cace81

Add missing filtering on lists - reusable code for BsqSwapTrade filte…

4cd744f

…ring

Add missing filtering on lists - reusable code for Trade filtering

1b571e9

Add missing filtering on lists - introduce FilterableListItem

06b73a6

Add missing filtering on lists - fix codacy

291ccef

Add missing filtering on lists - introduce shared FilterBox

75b9902

Add missing filtering on lists - adapt PendingTradesView to use Filte…

752606d

…rBox

Add missing filtering on lists - adapt OpenOffersView to use FilterBox

caee121

Add missing filtering on lists - adapt FailedTradesView to use FilterBox

f381644

Add missing filtering on lists - adapt UnconfirmedBsqSwapsView to use…

1791a38

… FilterBox

Add missing filtering on lists - adapt ClosedTradesView to use FilterBox

3c43e42

ripcurlx reviewed Jan 25, 2022

View reviewed changes

desktop/src/main/java/bisq/desktop/components/list/FilterBox.java Show resolved Hide resolved

ripcurlx reviewed Jan 25, 2022

View reviewed changes

desktop/src/main/java/bisq/desktop/main/portfolio/bsqswaps/UnconfirmedBsqSwapsView.java Show resolved Hide resolved

xyzmaker123 added 3 commits January 25, 2022 14:37

Add missing filtering on lists - fix NPE on WithdrawalView

024db34

Add missing filtering on lists - fix license

4268d5e

Add missing filtering on lists - use containsIgnoreCase

98a8a2e

ghost requested a review from ripcurlx January 25, 2022 14:13

ripcurlx approved these changes Jan 26, 2022

View reviewed changes

ripcurlx merged commit ffd2a92 into bisq-network:master Jan 26, 2022

ghost mentioned this pull request Feb 2, 2022

Column sorting - fix string comparators #6019

Merged

ghost mentioned this pull request Feb 15, 2022

For cycle 33 bisq-network/compensation#998

Closed

ripcurlx mentioned this pull request Feb 23, 2022

For Cycle 33 bisq-network/compensation#1016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing filtering on lists #5963

Add missing filtering on lists #5963

ghost commented Jan 12, 2022

chimp1984 Jan 20, 2022

ghost Jan 21, 2022

chimp1984 Jan 20, 2022

ghost Jan 21, 2022

chimp1984 Jan 20, 2022

ghost Jan 21, 2022

chimp1984 commented Jan 20, 2022 •

edited

Loading

ghost commented Jan 21, 2022

chimp1984 commented Jan 21, 2022

ghost commented Jan 22, 2022

ripcurlx commented Jan 24, 2022

ghost commented Jan 24, 2022

ripcurlx commented Jan 25, 2022

ripcurlx commented Jan 25, 2022

ripcurlx commented Jan 25, 2022

ghost commented Jan 25, 2022

ripcurlx left a comment

ghost commented Feb 1, 2022

pazza83 commented Mar 12, 2022

Add missing filtering on lists #5963

Add missing filtering on lists #5963

Conversation

ghost commented Jan 12, 2022

chimp1984 Jan 20, 2022

Choose a reason for hiding this comment

ghost Jan 21, 2022

Choose a reason for hiding this comment

chimp1984 Jan 20, 2022

Choose a reason for hiding this comment

ghost Jan 21, 2022

Choose a reason for hiding this comment

chimp1984 Jan 20, 2022

Choose a reason for hiding this comment

ghost Jan 21, 2022

Choose a reason for hiding this comment

chimp1984 commented Jan 20, 2022 • edited Loading

ghost commented Jan 21, 2022

1.

2.

3.

chimp1984 commented Jan 21, 2022

ghost commented Jan 22, 2022

ripcurlx commented Jan 24, 2022

ghost commented Jan 24, 2022

ripcurlx commented Jan 25, 2022

ripcurlx commented Jan 25, 2022

ripcurlx commented Jan 25, 2022

ghost commented Jan 25, 2022

ripcurlx left a comment

Choose a reason for hiding this comment

ghost commented Feb 1, 2022

pazza83 commented Mar 12, 2022

chimp1984 commented Jan 20, 2022 •

edited

Loading