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

Apply Localized Number Formatting To Transaction Details #5902

Closed

Conversation

alvasw
Copy link
Contributor

@alvasw alvasw commented Dec 7, 2021

Fixes #3145

By default all amounts are separated by a "." In some locales that is
not the correct separator. This change formats all amounts and
percentages using the correct locale.

@boring-cyborg
Copy link

boring-cyborg bot commented Dec 7, 2021

Thanks for opening this pull request!

Please check out our contributor checklist and check if Travis or Codacy found any issues with your PR. Also make sure your commits are signed, and that you applied Bisq's code style and formatting.

A maintainer will add an is:priority label to your PR if it is up for compensation. Please see our Bisq Q1 2020 Update post for more details.

@ripcurlx ripcurlx added this to the v1.8.1 milestone Dec 8, 2021
@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 9, 2021

Changing to a localized formatting broke some tests if a different locale than the US_en is set.
Having DE_de causes following tests to fail:

OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

bisq.desktop.main.offer.bisq_v1.createoffer.CreateOfferViewModelTest > testSyncMinAmountWithAmountWhenHigherMinAmountValueIsSet FAILED
    org.junit.ComparisonFailure at CreateOfferViewModelTest.java:222

bisq.desktop.main.offer.bisq_v1.createoffer.CreateOfferViewModelTest > testSyncMinAmountWithAmountWhenSameValueIsSet FAILED
    org.junit.ComparisonFailure at CreateOfferViewModelTest.java:206

bisq.desktop.main.offer.bisq_v1.createoffer.CreateOfferViewModelTest > testSyncMinAmountWithAmountUntilChanged FAILED
    org.junit.ComparisonFailure at CreateOfferViewModelTest.java:166

bisq.desktop.main.offer.bisq_v1.createoffer.CreateOfferViewModelTest > testSyncPriceMarginWithVolumeAndFixedPrice FAILED
    org.junit.ComparisonFailure at CreateOfferViewModelTest.java:237

bisq.desktop.main.offer.bisq_v1.createoffer.CreateOfferViewModelTest > testSyncMinAmountWithAmountWhenZeroCoinIsSet FAILED
    org.junit.ComparisonFailure at CreateOfferViewModelTest.java:189

bisq.desktop.main.offer.offerbook.OfferBookViewModelTest > testGetPrice FAILED
    org.junit.ComparisonFailure at OfferBookViewModelTest.java:453

bisq.desktop.util.DisplayUtilsTest > testFormatSameVolume FAILED
    org.junit.ComparisonFailure at DisplayUtilsTest.java:67

bisq.desktop.util.DisplayUtilsTest > testFormatDifferentVolume FAILED
    org.junit.ComparisonFailure at DisplayUtilsTest.java:79

126 tests completed, 8 failed, 7 skipped

> Task :desktop:test FAILED

Could you please adapt those tests, so they don't use the current locale - Thanks!

By default all amounts are separated by a "." In some locales that is
not the correct separator. This change formats all amounts and
percentages using the correct locale.

Closes bisq-network#3145
@alvasw alvasw force-pushed the apply_localized_number_formatting branch from f66b1d3 to ebb40ff Compare December 10, 2021 21:34
@alvasw
Copy link
Contributor Author

alvasw commented Dec 10, 2021

Oops, didn't think about that. I switched my OS to German and fixed the code.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK - There are a couple of issues that need to get addressed.

If we localize the BSQ formatting it also needs to be done in one go for the other number formatting as well. Otherwise we end in situations like in this screenshot:
Bildschirmfoto 2021-12-20 um 11 27 11

I also found an exception when clicking around on the Mainnet app when entering Facts & Figures > BSQ Supply.

java.lang.NumberFormatException: For input string: "0,00"
	at java.base/jdk.internal.math.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:2054)
	at java.base/jdk.internal.math.FloatingDecimal.parseDouble(FloatingDecimal.java:110)
	at java.base/java.lang.Double.parseDouble(Double.java:549)
	at bisq.desktop.main.dao.economy.supply.dao.DaoChartViewModel$1.toString(DaoChartViewModel.java:91)
	at bisq.desktop.main.dao.economy.supply.dao.DaoChartViewModel$1.toString(DaoChartViewModel.java:88)
	at javafx.scene.chart.NumberAxis.measureTickMarkSize(NumberAxis.java:334)

I haven't tried it out yet, but I think it is also good to try out a regular BSQ trade (Altcoin or Altcoin instant) to see if the amount copy & pasting also doesn't have any issues with the new formatting.

@ripcurlx
Copy link
Contributor

@alvasw Do you want to continue to work on this or is this open for other developers to pick it up from here?

@alvasw
Copy link
Contributor Author

alvasw commented Jan 26, 2022

Hey @ripcurlx!
Sorry for stalling this pull request. I'm pretty busy at the moment. It's open for anyone to continue.

@alvasw alvasw closed this Jan 26, 2022
@ghubstan
Copy link
Contributor

Fixes #3145

By default all amounts are separated by a "." In some locales that is not the correct separator. This change formats all amounts and percentages using the correct locale.

I am a little late to respond to this, but using the US locale for CLI output was a deliberate decision at the beginning of the Bisq 1 API work. So was using English for all CLI console output.

@alvasw alvasw deleted the apply_localized_number_formatting branch February 6, 2023 12:04
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.

Use localised number formats for transaction details
3 participants