Replies: 33 comments
-
@chingucoding This is essentially the same thing #1645 is trying to solve. The way it was initially designed was to have all the realized containers hookup to the SelectionModel.SelectionChanged and re-evaluate if it was still selected when ever selection changes. We later realized that the container should probably not be doing that and instead just be told if it is selected or not. In order to do that we either need to walk all the realized containers and re-evaluate their state or add more information to the selection changed event in order to be able to find the containers that need to be updated. This is also highlighting one more issue, which is, there is no easy way to iterate over all the realized containers in ItemsRepeater although we have never had the need to do it until now. |
Beta Was this translation helpful? Give feedback.
-
That makes sense. So should I create a proposal for something to we could use to deal with this, e.g. "GetRealizedContainers" or "UpdateRealizedContainer(object item)" as there currently is no elegant way to handle this? |
Beta Was this translation helpful? Give feedback.
-
@anawishnoff as FYI. Lets start a discussion. I think ListView exposes start and end indices of visible and realized containers, but ListView has the restriction of contiguous items. Repeater can support any custom layout, so there is no restriction on contiguousness of indices realized, so perhaps visible containers and realized containers might be more accurate. Another option is to get all realized containers and a different api to check if it is visible on screen or in the buffer area. |
Beta Was this translation helpful? Give feedback.
-
Does it make sense to differentiate between visible and realized containers? If the selection changes, and the previous selected item was realized but is not visible, and we scroll back to it, will it get updated? I don't think that it would be rerendered. If we expose the realized container, would there be a way to force them to rerender themselves e.g. using UpdateLayout? If that's not the case it might make more sense to provide an API along the lines of Something that will be problematic is the performance. After all, if an item changes in size, the layout would need to rerun its arrange step. Would we be able to keep up the scroll position with that? |
Beta Was this translation helpful? Give feedback.
-
We run layout on all the realized containers, so we are already doing all the work on them. The reason for having realized containers outside of the visible area is to have a buffer ready. Our scrolling is independent and layout does not get called for every tick, so if we did not have a buffer we would end seeing black (no element in that place) when you quickly scroll since the scroller is showing an area that does not have any elements laid out yet. For this particular case, visible/released do not make any difference. We will need to update selection state on all realized containers. Ideally selection should not change the size, but if it does, then layout will run again and it should work as you expect. The anchoring feature of scroller will ensure that the element that is visible wont jump around when the sizes change. So perhaps just getting a list of realized containers would suffice. We could later add API to differentiate if something is visible or not if desired. |
Beta Was this translation helpful? Give feedback.
-
@chingucoding @ranjeshj A more robust solution as you two have been discussing would be a GetRealizedContainer property? I like the idea, although I am curious about performance impact. As for re-rendering items, why couldn't you use the UpdateLayout method on each container? (Actually asking - don't know the answer!) Would this have more negative performance impact? Would it be better/possible to update the layout on only changed containers - i.e. containers that have changed their selection state? |
Beta Was this translation helpful? Give feedback.
-
Performance impact is the main problem I see with that. If we already have an existing list of realized containers, exposing it is probably not that bad. However if we have to construct that list, just for that new API, performance impact could become a very critical aspect.
I haven't tried that, so I am not sure what would happen. Would calling Calling UpdateLayout on the Repeater itsself did not work however. Debugging it, I found out that the TemplateSelector got called for every item except the selected one (probably since it had focus).
If the layout does not run the arrange or layout phase, being able to update a single container would probably be the best case scenario. However if the size changes on those containers, we might as well ask the layout to rearrange, since we need to update the layout to prevent clipping or interlacing of items. |
Beta Was this translation helpful? Give feedback.
-
+1. Just exposing the list of realized containers is going to be less expensive for sure compared to exposing indices and getting callbacks trying to get container for each index. It also enables us to support non-contiguous scenarios. Getting index for a container once we have the container is not very expensive. Going from index to container is more expensive (we need to walk through all the containers looking for the one with the specified index) Can you explain why we need to call UpdateLayout ? Ideally we should not have to explicitly do that. |
Beta Was this translation helpful? Give feedback.
-
If we change the SelectedItem of the SelectionModel, the ItemsRepeater has no way that it should rerender as it's not aware of the SelectionModels status. So we need to manually look out for that and update the appropriate items. |
Beta Was this translation helpful? Give feedback.
-
When selection changes SelectionModel will raise SelectionChanged event which the control (that is wiring up selection) needs to listen to. In the SelectionChanged handler, the control can walk through all the realized containers, get the index of each container and check if that container is selected or not in the selection model and update IsSelected on the container. Changing IsSelected should call InvalidateMeasure on the container which will take care of updating the visuals on the container and run layout if necessary. Currently, we have to walk through all the realized containers and update IsSelected unfortunately. If SelectionChanged event gave us the subset of container indices that were removed from selection/added to selection, we can do the operation just for those specific containers (go from index to container). Given that the number of realized containers is relatively small, the initial implementation did not think this was a priority. |
Beta Was this translation helpful? Give feedback.
-
So the go to way to do this would creating a field IsSelected on the container? Or did I misunderstood that? My idea was to have a template for selected/unselected. However calling update on the realized items did not change the visuals accordingly. Is the realized container even aware of the TemplateSelector? In the visual tree, it just shows the items as specified by the template. |
Beta Was this translation helpful? Give feedback.
-
IsSelected property is one way to do it which decouples it from everything else. Lets consider an Itemcontainer control that has an IsSelected property. Setting that property to true/false can change a visual state (Selected or not). Then the template can have the visuals to show that it is selected or not (and can be customized by re-templating). The template selector comes in before all this to create the ItemContainer. When ItemsRepeater wants to create an element for index say 5, it will go through the template selector to create the ItemContainer. Once the container is created and put into the tree, its job is done. The template selector does not create different containers for selected vs not-selected if that is what you are asking. The same container just shows selection visuals (might be check box, or changed background or something). That is the way I would go about doing it. You might have a different idea for how to achieve something similar though. |
Beta Was this translation helpful? Give feedback.
-
Creating a new control for this would work, indeed. However in my opinion, creating a new control just to have different visuals for selected/not selected state seems a bit overkill. Something I would love to do is have different templates because they are very easy to define and are not that much project overhead (new controls have to be somewhere etc). So having the a selection aware TemplateSelector would be a somewhat reasonable way, however since the TemplateSelector needs to be run again after selection changed, this gets very complicated. While VisualStates are also an option, I haven't gotten them to work, and if you have large changes to a selected item, that can become a bit problematic and hard to read. So a separate template would in some cases just be ideal. Now since having the realized containers also wouldn't really solve that, maybe we could provide an API to force a rerender of a container for a given item? So you could do something like this: public OnSelectionChanged(object sender, FictiousSelectionArgs args)
{
// Remove selection indicator from old item:
repeater.UpdateContainerOfItem(args.DeselectedItem);
// Update the template of newly selected item
repeater.UpdateContainerOfItem(args.SelectedItem);
} Currently there is |
Beta Was this translation helpful? Give feedback.
-
That is an interesting way to approach the problem. When you are saying rerender, you essentially mean throwing out the current container and creating a new one from scratch and passing the fact that you need a new container that has specific selection visuals on it. Three concerns if we go that route, (1) if we are removing and replacing the container, it is likely to create a jarring visual experience. (2) if we select all, we end up throwing out everything and replacing everything which can be bad perf wise. (3) ScrollViewer's anchoring is tracking containers, if we completely change them out, we will end up seeing a jump in the visible portion of the screen. Also, what would we do for drag visuals, pointer over, pressed etc, we would have to handle those on both templates now ? I agree Control is a bit heavy weight. Currently there is no prescribed ItemContainer for Repeater, which is why it is quite a bit of work to get all this correct. We've had discussions about creating a Container that does all this or possibly helpers to help someone create their own container. For example NavigationViewItem could be using those helpers. |
Beta Was this translation helpful? Give feedback.
-
Essentially yes that would mean throwing away the container. That would definitely create a possible problem with flickering, agreed. If we select all items, in either way all of them need to change, and if that change would include completely changing the visual tree (because we would completely change the visuals), maybe it could actually be faster to rerender them? The ScrollViewers anchoring is definitely a huge problem here, and there probably isn't a way to work around that except swapping the content out but live the container the same (which then again could create a jarring visual experience).
Wouldn't that already apply to any case where a TemplateSelector is used? Maybe this is something, we could solve with an ItemContainer, like GridViewItem of GridView. That way, anchoring would still work, we could potentially work around the potential of visual glitches when switching and provide useful helpers for every item. But that would of course impact the performance significantly as we would add more items to the visual tree. |
Beta Was this translation helpful? Give feedback.
-
That would also be a very good idea. However what is the benefit compared to an already existing control e.g. Button or ContentControl which already supports a lot of cases? Maybe this is something that the Windows Community Toolkit could help out with? (@michael-hawker FYI) That way, there is an easy way to get started but if people want to get the best performance they can do it now. Should we create a separate issue to further discuss what should and can be covered by such an control? |
Beta Was this translation helpful? Give feedback.
-
Another option might be to think of these as attachable behaviors to any control that is the root of the template. I have not thought through all the details if we went that route to be frank. But that might avoid the necessity to create a control. |
Beta Was this translation helpful? Give feedback.
-
That would also be a suitable path to use, and that could be easier to use for developers. |
Beta Was this translation helpful? Give feedback.
-
I like the idea of having a flexible container that can hold items and handle selection without the above mentioned setbacks (removing entire container, flickering, etc) that can also be reusable to other collection type controls if developers want to implement their own selection visual template as was one of the original point of this proposal (I believe). Although, creating something new like this would be a heavy duty project and would take much longer to implement. I'd like to hear more about creating these attachable behaviors. I'm a little fuzzy on exactly what role they'd play. What would a behavior be? It would be the root element of an item template? |
Beta Was this translation helpful? Give feedback.
-
I've tried to implement parts of those two scenarios (container that handles everything and attached behaviors) in a test project: https://github.com/chingucoding/ItemsRepeaterExperiments I think there is a problem which we can't get around: We need a ContainerHaving a container that we wrap around each item similiar to ListView has some benefits:
However a container has some drawbacks:
Attached behaviorAttached behaviors have one big benefit: They are not costing performance if unused. However they also have their issues:
So there seems to be no good solution to the workflow with TemplateSelectors that need to switch after initial setup? Maybe we could provide a allround container that people can use in their template (as done in my sample project), but that is not necessary to use? Making it the root UIElement of the DataTemplate seems reasonable to do, and if somebody is concerned with the performance, they can implement only what they need themselves. Also currently there are some scenarios which should be easier e.g. getting the ItemsSource item from a given UIElement. Those are also mentioned in the sample project with the "@winui" keyword in the comments of the functions. |
Beta Was this translation helpful? Give feedback.
-
Thanks so much for setting up this prototype and doing this research @chingucoding! This is really detailed and helpful. I've been playing around with the prototype and I have a few clarifying questions from your comment:
When you say uses a Why would we need to force re-render of an item if using these containers? Are there certain scenarios that can't be expressed using visual states? Or is this an issue solely for the TemplateSelector changing the appearance after an event? As for the attached behaviors stuff:
By this do you mean keeping track of selected and deselected items? That goes back to the other issue we have open about this (#2247). Would something like that solve this issue? I'm not sure which is the better solution but the container seems more all-inclusive. Thanks again for setting all this up! |
Beta Was this translation helpful? Give feedback.
-
With
That is solely an issue for the TemplateSelector issue. Something that we could do to circumvent this is to just say that it is not recommended. For other scenarios we don't need a rerender.
I was thinking more about the pattern on how to use the attached behaviors. In the sample project, we create the wrapper for those attached behaviors every time we need some functionality of them. But that seems like a bad idea, instead it would be easier to just create the attached behavior for the items, and then simply be able to reference that, similiar to the container which you can access all the time. For example, if we would introduce an IsSelected flag on an attached behavior and use the fire and forget pattern, that flag would actually be useless as the object that store's the flag gets thrown away. Hope that makes sense. So both methods have it's advantages and disadvantages. Maybe this is something where we just provide some helpers but not a built in solution that people can use to get started easier with the ItemsRepeater or we leave the helpers for that up to the Windows Community Toolkit. |
Beta Was this translation helpful? Give feedback.
-
@chingucoding yeah, this seems like there's potential for using the Behaviors library to help hook this all together more easily. We have helpers in the Toolkit around this and a few Behaviors already. Though I'm not sure how that'd play with the Visual State/Control stuff, not sure if I fully grasp that part of the issue you were facing yet, or if we'd just want to expose a dependency property for selection to let the developer do what they wish (i.e. just bind with a converter for a border or something)? |
Beta Was this translation helpful? Give feedback.
-
The general question is: What would be the easiest for ItemsRepeater users without them having to start from scratch. Having a DP for IsSelected could also work in some cases, but maybe in other cases having a VisualState is better, especially when there are many things one wants to change for selected items. |
Beta Was this translation helpful? Give feedback.
-
@chingucoding, Thank you for creating the prototype. Can you please add this to a prototype folder and create a PR ? We can use the PR to annotate and improve the prototype and discuss options through the PR. Some notes regarding the prototype
I have not answered many of your questions :). Lets start a PR and I can try to answer them as we go. |
Beta Was this translation helpful? Give feedback.
-
Do you want me to create the PR to a new prototype folder inside the WinUI repo?
My idea was that we just switch visual states and the end user can decide what action to take upon visual state switch, otherwise ItemsRepeater is just becoming a GridView with different layouts.
I've talked with @michael-hawker about this yesterday. Since he wanted to explore this too, what would be the best thing to do now? @michael-hawker FYI. |
Beta Was this translation helpful? Give feedback.
-
Yes. Please create a PR to a new feature branch named (feature\ItemContainerPrototype). The PR is just a way to comment on different parts of the code.
Yes. This option takes us closer to how ListViewItem works. You can still change what happens in the visual states by providing a style for the HandleContainer. One advantage I see with this option is ease of use since we can provide a default style/template similar to ListViewItem. |
Beta Was this translation helpful? Give feedback.
-
There currently is no branch "feature\ItemContainerPrototype" and since I do not have write access to the repository, I can't create it. Do you want me to create the PR just inside my fork or towards master branch of this repo?
Is that the direction we want to go? Currently almost any aspect of ItemsRepeater is more like "choose what you want, nothing preconfigured" and providing a Container that has all states and styles already defined seems a different direction than what we did on previous parts of the repeater. That you essentially need to create a new style for the container to change the visuals seems a bit off here, but that's just my opinion here. |
Beta Was this translation helpful? Give feedback.
-
Yes. against master should be fine as well.
Making it easier to consume would involve providing some defaults. If there is a way to completely replace the visuals, then it is probably ok. I agree that the default container would have a lot more than what one would need (similar to ListViewItem), but it could be a quick way to provide behavior/visuals that matches the current ListView for someone who does not want to take the time to write it all. Note that this is still decoupled, so we do not require the usage of this container with Repeater. The style can include only the pieces you need (reducing the element tree). Essentially what you provided inline in your example in the ItemTemplate would be in the style. At this point, I'm just trying to explore the options more to see what issues we might hit down the line. |
Beta Was this translation helpful? Give feedback.
-
I see, yes that makes sense. Created #2284 now with updated handle container. |
Beta Was this translation helpful? Give feedback.
-
Scenario: ItemsRepeater with SelectionModel and a TemplateSelector that chooses the template based on the selected item:
Now when we switch selection in the SelectionModel:
we know the correct selection and that is all fine. However, there is no way we update the previous selection and the new selection without forcing the ItemsRepeater to rerender every item. Is there some hidden function I am missing here, or is this something currently missing in the ItemsRepeater?
Context: I found this while trying to write a sample for the XAML Controls Gallery showcasing on how to use the SelectionModel with the ItemsRepeater control.
The current source code is available here: https://github.com/chingucoding/Xaml-Controls-Gallery/tree/selectionmodel-sample
PS: To force the ItemsRepeater to really rerender everything, I had to use the following workaround since focused items with the StackLayout unfortunately don't get rerendered:
Beta Was this translation helpful? Give feedback.
All reactions