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

ListBox scroll issue. #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ListBox scroll issue. #268

wants to merge 1 commit into from

Conversation

dp901
Copy link
Contributor

@dp901 dp901 commented Sep 13, 2016

When a listbox contains items that are not of type "ListBoxItem" then scrolling causes item selection. There is an issue that demonstrates that (IssueTests/listbox.fap). The above change fixes the problem but I am not very sure if it is the right way to handle it.

When a listbox contains items that are not of type "ListBoxItem" then scrolling causes item selection. There is an issue that demonstrates that (IssueTests/listbox.fap). The above change fixes the problem but I am not very sure if it is the right way to handle it.
@BSick7
Copy link
Member

BSick7 commented Sep 13, 2016

Good find.
Give me some time to dig through this.

@dp901
Copy link
Contributor Author

dp901 commented Sep 14, 2016

Thanks Brad.

@BSick7
Copy link
Member

BSick7 commented Sep 21, 2016

Looks like you're checking to see if the current item is a UIElement as to whether to reuse a recycled container.

This doesn't feel right.

I would guess that selection problems would be in proximity to the selection code.

I'm open to being proved wrong with tests.

@dp901
Copy link
Contributor Author

dp901 commented Sep 30, 2016

I'm struggling to put together a test that demonstrates the problem. The reason I'm checking if the current item is a UIElement before setting it to a recycled item is because the cache is only supposed to contain UIElement items. My listbox does not contain items of ListBoxItem type and this is only when the error occurs.
The selection code seems to be working as expected, it's like we are telling it to select an item from list when we shouldn't.

@dp901
Copy link
Contributor Author

dp901 commented Sep 30, 2016

A few lines above you are also checking for a UIElement before setting generator.Current to a UIElement.
if (generator.CurrentItem instanceof UIElement)
generator.Current = generator.CurrentItem;
If the cache only contains UIElements I don't understand why the UIElement check is not valid in that case.

@BSick7
Copy link
Member

BSick7 commented Sep 30, 2016

I see where you're headed.

In the following, we are testing to see whether the user's "Item" is already of type UIElement (usually ListBoxItem). If it is, we just use that for the current visual.

                    if (ic.IsItemItsOwnContainer(generator.CurrentItem)) {
                        if (generator.CurrentItem instanceof UIElement)
                            generator.Current = <UIElement>generator.CurrentItem;
                        generator.IsCurrentNew = true;
                    }

The point of the following is to pull a container out of the recycled cache instead of generating a new one. We would want to do this if the current "Item" was not a UIElement. If generator.CurrentItem is a UIElement, the if block would satisfy.

                    } else if (cache.length > 0  && generator.CurrentItem instanceof UIElement) {
                        generator.Current = cache.pop();
                        generator.IsCurrentNew = true;
                    }

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