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 AutoCompleteBox control #1427

Merged
merged 17 commits into from Apr 8, 2018

Conversation

Projects
None yet
4 participants
@sdoroff
Contributor

sdoroff commented Mar 6, 2018

This was ported over from the Silverlight Toolkit.

A few notes / questions:

  • CancelableEventArgs should probably live in one of the lower level assemblies, I wasn't sure which one.
  • TextBox.GetLine would return a line that didn't exist if the caret was at the very end of text. This would cause the KeyDown event to get marked handled incorrectly.
  • The change to KeyboardDevice.SetFocusedElement allows the newly focused element to be checked in the LostFocus handler. This is necessary for tracking focus within the Popup and seems generally useful.
  • I attempted to implement UseFloatingWatermark but it caused some problems with the Popup obscuring the TextBox. I may try to get the Popup to reposition itself when its PlacementTarget size changes at some point in the future.
  • I'm honestly a little self-conscious about this being the opening page of the ControlCatalog. If there's anything you think would make the Page look nicer, let me know.

sdoroff added some commits Feb 12, 2018

@Tirraon

This comment has been minimized.

Tirraon commented Mar 6, 2018

I have some question about future (probably depended on Xaml Standard future)
UWP has a control with another name AutoSuggestBox.
Should they be similar?
As I know, Xamarin Forms hasn't something like that for compare.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Mar 6, 2018

The CancelableEventArgs class should probably be in the Avalonia.Interactivity project since that's where we define routed events.

@grokys

This comment has been minimized.

Member

grokys commented Mar 7, 2018

Thanks for this @sdoroff. Not had a chance to look at it yet, but what @Tirraon mentions is a good point. We should probably be aiming for something like UWP has rather than Silverlight. How do they compare?

@Tirraon

This comment has been minimized.

Tirraon commented Mar 7, 2018

I noticed, that AutoCompleteBox is more functional than AutoSuggestBox.
UWP control doesn't any completion work - devs should subscribe to events and update suggestion items source manually. Maybe it's no reasons for removing completion from Avalonia implementation.

About AutoSuggestBox members, which don't exist in AutoCompleteBox or named differently:
Properties:
Style TextBoxStyle
bool UpdateTextOnSelect
string TextMemberPath
bool AutoMaximizeSuggestionArea
double MaxSuggestionListHeight
string PlaceholderText // Watermark in Silverlight
IconElement QueryIcon // If setted, AutoSuggestBox will have clickable query button on right corner
LightDismissOverlayMode LightDismissOverlayMode // Configure popup dismiss behavior

Events:
Silverlight: EventHandler<SelectionChangedEventArgs> SelectionChanged;
UWP: TypedEventHandler<AutoSuggestBox, AutoSuggestBoxSuggestionChosenEventArgs> SuggestionChosen;

Silverlight: EventHandler TextChanged;
UWP: TypedEventHandler<AutoSuggestBox, AutoSuggestBoxTextChangedEventArgs> TextChanged;

And UWP has one another event, which "Occurs when the user submits a search query" (at least: Enter pressed or query button clicked)
event TypedEventHandler<AutoSuggestBox, AutoSuggestBoxQuerySubmittedEventArgs> QuerySubmitted;

@grokys

This comment has been minimized.

Member

grokys commented Mar 7, 2018

Would it make sense to build this control on the UWP control's interface? It's been a while since I've needed an autocomplete control, but I remember last time I needed one, I couldn't find one that was flexible enough for my use-case. Perhaps this is why UWP's control is as it is?

@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Mar 7, 2018

@grokys - the Silverlight version supports fully custom population by handling the Populating event. Details are described here.
I've personally found the built in search and completion functionality to be pretty robust and useful whenever dealing with a local dataset. If mirroring the UWP api is desired, it should be possible to rework the code such that it is a super-set of the AutoSuggestionBox

@grokys

This comment has been minimized.

Member

grokys commented Mar 7, 2018

@sdoroff again, sorry that I've not had chance to look into this properly yet, but does it support async queries for getting the list of auto-complete items? You mentioned "dealing with a local dataset", but what about a remote dataset? Could it be used to provide autocomplete for a google search for example?

@grokys

This comment has been minimized.

Member

grokys commented Mar 7, 2018

Ah sorry, I think the link you posted answers that. Answer seems to be "kinda, but in a dated way" to me, or am I mistaken? Any way, I think I need to take a proper look at it - apologies for the drive-by comments.

@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Mar 8, 2018

@grokys - I can't argue with "kinda, but in a dated way". I agree it's an important use case to support more robustly. What would a more modern API look like? The UWP implementation, though somewhat better named, isn't actually any more functional.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Mar 8, 2018

We could make Populating a bindable delegate that returns a Task<IEnumerable<string>> for a list of options. Then people could bind it to properties or methods on their object.

@Tirraon

This comment has been minimized.

Tirraon commented Mar 9, 2018

I forgot about one detail:
AutoSuggestControl is based on ItemsControl. ItemTempate, ItemContainerStyle and other is available for it.

@grokys

This comment has been minimized.

Member

grokys commented Mar 9, 2018

We could make Populating a bindable delegate that returns a Task<IEnumerable> for a list of options. Then people could bind it to properties or methods on their object.

Yeah that's what I was thinking originally, but what if e.g. the data is paged?

@Tirraon

This comment has been minimized.

Tirraon commented Mar 9, 2018

@grokys does Avalonia support something like ISupportIncrementalLoading?

@grokys

This comment has been minimized.

Member

grokys commented Mar 12, 2018

@Tirraon not currently, it's something I would like though.

@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Mar 13, 2018

@grokys - I'm not sure how common of a scenario using a paged data source within an AutoCompleteBox actually is. I suspect most uses would just request the top n result for the current search text.

@jkoritzinsky - I like the bindable delegate idea. Though I had a few questions:

  1. Should it be a named Delegate type or just a Func<string, Task<IEnumerable>>
  2. How should the control handle a text input triggering a new query while a previous Task is still running? Do we attempt to cancel the running task or just discard it and trust the delegate will take care of things if necessary?
  3. Does anyone have some pointers on building a simple loading circle animation? I think it would be a nice indicator of asynchronous loading but I don't have any experience with Avalonia animation.
@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Mar 13, 2018

@sdoroff I have some answers for a few of your questions.

  1. I think it should be Func<string, Task<IEnumerable<object>>> since, at least in my experience, the generic delegate types have a better IDE experience for the user. Additionally, I don't think our method accessor/binding code currently supports non-Action/non-Func delegate types. I have to investigate that though.
  2. I think we should try to cancel the task. So maybe the delegate should take a cancellation token?
@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Mar 14, 2018

I added asynchronous population to the control. I don't have a ton of experience with async/await, so I'd greatly appreciate it if someone could take a look at the code.
The delegate property is currently called AsyncPopulator. I don't love the name and open to other suggestions.

sdoroff added some commits Mar 14, 2018

Made use of existing CancelEventArgs
Uses System.ComponentModel.CancelEventArgs
instead of custom CancelableEventArgs class
@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Mar 19, 2018

I think I might of messed things up a bit by trying to rebase the branch on my local machine, I'm not particularly experienced with git. My apologies.

  • If necessary, is there a simple way to fix it so the pull request isn't cluttered my already merged commits
  • For future reference, what's the correct way to resolve merge conflicts on an already submitted pull request?
Merge remote-tracking branch 'origin/master' into pr/1427-added-autoc…
…ompletebox-control

 Conflicts:
	src/Avalonia.Themes.Default/DefaultTheme.xaml
@grokys

grokys approved these changes Apr 5, 2018

I think this looks good. We can probably leave out the question of whether we want to follow UWP's API here or not for now. I'd like to eventually move these non-core controls into another assembly, or even another repository so we can probably give the user a choice at that point.

@grokys grokys merged commit 4d5d44a into AvaloniaUI:master Apr 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grokys

This comment has been minimized.

Member

grokys commented Apr 8, 2018

Ok, merged! Thanks @sdoroff !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment