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]

Tested the functionality:

Bolus:

Enter an amount, tap Clear and Done, observed the amount field being successfully cleared ✅

Carbs, Fat, Protein

For each of the input fields:
Enter an amount, tap Clear and Done, observed the amount field being successfully cleared ✅

Entering something for two or three of carbs, fat or protein: Tapping Clear clears all three entries.

From my testing point of view: LGTM!

Would be good to have someone else take a look at the code changes.

[bjornoleh]

I am actually able to somehow get different results too, with values not being cleared, please let me try to reproduce 👀

Testing again:

Bolus:

• Enter an amount, tap Clear and Done, observed the amount field being successfully cleared ✅

• Enter an amount, tap Done and then mark the entered amount and tap Clear, observed the amount field not being successfully cleared 🚫

Carbs, Fat, Protein

• Enter an amount, tap Clear and Done, observed the amount field being successfully cleared ✅

• Enter an amount, tap Done and then mark the entered amount and tap Clear, observed the amount field not being successfully cleared 🚫

[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?

[bjornoleh]

Looks good now that the cursor for notes is forced to the right too.

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

[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

[bjornoleh]

LGTM! 🚀

[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.

[marv-out]

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

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<Decimal>,
        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)
    }
}

[marv-out]

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.

[marv-out]

I could do this tomorrow 😄