kskandis commented 4 months ago

Resolves Incorrect bolus amount entered due to data entry problem #358

dnzxy commented 4 months ago

Thanks for providing this fix 💪 How does this relate or influence or affect #352 and #356 ?

kskandis commented 4 months ago

Thanks for providing this fix 💪 How does this relate or influence or affect #352 and #356 ? Sorry, I didn't see these PRs.

352 does include a removal of the groupingseparator for `extension TextFieldWithToolBar.Coordinator: UITextFieldDelegate {`which is also in this PR, so there could be a minor file change conflict, easy to resolve.

Remaining fixes in #352 are for extension TextFieldWithToolBarString.Coordinator: UITextFieldDelegate { which this PR does not address, so there should be no conflict.

352 does not include the primary fix for `extension TextFieldWithToolBar.Coordinator` in this PR though.

356 issues are not addressed in the PR. Files changed would have no conflict.

dnzxy commented 4 months ago

Thanks for the quick reply. So I think #352 can be closed and #356 will not be affected - if this is tested and approved and fixes the issue in full?

kskandis commented 4 months ago

So I think #352 can be closed

No, #352 has fixes to the Note textfield which THIS PR does not address so that is a separate fix and assuming test and works, should be merged. #352 ALSO has a few lines in its code that this PR also has very similar code for the thousand separator symbol. What I can do is merge #352's thousand separator code into this PR so that it is exactly the same and hence there would be no conflicts. Then you can merge this PR and afterwards merge #352. Would that work?

OR alternatively, #352 author could remove the thousand separator code fix since it is unrelated to the Note field. Then both of these PRs can be merged w/out conflict.

MikePlante1 commented 4 months ago

OR alternatively, #352 author could remove the thousand separator code fix since it is unrelated to the Note field. Then both of these PRs can be merged w/out conflict.

Okay. This has now been removed from my PR.

marionbarker commented 4 months ago

Summary

There are places where you cannot type #.0# and get anything other than ##. Perhaps none the places should accept those values, but if people type them, their entry does not match what was typed.

Configuration

iPhone SE, iOS 17.5.1, blank instance (deleted app before starting)
Trio dev, commit dff37ac2
Applied all open "entry" problem PR (got fresh patches from the PR as needed)
all applied without errors

my CLI:

dff37ac2 (HEAD -> dev, origin/dev, origin/HEAD) Merge pull request #359 from kskandis/dev

 1016  git apply ~/dev/git_local/trio-patches/trio-pr-362-uitextfield-issue.patch
 1017  git apply ~/dev/git_local/trio-patches/trio-pr-356-mmol-fix.patch
 1018  git apply ~/dev/git_local/trio-patches/trio-pr-352_deleting-text-bug.patch

Testing:

Work my way through adding information to a fresh build - remark if there are issues.

Could not enter #.0# for these fields

Notifications;

Low and High in mmol/L (note that 4.1 was ok, but type 4.05 and get 45)
Carbs required: type 1.5 and get 2 (ok), but type 1.05 and get 15

Statistics

Hours: can't type 4.05, if you type 4.5 it rounds up to 5, but you can type 66
Low and High: if you type 5.01 you get 51 mmol/L
but then leaving the screen and returning: Hours changed to 24 and low and high changed to 5 and 10 mmol/L
tried again with 6 and 12 - left and returned and got 5 and 12 (Issue #361 refers to this type of behavior)

Preferences

All the entries on this screen allow the user to type in #.0# with no problem, even when it makes no sense
Leave and return and the values typed in are maintained.

Pump Settings

.0# is accepted for all entries and maintained

Meal Settings

some are ok with #.0#
Fat and Protein conversion settings are not ok
- for delay in minutes, type 60.5 and get 605.
- maximum duration, type in 8.5 and get 85.
- interval in minutes, type 30.5 and get 305
- override with a factor, type 1.05 and get 15

These use picker wheels: basal profile, ISF, CR, Target Glucose

Log Insulin

.0# is accepted and saved

Log Glucose (mmol/L)

10.# is accepted
38.9 is accepted
10.05 shows up at 105

Carb Entry

The carb, fat and protein rows convert #.0# to ##
The notes field is fine

Enact Temp Target

Target entry of #.0# becomes ##
Duration entry of 30.05 becomes 305
Temp Target of 55 appears to have been accepted
Cancel the TT shortly after enacting it

Enact Bolus

.0# is accepted

kskandis commented 4 months ago

Meal Settings

some are ok with #.0#

Fat and Protein conversion settings are not ok

for delay in minutes, type 60.5 and get 605.

Thanks, Marion, as always, for your thorough testing. I actually only tested with Bolus Enact Entry, Meal Settings, and Carb Entry, all of which appeared to work for me.

For the Meal Settings - Delay in Minutes/Max Duration in Hours/Interval in Minutes, I believe should NOT accept a decimal symbol, since their View code specifically says it is an intFormater with allowsFloats=false - see my first comment in the corrsponding issue report. So that is how I coded it. Should they allow fractions?

Meal Settings - Override With A Factor Of has formatter with formatter.maximumFractionDigits = 1. To my understanding, this means data entry should allow only 1 digit entered to the right of the decimal separator. Hence, entering '1.05' would not be allowed. Once you enter 1.0, the textfield will drop the 0 and display 1, so if you then enter a 5, textfield will correctly display as 15. Am I misunderstanding the requirements?

I suspect the other fields that you tested have similar issues, esp anything whose View formatters have maximumFractionDigits = 1 (eg., Carb Entry, TT Entry) or allowsFloats=false. What are the expected requirements for each of these fields? Perhaps the Formatters need to be reviewed for each field?

kskandis commented 4 months ago

This fixes the one issue, #362, so I'm adding an approval. As one of the comments, I went through all the data entry fields for others to review.

Please see above comment! I think this PR would apply to all NumberFormat textfields but maybe we need to clarify data entry requirements for each field. Eg., should maxFractionDigits be 1, 2, 3, etc., should decimal entry be allowed. I added code specifically to support the View formatters but the View formatters (for each field) need to be defined appropriately for the field. I did not change any of the formatters which are defined on each field in various files.

kskandis commented 4 months ago

As one of the comments, I went through all the data entry fields for others to review.

I removed the code which addresses your other comments in your review.

You should now be able to enter decimal for the Integer fields in Meals Settings but upon returning to the screen, due to allowFloats=false, the number will revert to an Integer.

marionbarker commented 4 months ago

Summary

Success - thank you Using "." as the decimal separator:

If an entry must be an integer, typing in x.yz becomes x', where x' is rounded up or down depending on 0.yz.
If an entry may only have one place to the right of the decimal, typing in x.yz becomes x.y', where y' is rounded up or down depending on the value of z.
There is one nit (see end of comment) but not sufficient to delay approval
LGTM! As well as PR 352 and 356.

Configuration

Downloaded a fresh patch to get latest commit for PR 362.
started with Trio dev branch, commit 207dd17d

applied all the entry commits current open:

1028  git apply ~/dev/git_local/trio-patches/trio-pr-352_deleting-text-bug.patch
1029  git apply ~/dev/git_local/trio-patches/trio-pr-356-mmol-fix.patch
1030  git apply ~/dev/git_local/trio-patches/trio-pr-362-uitextfield-issue.patch

Tests

Repeated the testing from this comment.

Notifications - works as expected, thank you
Statistics - works as expected, thank you
Preferences - no change (already ok)
Pump Settings - no change (already ok)
Meal Settings - works as expected, thank you
Log Insulin - no change (already ok)
Carb Entry - works as expected, thank you
Enact Temp Target - works as expected, thank you
Enact Bolus - no change (already ok)

One minor nit - when using "." as a decimal separator, when I have ##.# and hit back space, I am presented with ## with no decimal separator. Most people hitting backspace to modify a number will notice, but seems like the decimal separator should not be removed until hitting the keyboard symbol or hitting backspace. This will not prevent me from approving this.

kskandis commented 4 months ago

One minor nit - when using "." as a decimal separator, when I have ##.# and hit back space, I am presented with ## with no decimal separator. Most people hitting backspace to modify a number will notice, but seems like the decimal separator should not be removed until hitting the keyboard symbol or hitting backspace.

Thank you, again, for your thorough testing!! Yes, I agree with this comment. Hwr, it does behave the same as in Loop - Add Carb Entry.

If an entry must be an integer, typing in x.yz becomes x', where x' is rounded up or down depending on 0.yz.

AFAIK, only the Meal Settings (top 3 under Conversion Settings displaying # of Hours or Minutes) contain what appear to be Integers (allowFloats=false).

dnzxy commented 4 months ago

Merging based on rigoros testing by Marion and Mike and 2 approvals (formerly 3, but changes since Mike tested).

nightscout / Trio

resolve incorrect bolus amount due to uitextfield issue #362

352 does include a removal of the groupingseparator for `extension TextFieldWithToolBar.Coordinator: UITextFieldDelegate {`which is also in this PR, so there could be a minor file change conflict, easy to resolve.

352 does not include the primary fix for `extension TextFieldWithToolBar.Coordinator` in this PR though.

356 issues are not addressed in the PR. Files changed would have no conflict.

Summary

Configuration

Testing:

Could not enter #.0# for these fields

.0# is accepted for all entries and maintained

.0# is accepted and saved

.0# is accepted

Summary

Configuration

Tests

nightscout / Trio

resolve incorrect bolus amount due to uitextfield issue #362

352 does include a removal of the groupingseparator for extension TextFieldWithToolBar.Coordinator: UITextFieldDelegate {which is also in this PR, so there could be a minor file change conflict, easy to resolve.

352 does not include the primary fix for extension TextFieldWithToolBar.Coordinator in this PR though.

356 issues are not addressed in the PR. Files changed would have no conflict.

Summary

Configuration

Testing:

Could not enter #.0# for these fields

.0# is accepted for all entries and maintained

.0# is accepted and saved

.0# is accepted

Summary

Configuration

Tests

352 does include a removal of the groupingseparator for `extension TextFieldWithToolBar.Coordinator: UITextFieldDelegate {`which is also in this PR, so there could be a minor file change conflict, easy to resolve.

352 does not include the primary fix for `extension TextFieldWithToolBar.Coordinator` in this PR though.