Skip to content
This repository was archived by the owner on Nov 5, 2024. It is now read-only.

arabic number pad fix ,shifted from native numbers to standard US num… #2804

Closed
wants to merge 1 commit into from
Closed

arabic number pad fix ,shifted from native numbers to standard US num… #2804

wants to merge 1 commit into from

Conversation

AndroidPoet
Copy link

@AndroidPoet AndroidPoet commented Oct 23, 2023

…ber system

Pull Request (PR) Checklist

Please check if your pull request fulfills the following requirements:

  • The PR is submitted to the main branch.
  • [x]I've read the Contribution Guidelines and my PR doesn't break the "Contributing Rules".
  • I've read the Architecture Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • 🎬 I've attached a screen recoding of the changes.
    bug_fix.webm

Tip: drag & drop the video to the PR description.

What's changed?

Describe with a few bullets what's new:

  • Might effect other areas where Locale is used to format integers.

💡 Tip: Please, attach screenshots and screen recordings. It helps a lot!

Risk Factors

What may go wrong if we merge your PR?

  • Might effect other areas where Locale is used to format intergers.

In what cases your code won't work?

  • I have tested most use cases it worked.

This may be a future issue! I'm not sure if Ivy Wallet will use numbers for these strings. It uses native right-to-left language numbers

Does this PR closes any GitHub Issues?

Check Ivy Wallet Issues.

Replace ISSUE_NUMBER with your issue number (for example Closes #1234). If you've done that correctly, you'll see the issue title linked when previewing your PR description.

Troubleshooting CI failures

If you see any of the PR checks failing (❌) go to Actions and find it there. Or simply click "Details" next to the failed check and explore the logs to see why it has failed.

Detekt

Detekt is a static code analyzer for Kotlin that we use to enforce code readibility and good practices.

To run Detekt locally:

./gradlew detekt

If the Detekt errors are caused by a legacy code, you can suppress them using a basline.

Detekt baseline (not recommended)

./scripts/detektBaseline.sh

Lint

We use the standard Android Lint plus Slack's compose-lints as an addition to enforce proper Compose usage.

To run Lint locally:

./scripts/lint.sh

If the Lint errors are caused by a legacy code, you can suppress them using a basline.

Lint baseline (not recommended)

./scripts/lintBaseline.sh

Unit tests

If this job is failing this means that your changes break an existing unit test. You must identify the failing tests and fix your code.

To run the Unit tests locally:

./gradlew testDebugUnitTest

@AndroidPoet
Copy link
Author

Hi @ILIYANGERMANOV, I need some help! With the above change, it will fix the number system when using Arabic or any right-to-left language.

However,
I also added a screenshot for a potential future issue.Screenshot number value's are in native numbers. Please let me know if this solution is a good approach, and if the screenshot I shared is related to this issue. If not, we can close this one create another issue to address that

@@ -131,7 +132,7 @@ fun shouldShortAmount(amount: Double): Boolean {
}

fun formatInt(number: Int): String {
return DecimalFormat("#,###,###,###").format(number)
return DecimalFormat("#,###,###,###", DecimalFormatSymbols(Locale.US)).format(number)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cna you verify if this breaks:

  • entering amounts in US, IN, Arabic
  • Import/export CSV backup
  • Import/export Backup JSON
  • Import old backup CSV (previous Ivy version)
  • Import old backup JSON zip

I'm concerned that this PR risks breaking this and everything using those functions.

We must add a backup logic unit tests with top priority (not part of this PR but a good issue to help us be safe)

Copy link
Author

Choose a reason for hiding this comment

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

I agree! I saw this under legacy code. This was a bit of a hack. It needs a proper solution. This depends on two questions:

(1.) Are we dealing with right-to-left language separators, which are ',' instead of '.'? If users of that country don't need their native numbers when dealing with calculations, I guess it should be completely removed, and only the US standard number formatter should be used.

(2.) In this part, we are directly manipulating data from the UI, which forces us to handle things according to the UI. I believe the best way to handle this would be for the UI not to be aware of the operations we are performing. It should be in some repository, and, if needed, can be injected anywhere in a centralized system. This way, the whole app will be in sync, instead of using extension functions. This is a really important part of the app.
Number Pad for input

@Composable
fun AmountInput(
    currency: String,
    amount: String,
    decimalCountMax: Int = 2,
    setAmount: (String) -> Unit,
    ) {
    var firstInput by remember { mutableStateOf(true) }


    AmountKeyboard(
        forCalculator = false,
        onNumberPressed = {
            if (firstInput) {
                setAmount(it)
                firstInput = false
            } else {
                val formattedAmount = formatInputAmount(
                    currency = currency,
                    amount = amount,
                    newSymbol = it,
                    decimalCountMax = decimalCountMax
                )
                if (formattedAmount != null) {
                    setAmount(formattedAmount)
                }
            }
        },
        onDecimalPoint = {
            if (firstInput) {
                setAmount("0${localDecimalSeparator()}")
                firstInput = false
            } else {
                val newlyEnteredString = if (amount.isEmpty()) {
                    "0${localDecimalSeparator()}"
                } else {
                    "$amount${localDecimalSeparator()}"
                }
                if (newlyEnteredString.amountToDoubleOrNull() != null) {
                    setAmount(newlyEnteredString)
                }
            }
        },
        onBackspace = {
            if (firstInput) {
                setAmount("")
                firstInput = false
            } else {
                if (amount.isNotEmpty()) {
                    val formattedNumber = formatNumber(amount.dropLast(1))
                    setAmount(formattedNumber ?: "")
                }
            }
        }
    )
}

My suggestion might be overkill or might not be perfect. I would love to hear from you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we need to centralize it and we started such efforts with out architecture and :ivy-data. I don't have much context on this task so apply the solution that makes the most sense to you. My only requirement is to not break the existing legacy code. Critical features:

  • numpad on all Locales
  • backups

If this change can't be applied safely my suggestion would be to postpone this task and focus migrating the app to the new architecture and adding tests

Copy link
Author

Choose a reason for hiding this comment

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

Let's move on to other tasks for now. I'm still relatively new to the source code, and there are backups involved. Once we gain more confidence in the architecture and I become more familiar with the code, we can revisit this task.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense 👍 This task is complex to implement even if you're familiar cuz of the shitty legacy code 😄 Apologies for it!

Copy link
Author

Choose a reason for hiding this comment

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

No worries! We will migrate it slowly.

@github-actions github-actions bot added the Stale No activity, will be automatically closed soon. label Oct 27, 2023
@github-actions github-actions bot closed this Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Stale No activity, will be automatically closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example issue [BUG] Can't enter numbers
2 participants