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

Added the Decimal Support while entering target for any Habit. #911

Closed
wants to merge 1 commit into from

Conversation

sumanabhi
Copy link
Contributor

@sumanabhi sumanabhi commented May 18, 2021

Added the Decimal Support while entering the target for any Habit.

This PR will resolve this issue #900

Changes I made:-

Change the input type for Target EditText

You can check attached Screenshots here ->

Screenshot_20210519-030614

@sumanabhi sumanabhi marked this pull request as ready for review May 18, 2021 21:53
@hiqua
Copy link
Collaborator

hiqua commented May 18, 2021

My issue was about when you entered a new checkmark, while you seem to have looked at new habits, correct? Are these related?

In any case I think the (well "my") idea was more to keep the current interface, but just allow the user to switch to the fractional part by using . from the numpad.

Another thing is that currently, the measure can only be specified by increments of 0.05, while your changes seem to allow any float as input. I'm not sure what the reasoning was to have this restriction, but it could have been to avoid weird rounding issues with floats.

My idea initially when opening the issue was that once you've entered the digit, it would be nice to finish entering the full measure by just pressing . then the fractional part.

On the screenshot below, the user has entered 1 and could now enter .05 instead of scrolling to enter the fractional part.

If my intuition above regarding floats is correct (I haven't checked), I think the fractional part should then be rounded to the closest possibility (e.g. 1.04 to 1.05).

signal-2021-05-19-011525

@iSoron
Copy link
Owner

iSoron commented May 19, 2021

Thank you for the PR, @sumanabhi. Issue #900 is about something else, but I think we should allow fractional target values. The integer restriction in this form was just by accident. I'm planning to merge this PR in the branch hotfix/2.0.2.

Another thing is that currently, the measure can only be specified by increments of 0.05, while your changes seem to allow any float as input. I'm not sure what the reasoning was to have this restriction, but it could have been to avoid weird rounding issues with floats.

You're right; we store entries as integer numbers to avoid rounding issues with floating point numbers. The 0.05 increment restriction, however, was implemented just to make it easier to scroll to the desired fractional value. If we have a faster way to enter the fractional part of the value, that restriction can be removed.

@hiqua
Copy link
Collaborator

hiqua commented May 19, 2021

You're right; we store entries as integer numbers to avoid rounding issues with floating point numbers. The 0.05 increment restriction, however, was implemented just to make it easier to scroll to the desired fractional value. If we have a faster way to enter the fractional part of the value, that restriction can be removed.

So we can just use the same kind of field as in this PR for new checkmark entries then, if I understood you correctly?

@sumanabhi
Copy link
Contributor Author

Hey @iSoron @hiqua ,

Thanks for the approval of PR.

I think I was misunderstood the scenario due to not having detailed knowledge of application. But now after seeing the screenshot attached by @hiqua I can check and try to understand the issue and then will let you know if I'm able to solve it or not.

@iSoron
Copy link
Owner

iSoron commented May 22, 2021

Merged in branch hotfix/2.0.2 (b1c53bd)

@iSoron iSoron closed this May 22, 2021
@iSoron
Copy link
Owner

iSoron commented May 22, 2021

So we can just use the same kind of field as in this PR for new checkmark entries then, if I understood you correctly?

@hiqua Sorry, your suggestion is not clear to me. Are you proposing applying the solution from this PR to number_picker_dialog.xml? Note that we currently use two NumberPickers in that XML layout, so that users can either type or scroll, whichever is the most convenient. If we switch to EditText with numberDecimal as input type, users would lose the ability to scroll.

@iSoron iSoron added this to the Loop 2.0.2 milestone May 22, 2021
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.

3 participants