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

ListView: OnSelectedChanged is called when it didn't actually change (OnEnter) #2140

Closed
tig opened this issue Oct 29, 2022 · 10 comments · Fixed by #2152
Closed

ListView: OnSelectedChanged is called when it didn't actually change (OnEnter) #2140

tig opened this issue Oct 29, 2022 · 10 comments · Fixed by #2152
Labels

Comments

@tig
Copy link
Collaborator

tig commented Oct 29, 2022

image

Because of this, there's no way for an override to know if the selection REALLY changed.

In UI Catalog, this means that whenever the focus returns to the Category ListView, the Secenario ListView gets regenerated, causing IT to lose what Scenario was previously selected:

00ibvTI 1

I believe the call to OnSelectedChanged in OnEnter should be removed.

@BDisp - what do you think?

@tig tig added the bug label Oct 29, 2022
@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

Actually the item is only selected if the Enter key or double click was used. That why the item is never selected. The reason why the OnSelectedChanged is called in OnEnter is because when a ListView is instantiated the selected item is always 0, even is empty, and no event is fired to subscribers to update some state. I think the best solution is instantiate with the selected item equal to -1, unless it set on the property. In the OnEnter if the selected item is equal to -1 then selected the first item if not empty.

@tig
Copy link
Collaborator Author

tig commented Oct 30, 2022

I do not understand you OR you are confused about how a list box is supposed to work.

Moving up/down in a list box with keyboard should change the current selection.

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

I do not understand you OR you are confused about how a list box is supposed to work.

Moving up/down in a list box with keyboard should change the current selection.

You are right, sorry. I was confused with the OnOpenSelected. But about updating the state through an event on initializing is true and that why it is in the OnEnter. How about saving the current selected item in a variable to restore after regenerated?

@tig
Copy link
Collaborator Author

tig commented Oct 30, 2022

I don't understand, sorry.

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

I don't understand, sorry.

Suppose the last selected item was 3 and is saved to a variable. After the scenario listview is regenerated set the selected item with that variable.

@tznind
Copy link
Collaborator

tznind commented Oct 30, 2022

As I understand it, @tig is saying that OnSelectedChanged event should not be fired in the OnEnter method of ListView:

if (lastSelectedItem == -1) {
EnsuresVisibilitySelectedItem ();
OnSelectedChanged ();
}

@BDisp are you describing a scenario where having this call in OnEnter is important?

To me it makes sense to remove it as system should be reporting a single action (i.e. Enter) and out of the box no selection changes when you Enter the control so firing OnSelectedChanged makes no difference. I agree obviously that if the user hooks Enter and provides a callback that updates the selection that itself would trigger the changed event.

But I do see the conditional of lastSelectedItem == -1... which I don't fully understand (how it relates to Enter / whether or not to fire OnSelectedChanged from the perspective of an API user)

@tig
Copy link
Collaborator Author

tig commented Oct 30, 2022

But I do see the conditional of lastSelectedItem == -1... which I don't fully understand (how it relates to Enter / whether or not to fire OnSelectedChanged from the perspective of an API user)

I don't understand the lastSelectedItem code either. I looked at it hard and just got more confused.

Let's step back to fundamentals and ensure we're on the same page:

  • A ListView with AllowsMultipleSelection == false (the default behavior) can have a single item in Source selected. ListView.SelectedItem sets/gets this index. This item is drawn in such a way that it is apparent to the user that it is selected (HotNormal or Focus depending on whether the ListView has focus or not).
  • It is possible (programmatically; there is no user input way) to make a single-selection ListView have NO item selected. SelectedItem will be -1 in this case. By default, when a ListView is created, Source set, SelectedItem is set to -1. **
  • The OnSelectedChanged override is called whenever SelectedItem changes. It should ONLY be called if the SelectedItem actually changes. That is how it worked when @BDisp added it in commit ID d54f620 (pr List view open item #485).

In PR #1081, @BDisp added EnsuresVisibility... and added the OnSelectedChanged() call to OnEnter. This broke the previous behavior, which I just discovered and am reporting in this issue.

** I believe this (setting SelectedItem to -1 on setting Source is the wrong behavior. I'd prefer it if the first item was selected by default, but it's not a big issue.

@BDisp lastSelectedItem seems mis-named or something. Can you please explain what your intent was in adding it?

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

I don't understand the lastSelectedItem code either. I looked at it hard and just got more confused.

Well, that I know the SelectedItem never was equal to -1 as initial value but equal to 0. The backed field is the selected. I never liked this initial setting but that was how it was. When the ListView get the focus another view that subscribe the SelectedChanged, never get fired because the selected item was 0 and the view doesn't get notified to fill his value. That why I created another variable lastSelectedItem with the initial value equal to -1. If the selected item is 0 and the lastSelectedItem == -1, then the subscriber is notified by the event to fill the values. If is wrong or not depend on how the ListView creator wants the selected item is greater than -1 if it has items. I started get this issue when I created the DynamicMenuBar and the DynamicStatusBar scenarios.

As I understand it, @tig is saying that OnSelectedChanged event should not be fired in the OnEnter method of ListView:

But I agree the subscriber can subscribe the Enter event to get the initial value.

@tig
Copy link
Collaborator Author

tig commented Oct 30, 2022

@BDisp do you know what will break if we fix this bug and change OnEnter to not call OnSelectedChanged?

@BDisp
Copy link
Collaborator

BDisp commented Oct 30, 2022

@BDisp do you know what will break if we fix this bug and change OnEnter to not call OnSelectedChanged?

I really don't know. Maybe the ComboBox too. But if I was you I'll go ahead with also remove the lastSelectedItem and using the selected to be -1 if there are no items and in the SelectedItem only call the OnSelectedChanged if it was changed. But also is needed very careful because the select field is set along the code without call the OnSelectedChanged and must do.
If you fell fine remove it or doing some unit test with a view getting the selected item from ListView when that view gets the focus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants