add observer, notify on ClearButttonTapped to set state value in Deci…

[kskandis]

This PR resolves “Clear” button for bolus and carb entries does not work as expected

273

Observer / notify added to ClearButtonTapped action. On doneButtonTapped action, Observers are removed.

Tested on Simulator and iPhone SE

[bjornoleh]

I updated and tested again. As far as I can tell, the problem still remains when tapping Done before Clear.

I also made another observation, which is unrelated to this PR: When selecting a previously entered value, it is difficult to get the cursor on the right hand side, instead it will typically be at the left of the value.

And when a value is removed by tapping Clear, it is not really cleared, but set to zero. When trying to edit this value, it can easily end up with a trailing zero, meaning ten times the intended amount.

Would it be possible to really clear the amount field, instead of setting to zero?

And when selecting a previously entered value, can we force either a right hand side cursor position (to more easily delete the existing value with backspace), or marking the value so that it is replaced when entering a new value?

[marionbarker]

Going to the way-back machine. Initially on FAX, there was a bug where the cursor wouldn't move to the end of the entered characters. (I think this was a temporary issue with a particular xcode/ios version that no longer exists. We had it in Loop-dev and then we didn't - long time ago.)

My memory says Ivan popped out the Clear button as a quick solution, so you could tap clear and then type in new characters.

[kskandis]

I changed this PR to go to alpha.

[bjornoleh]

@kskandis , I tested your last commit, and now it works exactly as requested 🥳

• selecting a filled out field puts the cursor to the right
• Clear really clears the input instead of entering a zero, and only one field is cleared at each tap
• Tapping Done before Clear works fine

And the correct behaviour is observed for catbs/fat/protein, bolus, TT Target/Duration, Profile (SMB minutes), Manual Temp Basal Amount, Logg insulin/glucose, Preferences, and probably all over.

The only exception so far is the Note field below Carbs/fat/protein. For this field, the cursor is not forced to the right. This field does have Clear / Done buttons, but instead a button to hide the keyboard. Perhaps because this is a text entry field and not a numeric one (text vs keyboard)? It would be nice if cursor position was forced to the right here as well :-)

[kskandis]

This field does have Clear / Done buttons, but instead a button to hide the keyboard. Perhaps because this is a text entry field and not a numeric one (text vs keyboard)?

Yes, the Clear and Done buttons don't exist for Note because it does not use the custom, DecimalTextField. It uses the SwiftUI TextField which does not support cursor positioning. I am using a tiny iPhone SE so I would not use this field with more than a few words. Maybe it would be useful to have the Clear/Done button for the Note field to allow new text to be entered quickly?

[kskandis]

Not sure if it is possible to add Clear/Done either? I’ll accept it as is, but feel free to make changes.

I'll see how it looks w/ the buttons. It make be too crowded w/ the alpha keyboard displayed esp on small phones like iPhone SE.

With commit, it looks ok on SE:

[bjornoleh]

Looks perfect now! Was it a hack to get Clear/Done for the Notes keyboard? Looks good and more consistent anyway.

Perhaps we could do away with the hide keyboard button then? Could you check in the commit history who added that? I think this was done recently. Just to check the reasons why it was added instead of Clear/Done buttons.

[kskandis]

Looks perfect now! Was it a hack to get Clear/Done for the Notes keyboard?

No, not really a hack. Original code was just using a standard SwiftUI textfield one line of code, which did not support any custom code, so I changed it to a custom UITextField to add custom cursor positioning, and the buttons, similar to what the decimal input fields are doing.

Perhaps we could do away with the hide keyboard button then? Could you check in the commit history who added that? I think this was done recently. Just to check the reasons why it was added instead of Clear/Done buttons.

Yes, I agree, keyboard is not needed. I'll remove it. It was added 10 months ago, but I think it was just a merge along with a bunch of other code changes coming from 2.2.3. The keyboard was probably added just because it was less code (just 3 lines) than to implement custom code. The keyboard wasn't actually needed there either since the return key will hide the keyboard, but maybe that's not as obvious as tapping a keyboard.

Here's the Trio github blames:

Here is iAps github blames:

Here it is without the keyboard, which in my opinion is more consistent with the other input fields on this screen.

[bjornoleh]

Thanks, that’s perfect! Do this, and let’s 🚢 it!

🚀

[kskandis]

Thanks, that’s perfect! Do this, and let’s 🚢 it!

Sorry, my response was out-of-sync with your review. Fix is resolved in this commit

[dnzxy]

Tagging @polscm32 to please give this a look and review. We've come across a similar issue and he has fixed this prior, cannot find the specfic commit right now though.

[polscm32]

The approach in the pull request for creating a Decimal Text Field and handling the toolbar is interesting, but in my opinion, it’s too complicated. The use of the Notification Broadcaster for event handling seems a bit excessive and can be simplified. This would make the code simpler and more readable. I have also fixed the problem in my personal repo and can provide the code if desired. I followed the implementation of the DismissibleKeyboardTextField struct in Loop. It’s simpler in my opinion, and button actions are managed directly in the Coordinator without the need for a Notification Broadcaster. The only thing I needed to change was the addition of a clear button, which is actually pretty straightforward (and some refactoring to adapt to our codebase).

[kskandis]

I have also fixed the problem in my personal repo and can provide the code if desired.

Yes, please do!

[dnzxy]

@kskandis I believe this is the implementation @polscm32 is referring to from his private project.

Click to show code

```swift import SwiftUI import UIKit public struct TextFieldWithToolBar: UIViewRepresentable { @Binding var text: Decimal var placeholder: String var textColor: UIColor var textAlignment: NSTextAlignment var keyboardType: UIKeyboardType var autocapitalizationType: UITextAutocapitalizationType var autocorrectionType: UITextAutocorrectionType var shouldBecomeFirstResponder: Bool var maxLength: Int? var isDismissible: Bool var textFieldDidBeginEditing: (() -> Void)? var numberFormatter: NumberFormatter public init( text: Binding, placeholder: String, textColor: UIColor = .label, textAlignment: NSTextAlignment = .right, keyboardType: UIKeyboardType = .decimalPad, autocapitalizationType: UITextAutocapitalizationType = .none, autocorrectionType: UITextAutocorrectionType = .no, shouldBecomeFirstResponder: Bool = false, maxLength: Int? = nil, isDismissible: Bool = true, textFieldDidBeginEditing: (() -> Void)? = nil, numberFormatter: NumberFormatter = NumberFormatter() ) { _text = text self.placeholder = placeholder self.textColor = textColor self.textAlignment = textAlignment self.keyboardType = keyboardType self.autocapitalizationType = autocapitalizationType self.autocorrectionType = autocorrectionType self.shouldBecomeFirstResponder = shouldBecomeFirstResponder self.maxLength = maxLength self.isDismissible = isDismissible self.textFieldDidBeginEditing = textFieldDidBeginEditing self.numberFormatter = numberFormatter self.numberFormatter.numberStyle = .decimal } public func makeUIView(context: Context) -> UITextField { let textField = UITextField() textField.inputAccessoryView = isDismissible ? makeDoneToolbar(for: textField, context: context) : nil textField.addTarget(context.coordinator, action: #selector(Coordinator.textChanged), for: .editingChanged) textField.addTarget(context.coordinator, action: #selector(Coordinator.editingDidBegin), for: .editingDidBegin) textField.delegate = context.coordinator return textField } private func makeDoneToolbar(for textField: UITextField, context: Context) -> UIToolbar { let toolbar = UIToolbar(frame: CGRect(x: 0, y: 0, width: UIScreen.main.bounds.width, height: 50)) let flexibleSpace = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil) let doneButton = UIBarButtonItem( image: UIImage(systemName: "keyboard.chevron.compact.down"), style: .done, target: textField, action: #selector(UITextField.resignFirstResponder) ) let clearButton = UIBarButtonItem( image: UIImage(systemName: "trash"), style: .plain, target: context.coordinator, action: #selector(Coordinator.clearText) ) toolbar.items = [clearButton, flexibleSpace, doneButton] toolbar.sizeToFit() return toolbar } public func updateUIView(_ textField: UITextField, context: Context) { if text != 0 { textField.text = numberFormatter.string(from: text as NSNumber) } else { textField.text = "" } textField.placeholder = placeholder textField.textColor = textColor textField.textAlignment = textAlignment textField.keyboardType = keyboardType textField.autocapitalizationType = autocapitalizationType textField.autocorrectionType = autocorrectionType if shouldBecomeFirstResponder, !context.coordinator.didBecomeFirstResponder { if textField.window != nil, textField.becomeFirstResponder() { context.coordinator.didBecomeFirstResponder = true } } else if !shouldBecomeFirstResponder, context.coordinator.didBecomeFirstResponder { context.coordinator.didBecomeFirstResponder = false } } public func makeCoordinator() -> Coordinator { Coordinator(self, maxLength: maxLength) } public final class Coordinator: NSObject { var parent: TextFieldWithToolBar let maxLength: Int? var didBecomeFirstResponder = false init(_ parent: TextFieldWithToolBar, maxLength: Int?) { self.parent = parent self.maxLength = maxLength } @objc fileprivate func textChanged(_ textField: UITextField) { if let text = textField.text, let value = parent.numberFormatter.number(from: text)?.decimalValue { parent.text = value } else { parent.text = 0 } } @objc fileprivate func clearText() { parent.text = 0 } @objc fileprivate func editingDidBegin(_ textField: UITextField) { DispatchQueue.main.async { textField.moveCursorToEnd() } } } } extension TextFieldWithToolBar.Coordinator: UITextFieldDelegate { public func textField( _ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String ) -> Bool { guard let maxLength = maxLength else { return true } let currentString: NSString = (textField.text ?? "") as NSString let newString: NSString = currentString.replacingCharacters(in: range, with: string) as NSString return newString.length <= maxLength } public func textFieldDidBeginEditing(_: UITextField) { parent.textFieldDidBeginEditing?() } } extension UITextField { func moveCursorToEnd() { dispatchPrecondition(condition: .onQueue(.main)) let newPosition = endOfDocument selectedTextRange = textRange(from: newPosition, to: newPosition) } } extension UIApplication { func endEditing() { sendAction(#selector(UIResponder.resignFirstResponder), to: nil, from: nil, for: nil) } } ```

[polscm32]

Thanks Dan! Side note: I have also rewritten this code entirely in SwiftUI but theres apparently a constraint bug in SwiftUI regarding the toolbar so I've decided to leave it like that although I prefer this over UIKit. The code itself is working though. Its just the console that throws an error 🤷🏻‍♂️

[kskandis]

@kskandis I believe this is the implementation @polscm32 is referring to from his private project.

Click to show code

Looks good. Should we close this PR then, and @polscm32 create a new one with his code? I'm fine with that.

[polscm32]

I could do this tomorrow 😄