Secondary sort order in Market Offer Lists #4168
Conversation
This check hasn't been in place before. However, during testing, I encountered it once. Not sure why it can happen and how to react properly. NullPointerException is prohibited. Lets see if sorting issue pop up sometime.
desktop/src/main/java/bisq/desktop/main/market/offerbook/OfferBookChartViewModel.java
Outdated
Show resolved
Hide resolved
|
ACK from a functionality point of view. Could you add this to the actual Offerbook as well? |
desktop/src/main/java/bisq/desktop/main/market/offerbook/OfferBookChartViewModel.java
Outdated
Show resolved
Hide resolved
Actually, I would prefer it that is a separate issue. Reasons are that it is not trivial to code that into Bisq, because the other lists can be sorted by clicking the column headers.
I chose this representation because it is better than having it all scrambled up and because more BSQ trades are done with small amounts at the moment. |
…BookChartViewModel.java Co-authored-by: cd2357 <[email protected]>
There was a problem hiding this comment.
NACK
The routine is very complicated, especially the primary comparator, because we're checking inside the comparator (twice, for some reason) for whether we're comparing sell offers or buy offers and then we're reversing order inside it as well. That shouldn't happen in the comparator. The comparator in both sell and buy cases should be Comparator.comparing(Offer::getPrice).thenComparing(Offer::getAmount), with a .reversed.() inserted where needed, for example: Comparator.comparing(Offer::getPrice).reversed().thenComparing(Offer::getAmount)
The rest of the functionality should be achieved by filtering the stream on whether it's a buy/sell (which is already being done at
| * @param reversePrimarySortOrder | ||
| * @return | ||
| */ | ||
| private Comparator<Offer> getComparatorWithSecondarySortOrder(boolean reversePrimarySortOrder) { |
There was a problem hiding this comment.
I think this name is preferable: getOfferComparator(...). The problem with getComparator was just that it didn't mention the object of comparison.
There was a problem hiding this comment.
Not requesting this change, since the whole method should be unnecessary.
|
@dmos62 originally, there is some double-check whether the Price is a crypto currency or not. Idk where that came from or what it is good for. But surely, there is a reason for that. So I kept it and just made it work. To your suggestion, of course, in a perfect world, that would be what we should do. Fully agree on that. Please go ahead an open a PR doing it just like that. I cannot not spend more time on that. |
|
Edit: posted link to branch with my changes, but then realised it will be easier to review if I squash to one commit and post to a second PR that supersedes this one. |
A rewrite of @freimair's PR bisq-network#4168. Adds a secondary sort order of offers in market offer book by offer amount that goes from high to low. Also, refactors-away overcomplicated previous implementation of primary sort. Co-authored-by: Florian Reimair <[email protected]> Co-authored-by: cd2357 <[email protected]>
|
Can this PR be closed? |
Fixes #4167
before:
after:
