fix: Accidentally tapping on suffix of a field does not focus input

[aidanlane]

Summary

Fixes the "Accidentally tapping on suffix of a field does not focus input" from issue #660.

Solution

• Extended TextFieldWithToolBar to add optional suffixText (with optional suffixForegroundColor). When this suffixText is tapped, then focus will be sent to the text field.

Test plan

• Build successfully without compilation errors
• Unit tests run successfully
• Verify (1) that tap gestures send focus to text field, (2) input values are persisted, and (3) live changes work:
  • [ ] Calibrations -> Meter glucose - dead code?
  • CarbEntryEditorView -> Carbs
  • CarbEntryEditorView -> Protein
  • CarbEntryEditorView -> Fat
  • DataTable -> New Glucose
  • [ ] ManualTempBasal -> Amount - unsure how to test
  • Treatments -> Carbs
  • Treatments -> Protein
  • Treatments -> Fat
  • Treatments -> Bolus
  • AddMealPresetView -> Carbs
  • AddMealPresetView -> Protein
  • AddMealPresetView -> Fat
• Multi day use in-vivo

Note

This PR addresses the "Accidentally tapping on suffix of a field does not focus input" from issue #660.

[bjornoleh]

Approved based on testing. I could test the same items that you had checked, but not libre calibration or manual temp basal.

No code review on my my part.

[aidanlane]

Is there anyone who would be able to tell me how to test the input fields for libre calibration or manual temp basal please?

Otherwise, would someone be able to test for it for me, as I only have G7 and Omnipods.

Thanks!

[bjornoleh]

I think perhaps we don’t use the input field for manual temp basal any longer? In Trio 0.2, this could be set when in open loop, a dedicated button appeared on the bottom of the screen then. If so, the manual temp basal code should probably be removed (not necessarily in this PR).

Regarding Libre, there used to be a Libre simulator one could choose when setting up Libre as CGM. I think perhaps this has been hidden behind a feature flag now?

[aidanlane]

Thanks @bjornoleh

It indeed looks like manual temp basal is dead code. I was able to remove the files and then there were only a few dead code looking references left that were quick to remove. After that it complied and ran fine. That said, I would suggest another PR and test plan for that, just to keep things clean, test surface minimal, and changes traceable.

I’ll have to dig deeper for the libre simulator.

[aidanlane]

Regarding Libre, there used to be a Libre simulator one could choose when setting up Libre as CGM. I think perhaps this has been hidden behind a feature flag now?

Do you know @kingst? (picking on you as you made the last commit to https://github.com/LoopKit/LibreTransmitter 😉)

[bjornoleh]

There’s some info about hidden simulators behind a debug flag here, but this is actually still a draft PR, so this isn’t merged.
#573 (comment)

[aidanlane]

Oh, that just appears to allow disabling of the main simulators, not the ability to enable the Libre Demo one.

[dnzxy]

Status here?

[aidanlane]

Code complete, working well in the wild.

Unable to perform two exploratory tests from the test plan. Need decision of the importance of them and if needed help with testing them.

[dnzxy]

Calibrations view can be tested by hard-coding the CGM to be of type Libre afaik.
The manual temp basal input is no longer in existence. There used to be a dialog / popover that could be opened from home view's navigation. You can still check that out if you build 0.2.x and do not close the loop IIRC.

[aidanlane]

Thanks @dnzxy.

I had a lot of trouble getting the Libre simulator to work. Are you able to at all?

Manual. I assume there’s no point in testing that for 0.5.1 then?

[aidanlane]

@marionbarker, do you know how I could test the libre calibrations?

[bjornoleh]

I'd say that testing the input field for Libre calibration isn’t really needed. Seems like a low risk, zero consequence thing to merge this as is.

[aidanlane]

Hey @dnzxy, is there anything left blocking this from being merged in?

[dnzxy]

Probably not. I have not tested this at all though.

[dnzxy]

All values for the property unitsText need to be the appropriate String(localized: '...') that are used throughout Trio.

I realize, you only changed / adjusted what was present, but reviewing it again now, this is actually an issue for stuff like "U/hr", because various languagues abbreviate this differently.

Please make this final adjustment, then this is good to go.

[aidanlane]

Hi @dnzxy, could you please review the string localizations please?

Also, have you a preferred approach for the string that set at runtime:
unitsText: state.units.rawValue
https://github.com/nightscout/Trio/pull/712/files#diff-165eb2712d13497fa2093a18e9f0a55f990d2d2e7f0e52c35fd2b48d80450f2eR52

[dnzxy]

I believe there‘s no localized version of that. I‘d go ahead and merge this one now, pending unit tests run.

Thank you.

[aidanlane]

Fantastic, thanks @dnzxy