load on demand

[danielklecha]

First of all - control looks great!

I am wondering if you have plans to build on demand feature in this project. I know thiat is complicated thing.

I worry that will be problem when I attach collection with 1 million records. Do you have experience how it will work with your control?

Something similar is available in wpf.lazycombobox but it's the only feature of this control...

IItemGroup interface will complicate on-demand feature but control can pass to "SourceService" additional information with last group and read next package with items.

Best

[cromica]

Thank you @danielklecha. At the moment we have no plans for implementing lazy loading. We do accept any contributions though.

[danielklecha]

@cromica I implemented lazy loading for your control in #10. It's similar to wpf.lazycombobox solution. I hope that you or other contributor) can verify it and approve or give some tips. Thanks!

[safi-07]

@cromica any updates on this?. I think this feature added by @danielklecha should be included.

[safi-07]

@danielklecha I think the names of Services, API can be improved. Them can be ISuggestionProvider for API and Dependency Property can be named as SuggestionProvider. GetMissingItemsAsync can be replace by GetSuggestions.

[danielklecha]

@safi-07 I updated the names. @cromica I did some additional tests with my own project and I didn't find any issues with this feature.

[safi-07]

@danielklecha. I think there is a problem with usability of this box when we select single item mode. It should be more like attached image control of jquery select2. Right now it seems like we are selecting another item. When we are not. Do you think its worth working on?. I think we should make search box a separate TemplatePart. Currently I am trying to make this happen on your code. Should I do it?

[danielklecha]

@safi-07 I think that your idea is better than current solution. If you want to change my code then I don't have contraindications.

[danielklecha]

@safi-07 I though about it second time. I worry that behavior of the control will be inconsicted for the user after change. If you move "search box" to dropdown then for single mode... if you switch between controls then you won't be able to search - unless you expand dropdown and change focus. So maybe it wil generate more problems than advantages... But maybe I misunderstand something in this concept.

I found two different problems in demo:

• when you click inside combo, click tab then focus will be moved to "clear selected items" (PASS), but when you click shift+tab and back to combobox then focus is on "search box" but you can't wrote anything (FAIL)
• when you choose single mode and click "select 20 random items" ten you will see 20 items in combobox (FAIL). But we don't have access to collection change event in IList in library so it could be fixed just in example.

[safi-07]

@danielklecha why are you calling LoadSuggestions(string.Empty); in CloseDropdownMenu(bool clearFilter, bool moveFocus) ?

[danielklecha]

@safi-07 This method has different name previously. I didn't added this call. It was there before my changes.

[safi-07]

@danielklecha I have fixed Tab Navigations and Scrolling issues. I have also added some new useful features. Please pull my latest code

[safi-07]

@danielklecha IsLoading feature that was introduced is not working because Task.Run is not awaited. Please have a look. As per my understanding Task.Run is only used when I/O Bound Task are performed.

[https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-using.html](Task.Run Article) [https://www.pluralsight.com/guides/using-task-run-async-await](Task.Run Article)

[danielklecha]

@safi-07 Are you thinking about code inside "LoadSuggestions" method? Well... My idea was to run async task and leave current thread. And all changes on the end of this task put into function which we send to Tak.Run. Also, we can add "Task.Run(...).ConfigureAwait(false)" Otherwise we need to call Task.Run(...).Wait() but it will break whole concept.

Do you think that we should remove async code and replace it with sync methods? First version of my changes contains sync methods but I thought that is wrong and changed it...

Simplest way to do a fire and forget method in c# 4.0

I need to read more about async but I want to know what is wrong from your point. I remember that "async" suffix should be added.

[safi-07]

@danielklecha if you are going to fetch a few records from million of records you must show some kind of loading record message to user. For that I have already added IsLoadingSuggestions and LoadingSuggestionsContent as dependency property. But as you are doing fire and forget this cant be executed.

IsLoadingSuggestions = true;
            Task.Run(async () =>
            {
                var items = await suggestionProvider.GetSuggestions(criteria, _suggestionProviderToken.Token);
                await Dispatcher.BeginInvoke(new Action(() =>
                {
                    if (suggestionProviderToken.IsCancellationRequested)
                    {
                        IsLoadingSuggestions = false;
                        return;
                    }
                    ItemsSource.Clear();
                    foreach (var item in items)
                        ItemsSource.Add(item);
                    if (!suggestionProviderToken.IsCancellationRequested)
                        ApplyItemsFilter(criteria);
                }));
            });
            IsLoadingSuggestions = false;

[safi-07]

@danielklecha you don't need to use wait or make sync methods. Make methods async and use await with it.

[danielklecha]

@safi-07 I've moved async code to separate method I added IsLoadingSuggestions property as comment - you need to uncomment it. You should be able to this in LoadSuggestions method:

if (SuggestionProvider == null)
{
    ApplyItemsFilter(criteria);
    return;
}
IsLoadingSuggestions = true;
LoadSuggestionsAsync(criteria).ContinueWith(t => IsLoadingSuggestions = false, TaskContinuationOptions.ExecuteSynchronously);

[safi-07]

@danielklecha can you help me with this? [https://stackoverflow.com/questions/58424028/template-part-not-available-as-visual-child-when-custom-control-visibility-is-c]

[danielklecha]

@safi-07 As I wrote on github Dispatcher.BeginInvoke should work.

I think that this check should be added to ItemsSourceProperty, SelectedItemsProperty and SuggestionProviderProperty. I had this problem in #26 - Property is updated before control is loaded. I found workaround but dispatcher will be a better solution. I can update the code if you want.

[safi-07]

Please do so if possible.

[danielklecha]

@safi-07 I updated the code. I checked code again and it looks that we need to secure ItemsSourceProperty and SuggestionProviderProperty. I created control using code behind and everything goes well. Please try it yourself.

[safi-07]

Thanks. Working.

[safi-07]

@danielklecha All Suggestions Provider are giving validation by default. Can you please either include Validation (Data Annotation Or Custom Validation. e.g Required Validation) or accept my Pull Request so that I can do it?.

[danielklecha]

I'm not sure about which part of the code you wrote but sure - I will accept your pull request. Honestly, I've just seen it and there is few conflicts so I need to have a moment. I will try to do that tomorrow.

niedz., 20.10.2019, 21:02 użytkownik safi-07 notifications@github.com napisał:

@danielklecha https://github.com/danielklecha All Suggestions Provider are giving validation by default. Can you please either include Validation (Data Annotation Or Custom Validation. e.g Required Validation) or accept my Pull Request so that I can do it?.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sdl/Multiselect-ComboBox/issues/8?email_source=notifications&email_token=AA5R3PS7K46XF5D4XREOSPLQPSTMPA5CNFSM4HXGRJW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBYRHMQ#issuecomment-544281522, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5R3PQYMDX6OU2MUWEO7ODQPSTMPANCNFSM4HXGRJWQ .

[danielklecha]

I saw that my branch was merged to main so I close this issue.