Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secondary sort order for offer book (rewrite) #4259

Merged

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented May 15, 2020

Supersedes #4168. This is a rewrite of that PR.

Adds a secondary sort order to offers in market offer book by offer amount that goes from high to low. Also, replaces previous complicated implementation of primary sort.

Co-authored-by: @freimair
Co-authored-by: @cd2357

offerbook-secondary-sort-refactor

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 <office@florianreimair.at>
Co-authored-by: cd2357 <15956136+cd2357@users.noreply.github.com>
@dmos62 dmos62 changed the title Secondary sort order for offer book Secondary sort order for offer book (rewrite) May 15, 2020
@ghost
Copy link

ghost commented May 16, 2020

The class OfferBookChartViewModel has test-file OfferBookChartViewModelTest I believe the change should be reflected in the tests as well.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, ACK. Nice & clean.
[edit] There's a bug in altcoin markets - appears to sort the price incorrectly.

image

@dmos62
Copy link
Contributor Author

dmos62 commented May 25, 2020

@jmacxx thanks again! Fixed the ordering. I hadn't noticed that the tables are switched in Altcoin markets. Included an in-code explanation.
offer book bsq fixed altcoin sort
offerbook eur fixed altcoin sort

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good in both fiat and altcoin markets now.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@@ -276,22 +276,32 @@ private boolean isAnyPricePresent() {
}

private void updateChartData() {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer no blank lines at the beginning of methods.

@sqrrm sqrrm merged commit 6665713 into bisq-network:master May 25, 2020
@sqrrm sqrrm added this to the v1.3.5 milestone May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants