Skip to content
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.

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

Added "Insert" Mode #35

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added "Insert" Mode #35

wants to merge 5 commits into from

Conversation

kav
Copy link
Contributor

@kav kav commented Aug 6, 2014

Moved the responsibility for trimming a partial
match to the autocomplete control.

(this and async are prerequisites for web lookup
addresses which is coming next)

Moved the responsibility for trimming a partial
match to the autocomplete control.

(this and async are prerequisites for web lookup
addresses which is coming next)
datasource = self.autocompleteDataSource;
} else if([DefaultAutocompleteDataSource respondsToSelector:@selector(textFieldShouldReplaceCompletion:)]) {
datasource = DefaultAutocompleteDataSource;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid that this will be a confusing behavior, where essentially the HTAutocompleteTextField can have two data sources. Is there any way that the goal can be accomplished with a single data source?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be noted that having a DefaultAutocompleteDataSource is fairly confusing in and of itself, partially because its unconventional.

@sibljon
Copy link
Contributor

sibljon commented Aug 6, 2014

If I'm reading this correctly, if the data source implements -textFieldShouldReplaceCompletion:, then the data source gets the final word on whether the autocomplete text is committed, right?

If my understand is correct, could you explain how this would be used? You also mentioned that there would be another pull request coming soon, so perhaps it'll be apparent once all of the pieces are assembled.

@kav
Copy link
Contributor Author

kav commented Aug 6, 2014

The data source does get final word. The reason it works this way is now the Autocomplete control's local variable has the access to the complete autocomplete string and the complete user entered string to determine whether it should be completing. Without that I ended up with quite a bit of hoop jumping to make sure the right address got completed in the right spot (they'd drift as the async calls got a bit ahead of themselves).

That said I considered a property on the control which would reduce the amount of code but given it is a datasource concern as it related to the format of the data return by the data source it really does belong there.

I did notice that I missed the "optional" line, that ended up in the async commit so I've added it back.

@sibljon
Copy link
Contributor

sibljon commented Aug 6, 2014

I think my hesitation lies in my believe that the user's acceptance of the autocomplete suggestion should be user-driven. In other words, when the user takes an action that would normally commit or discard the autocomplete suggestion, the text that's on screen should be immediately committed or discarded (regardless of outstanding network requests). Do you agree with this?

In fact, this brings to light some potential problems that arise from adding asynchronous autocompletion (#34) suggestions to begin with.

@kav
Copy link
Contributor Author

kav commented Aug 6, 2014

That's exactly how it works. If the suggestion has not yet been presented it isn't committed at all. Unless I'm missing something this should work exactly the way it did previously but allow the datasource to pass the full completion string back to the control rather than pre-trimming it.

kav added 3 commits August 6, 2014 17:07
Moved the responsibility for trimming a partial
match to the autocomplete control.

(this and async are prerequisites for web lookup
addresses which is coming next)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants