unoplatform / uno.extensions

Libraries to ease common developer tasks associated with building multi-platform mobile, desktop and web applications using Uno Platform or WinAppSDK.
https://platform.uno/
Other
73 stars 45 forks source link

Bug with Single-Item Selection with ComboBox #2393

Open modula2019 opened 2 months ago

modula2019 commented 2 months ago

Current behavior

I am following the instructions in the documentation below to implement selection in a combobox. I discover that combobox is able to identify item firstly selected and the items above it. The items below the firstly selected item are ignored. I see this as a bug because this is not what it is intended to do.

https://platform.uno/docs/articles/external/uno.extensions/doc/Learn/Mvux/Advanced/Selection.html#implement-selection-in-the-peopleapp

Expected behavior

The expected behaviour should be to identify every selection when it is made.

How to reproduce it (as minimally and precisely as possible)

UnoApp1.zip

A sample repo is attached here to check the behavior. The ComboBoxPage.xaml is the affected page

Environment

Nuget Package (s):

Package Version(s):

Affected platform(s):

Visual Studio:

Relevant plugins:

Anything else we need to know?

None. Everything needed is already supplied

dr1rrb commented 2 months ago

Checked your repro, indeed there is a bug.

It seems that the ComboBox is not un-selecting previous item when selection changes so we end to have more than on item selected and your SelectedPerson will remain stick to the first selected item.

Putting this issue into our backlog.

Workaround In the meantime if you want an **ugly** workaround, you can convert your `IListFeed People` to a `IListState` and then add this in your constructor: ```csharp Task.Run(async () => { var ctx = SourceContext.GetOrCreate(this); await foreach (var msg in ctx.GetOrCreateSource(People).WithCancellation(ctx.Token)) { var previousSelection = msg.Previous.GetSelectionInfo() ?? SelectionInfo.Empty; var currentSelection = msg.Current.GetSelectionInfo(); if (currentSelection is { Count: > 1 }) { foreach (var range in previousSelection.Ranges) { currentSelection = currentSelection.Remove(range); } global::System.Diagnostics.Debug.Assert(currentSelection.Count == 1); await People.UpdateMessageAsync(msg => msg.Selected(currentSelection), ctx.Token); } } }); ``` In short: Each time the `People` state is updated, we will validate that we have more than one item selected, and if so re-update the `People` state to keep only the freshly added item in the selection.
modula2019 commented 2 months ago

Thanks for the reply. Your suggestion works while we wait for permanent solution

vatsashah45 commented 2 months ago

ComboBox doesn't support ISelectionInfo on Windows. The team needs to fix the support of ICollectionView on Uno Platform Apps.

ComboTest.zip

@modula2019 @dr1rrb

dr1rrb commented 2 months ago

Here is more details about the next steps:

  1. We will have to make sure that on Windows, our implementation of ICollectionView in MVUX works properly with the ComboBox (i.e. replace single select item and )
  2. We then we will have to confirm support of both the ICollectionView AND the ISelectionInfo (still in MVUX) for controls likes ListView on windows
  3. Then we will make sure that the ComboBox on Uno is properly align with windows for bothe ISelectionInfo and ICollectionView. Note: If needed we will remove the support of the ISelectionInfo frm the ComboxBox in uno, but if possible we will keep it.
dr1rrb commented 2 months ago

Hey @modula2019, the workaround I provided was an evolution of my investigation to understand what was the issue. It has the advantage to not alter the code of your app and can be just removed once the bug is going to be fixed. But a simpler workaround would be to remove the .Selection(SelectedPerson) from the IListState<Person> People and then add a binding on the ComboBox.SelectedItem={Binding SelectedPerson.Value, Mode=TwoWay}.

Note: The .Value is needed to be able to set an instance of People as for the binding the entity is exposed de-normalized to be able to edit each property of the Person individually, like <TextBox Text="{SelectedPerson.FirstName, Mode=TwoWay}" />.

modula2019 commented 2 months ago

Hey @modula2019, the workaround I provided was an evolution of my investigation to understand what was the issue. It has the advantage to not alter the code of your app and can be just removed once the bug is going to be fixed. But a simpler workaround would be to remove the .Selection(SelectedPerson) from the IListState<Person> People and then add a binding on the ComboBox.SelectedItem={Binding SelectedPerson.Value, Mode=TwoWay}.

Note: The .Value is needed to be able to set an instance of People as for the binding the entity is exposed de-normalized to be able to edit each property of the Person individually, like <TextBox Text="{SelectedPerson.FirstName, Mode=TwoWay}" />.

@dr1rrb Thanks, this makes it much easier. The workaround worked perfectly. Thanks once again

agneszitte commented 3 weeks ago

https://github.com/unoplatform/uno/pull/17582 needs to be done as well to be able to close this issue