Skip to content

LOOP-1916: Move cursor to the end on begin editing (take 2) #245

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.

Sign up for GitHub

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

Merged
rickpasetto merged 3 commits into from
Sep 21, 2020
Merged

LOOP-1916: Move cursor to the end on begin editing (take 2) #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6c7a23
LOOP-1916: Move cursor to the end on begin editing (take 2)
Sep 18, 2020
c833e8c
Merge branch 'dev' into rick/LOOP-1916-bolus-carb-entry-cursor-2
Sep 21, 2020
76ef484
Merge branch 'dev' into rick/LOOP-1916-bolus-carb-entry-cursor-2
Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 11 additions & 17 deletions Extensions/UITextField.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,20 @@ import UIKit

extension UITextField {

func selectAll(completion: (() -> Void)? = nil) {
DispatchQueue.main.async {
self.selectedTextRange = self.textRange(from: self.beginningOfDocument, to: self.endOfDocument)
completion?()
}
func selectAll() {
dispatchPrecondition(condition: .onQueue(.main))
self.selectedTextRange = self.textRange(from: self.beginningOfDocument, to: self.endOfDocument)
}

func moveCursorToEnd(completion: (() -> Void)? = nil) {
DispatchQueue.main.async {
let newPosition = self.endOfDocument
self.selectedTextRange = self.textRange(from: newPosition, to: newPosition)
completion?()
}
func moveCursorToEnd() {
dispatchPrecondition(condition: .onQueue(.main))
let newPosition = self.endOfDocument
self.selectedTextRange = self.textRange(from: newPosition, to: newPosition)
}

func moveCursorToBeginning(completion: (() -> Void)? = nil) {
DispatchQueue.main.async {
let newPosition = self.beginningOfDocument
self.selectedTextRange = self.textRange(from: newPosition, to: newPosition)
completion?()
}
func moveCursorToBeginning() {
dispatchPrecondition(condition: .onQueue(.main))
let newPosition = self.beginningOfDocument
self.selectedTextRange = self.textRange(from: newPosition, to: newPosition)
}
}
6 changes: 5 additions & 1 deletion LoopKitUI/Views/DismissibleKeyboardTextField.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,11 @@ public struct DismissibleKeyboardTextField: UIViewRepresentable {
}

@objc fileprivate func editingDidBegin(_ textField: UITextField) {
textField.moveCursorToEnd()
// Even though we are likely already on .main, we still need to queue this cursor (selection) change in
// order for it to work
DispatchQueue.main.async {
textField.moveCursorToEnd()
}
}
}
}
5 changes: 4 additions & 1 deletion LoopKitUI/Views/TextFieldTableViewCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,11 @@ public class TextFieldTableViewCell: UITableViewCell, UITextFieldDelegate {
// MARK: - UITextFieldDelegate

public func textFieldDidBeginEditing(_ textField: UITextField) {
textField.moveCursorToEnd { [weak self] in
// Even though we are likely already on .main, we still need to queue this cursor (selection) change in
// order for it to work
DispatchQueue.main.async { [weak self] in
guard let self = self else { return }
self.textField.moveCursorToEnd()
Copy link

@ps2 ps2 Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cell is used a lot of places in Loop, I think, and I haven't done a review to see if we really want this behavior on all of them. Some places, like dexcom code entry, the view is centered and the font is large, and the standard iOS behavior of the cursor landing where you tap is useful and easy to do, and probably expected. The places where we've had problems and really want the cursor to go to the end is in views where 1) making a too big value accidentally by prepending instead of appending is risky, 2) where the input is relatively small, and/or 3) the value is on the right hand side of the screen, making placement of the cursor on the right more difficult.

Copy link

@ps2 ps2 Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should add this behavior under a flag, or do a review of what interfaces this affects.

Copy link
Author

@rickpasetto rickpasetto Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO consistency is better here. I humbly disagree that this behavior should vary between different text field entry UI. That would only make matters more confusing for the user.

(P.S. The Dexcom code entry screen works fine, because it always starts empty.)

Copy link

@ps2 ps2 Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor placement is an iOS idiom; we're breaking convention and already ensuring inconsistency by forcing the cursor to the right. The user is spending their time in a system where our input fields, or at least those we choose to apply this too, will be the odd duck out, and this may be a confusing situation. I think the safety concerns we have seen for certain interfaces warrant this non-standard behavior, but I'm just wondering if we want to scope the damage.

Copy link

@ps2 ps2 Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And as for Dexcom code inputs; it's the same as other fields; it's only when editing a value that the issue arises. You may put in a code, realize it's wrong, want to edit it, but you can't do what you expect, and you end up having to delete and re-enter several valid values to fix the one you want to change.

Copy link
Author

@rickpasetto rickpasetto Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree that we're breaking iOS convention. I've used many apps that vary this behavior, from cursor-on-the-right, to cursor-on-the-left, to "select the whole text" field to edit it. While I haven't done a full assay on apps out there, I can confidently say this is true. So making statements like this is "non-standard" behavior that will cause "damage" is, I believe, overstating things a bit.

This PR is not changing anything. It is only addressing one of the architectural concerns you gave in the previous PR (#243). I recommend we merge this and continue to monitor the situation.

Re: the Dexcom code entry: I don't understand the steps you outline to reproduce. There's no "editing the value" in this case. It always starts empty.

Copy link

@ps2 ps2 Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely a convention. I just tried several apple apps (calendar, apple health, contacts) and different fields, and they all allowed placement of cursor on the right or left, and hover to place somewhere in the middle. I also just tried the dexcom cgm manager code entry, and was able to enter a code, then place the cursor in the middle, delete one digit, and replace it.

I'm not saying that this PR needs to be changed (I did approve it), but I do want to call attention to the fact that the related changes that were landed in the previous PR are addressing the very real problem in some UIs with a broad brush that affects other UIs that we haven't had issues with, and are now causing them to behave in non-standard ways, and yes, if this causes user confusion where we didn't need to, I do think it is damage.

Copy link
Author

@rickpasetto rickpasetto Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still do all of those things with the changes here (and in the prior PR). All I changed is the initial cursor placement. Maybe I don't understand your concern fully.

self.delegate?.textFieldTableViewCellDidBeginEditing(self)
}
}
Expand Down