ItemsRepeater and SelectionModel fix (#1429)

* Remove item from repeater children if there is no itemtemplate when being recycled. Add SourceIndex to ChildrenRequestedEventArgs to know which object's IndexPath.

* build fix.

* add some comments and ensure that we fail right away if the Source and SourceIndex properties are used outside of the event handler. We do not expect the app to hold on to args since we reuse them anyway.

* update raw ptr to weak ptr

dev/Repeater/APITests/ItemTemplateTests.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See LICENSE in the project root for license information.
 
 using Common;
@@ -70,7 +70,6 @@ public void ValidateRecycling()
                     ItemTemplate = elementFactory,
                 };
 
-
                 var context = (ElementFactoryGetArgs)RepeaterTestHooks.CreateRepeaterElementFactoryGetArgs();
                 context.Parent = repeater;
                 var clearContext = (ElementFactoryRecycleArgs)RepeaterTestHooks.CreateRepeaterElementFactoryRecycleArgs();
@@ -565,6 +564,39 @@ public void ValidateTemplateSwitchingRefreshesElements(Layout layout)
             }
         }
 
+        [TestMethod]
+        public void ValidateNullItemTemplateAndContainerInItems()
+        {
+            RunOnUIThread.Execute(() =>
+            {
+                const int numItems = 10;
+                ItemsRepeater repeater = null;
+                Content = CreateAndInitializeRepeater
+                (
+                    itemsSource: Enumerable.Range(0, numItems).Select(i => new Button() { Content = i }), // ItemsSource is UIElements
+                    elementFactory: null, // No ItemTemplate
+                    layout: new StackLayout(),
+                    repeater: ref repeater
+                );
+
+                Content.UpdateLayout();
+
+                Verify.AreEqual(numItems, VisualTreeHelper.GetChildrenCount(repeater));
+                
+                for (int i = 0; i < numItems; i++)
+                {
+                    var element = (Button)repeater.TryGetElement(i);
+                    Verify.AreEqual(i, element.Content);
+                }
+
+                Button button0 = (Button)repeater.TryGetElement(0);
+                Verify.IsNotNull(button0.Parent);
+
+                repeater.ItemsSource = null;
+                Verify.IsNull(button0.Parent);
+            });
+        }
+
         private ItemsRepeaterScrollHost CreateAndInitializeRepeater(
             object itemsSource,
             Layout layout,

dev/Repeater/APITests/SelectionModelTests.cs

@@ -69,7 +69,7 @@ public void ValidateSelectionChangedEvent()
                 selectionModel.Source = Enumerable.Range(0, 10).ToList();
 
                 int selectionChangedFiredCount = 0;
-                selectionModel.SelectionChanged += delegate (SelectionModel sender, SelectionModelSelectionChangedEventArgs args)
+                selectionModel.SelectionChanged += delegate (SelectionModel sender, SelectionModelSelectionChangedEventArgs args) 
                 {
                     selectionChangedFiredCount++;
                     ValidateSelection(selectionModel, new List<IndexPath>() { Path(4) }, new List<IndexPath>() { Path() });
@@ -127,7 +127,7 @@ public void ValidateOneLevelMultipleSelection()
                         Path(6)
                     },
                     new List<IndexPath>() { Path() });
-                
+
                 SetAnchorIndex(selectionModel, 4);
                 SelectRangeFromAnchor(selectionModel, 5, false /* select */);
                 ValidateSelection(selectionModel,
@@ -280,12 +280,16 @@ private void ValidateNestedMultipleSelection(bool handleChildrenRequested)
             RunOnUIThread.Execute(() =>
             {
                 SelectionModel selectionModel = new SelectionModel();
+                List<IndexPath> sourcePaths = new List<IndexPath>();
+
                 Log.Comment("Setting the source");
                 selectionModel.Source = CreateNestedData(3 /* levels */ , 2 /* groupsAtLevel */, 4 /* countAtLeaf */);
                 if (handleChildrenRequested)
                 {
                     selectionModel.ChildrenRequested += (SelectionModel sender, SelectionModelChildrenRequestedEventArgs args) =>
                     {
+                        Log.Comment("ChildrenRequestedIndexPath:" + args.SourceIndex);
+                        sourcePaths.Add(args.SourceIndex);
                         args.Children = args.Source is IEnumerable ? args.Source : null;
                     };
                 }
@@ -304,6 +308,36 @@ private void ValidateNestedMultipleSelection(bool handleChildrenRequested)
 
                 var endPath = Path(1, 1, 1, 0);
                 SelectRangeFromAnchor(selectionModel, endPath, true /* select */);
+
+                if (handleChildrenRequested)
+                {
+                    // Validate SourceIndices.
+                    var expectedSourceIndices = new List<IndexPath>()
+                    {
+                        Path(),
+                        Path(1),
+                        Path(1, 0),
+                        Path(1),
+                        Path(1, 0, 1),
+                        Path(1, 0, 1),
+                        Path(1, 0, 1),
+                        Path(1, 0, 1),
+                        Path(1, 1),
+                        Path(1, 1),
+                        Path(1, 1, 0),
+                        Path(1, 1, 0),
+                        Path(1, 1, 0),
+                        Path(1, 1, 0),
+                        Path(1, 1, 1)
+                    };
+
+                    Verify.AreEqual(expectedSourceIndices.Count, sourcePaths.Count);
+                    for (int i = 0; i < expectedSourceIndices.Count; i++)
+                    {
+                        Verify.IsTrue(AreEqual(expectedSourceIndices[i], sourcePaths[i]));
+                    }
+                }
+
                 ValidateSelection(selectionModel,
                     new List<IndexPath>()
                     {
@@ -1080,6 +1114,24 @@ private object GetData(SelectionModel selectionModel, IndexPath indexPath)
             return data;
         }
 
+        private bool AreEqual(IndexPath a, IndexPath b)
+        {
+            if (a.GetSize() != b.GetSize())
+            {
+                return false;
+            }
+
+            for (int i = 0; i < a.GetSize(); i++)
+            {
+                if (a.GetAt(i) != b.GetAt(i))
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
         private List<IndexPath> GetIndexPathsInSource(object source)
         {
             List<IndexPath> paths = new List<IndexPath>();

dev/Repeater/ItemsRepeater.idl

@@ -550,6 +550,7 @@ runtimeclass SelectionModelSelectionChangedEventArgs
 runtimeclass SelectionModelChildrenRequestedEventArgs
 {
     Object Source { get; };
+    IndexPath SourceIndex { get; };
     Object Children { get; set; };
 }
 

dev/Repeater/SelectionModel.cpp

@@ -517,23 +517,26 @@ void SelectionModel::OnSelectionInvalidatedDueToCollectionChange()
     OnSelectionChanged();
 }
 
-winrt::IInspectable SelectionModel::ResolvePath(const winrt::IInspectable& data)
+winrt::IInspectable SelectionModel::ResolvePath(const winrt::IInspectable& data, const std::weak_ptr<SelectionNode>& sourceNode)
 {
     winrt::IInspectable resolved = nullptr;
     // Raise ChildrenRequested event if there is a handler
     if (m_getChildrenEventSource)
     {
         if (!m_childrenRequestedEventArgs)
         {
-            m_childrenRequestedEventArgs = tracker_ref<winrt::SelectionModelChildrenRequestedEventArgs>(this, winrt::make<SelectionModelChildrenRequestedEventArgs>(data));
+            m_childrenRequestedEventArgs = tracker_ref<winrt::SelectionModelChildrenRequestedEventArgs>(this, winrt::make<SelectionModelChildrenRequestedEventArgs>(data, sourceNode));
         }
         else
         {
-            winrt::get_self<SelectionModelChildrenRequestedEventArgs>(m_childrenRequestedEventArgs.get())->Initialize(data);
+            winrt::get_self<SelectionModelChildrenRequestedEventArgs>(m_childrenRequestedEventArgs.get())->Initialize(data, sourceNode);
         }
 
         m_getChildrenEventSource(*this, m_childrenRequestedEventArgs.get());
         resolved = m_childrenRequestedEventArgs.get().Children();
+
+        // Clear out the values in the args so that it cannot be used after the event handler call.
+        winrt::get_self<SelectionModelChildrenRequestedEventArgs>(m_childrenRequestedEventArgs.get())->Initialize(nullptr, std::weak_ptr<SelectionNode>() /* empty weakptr */);
     }
     else
     {

dev/Repeater/SelectionModel.h

@@ -88,7 +88,7 @@ class SelectionModel :
     void OnPropertyChanged(winrt::hstring const& propertyName);
 #pragma endregion
 
-    winrt::IInspectable ResolvePath(const winrt::IInspectable& data);
+    winrt::IInspectable ResolvePath(const winrt::IInspectable& data, const std::weak_ptr<SelectionNode>& sourceNode);
     void OnSelectionInvalidatedDueToCollectionChange();
     std::shared_ptr<SelectionNode> SharedLeafNode() { return m_leafNode; }
 
@@ -120,4 +120,4 @@ class SelectionModel :
 
     // use just one instance of a leaf node to avoid creating a bunch of these.
     std::shared_ptr<SelectionNode> m_leafNode;
-};
+};

dev/Repeater/SelectionModelChildrenRequestedEventArgs.cpp

@@ -4,20 +4,38 @@
 #include "pch.h"
 #include "common.h"
 #include "ItemsRepeater.common.h"
+#include "SelectionNode.h"
 #include "SelectionModelChildrenRequestedEventArgs.h"
 
-SelectionModelChildrenRequestedEventArgs::SelectionModelChildrenRequestedEventArgs(const winrt::IInspectable& source)
+SelectionModelChildrenRequestedEventArgs::SelectionModelChildrenRequestedEventArgs(const winrt::IInspectable& source, const std::weak_ptr<SelectionNode>& sourceNode)
 {
-    Initialize(source);
+    Initialize(source, sourceNode);
 }
 
 #pragma region ISelectionModelChildrenRequestedEventArgs
 
 winrt::IInspectable SelectionModelChildrenRequestedEventArgs::Source()
 {
+    auto node = m_sourceNode.lock();
+    if (!node)
+    {
+        throw winrt::hresult_error(E_FAIL, L"Source can only be accesed in the ChildrenRequested event handler.");
+    }
+
     return m_source.get();
 }
 
+winrt::IndexPath SelectionModelChildrenRequestedEventArgs::SourceIndex()
+{
+    auto node = m_sourceNode.lock();
+    if (!node)
+    {
+        throw winrt::hresult_error(E_FAIL, L"SourceIndex can only be accesed in the ChildrenRequested event handler.");
+    }
+
+    return node->IndexPath();
+}
+
 winrt::IInspectable SelectionModelChildrenRequestedEventArgs::Children()
 {
     return m_children.get();
@@ -30,8 +48,9 @@ void SelectionModelChildrenRequestedEventArgs::Children(winrt::IInspectable cons
 
 #pragma endregion
 
-void SelectionModelChildrenRequestedEventArgs::Initialize(const winrt::IInspectable& source)
+void SelectionModelChildrenRequestedEventArgs::Initialize(const winrt::IInspectable& source, const std::weak_ptr<SelectionNode>& sourceNode)
 {
     m_source.set(source);
+    m_sourceNode = sourceNode;
     m_children.set(nullptr);
 }

dev/Repeater/SelectionModelChildrenRequestedEventArgs.h

@@ -9,17 +9,19 @@ class SelectionModelChildrenRequestedEventArgs :
     public ReferenceTracker<SelectionModelChildrenRequestedEventArgs, winrt::implementation::SelectionModelChildrenRequestedEventArgsT, winrt::composable, winrt::composing>
 {
 public:
-    SelectionModelChildrenRequestedEventArgs(const winrt::IInspectable& data);
+    SelectionModelChildrenRequestedEventArgs(const winrt::IInspectable& data, const std::weak_ptr<SelectionNode>& sourceNode);
 
 #pragma region ISelectionModelChildrenRequestedEventArgs
     winrt::IInspectable Source();
+    winrt::IndexPath SourceIndex();
     winrt::IInspectable Children();
     void Children(winrt::IInspectable const& value);
 #pragma endregion
 
-    void Initialize(const winrt::IInspectable& source);
+    void Initialize(const winrt::IInspectable& source, const std::weak_ptr<SelectionNode>& sourceNode);
 
 private:
     tracker_ref<winrt::IInspectable> m_source{ this };
     tracker_ref<winrt::IInspectable> m_children{ this };
-};
+    std::weak_ptr<SelectionNode> m_sourceNode;
+};

dev/Repeater/SelectionNode.cpp

@@ -6,6 +6,7 @@
 #include "ItemsRepeater.common.h"
 #include "SelectionNode.h"
 #include "SelectionModel.h"
+#include "IndexPath.h"
 
 SelectionNode::SelectionNode(SelectionModel* manager, SelectionNode* parent) :
     m_manager(manager), m_parent(parent), m_source(manager), m_dataSource(manager)
@@ -77,6 +78,26 @@ void SelectionNode::AnchorIndex(int value)
     m_anchorIndex = value;
 }
 
+winrt::IndexPath SelectionNode::IndexPath()
+{
+    std::vector<int> path;
+    auto parent = m_parent;
+    auto child = this;
+    while (parent != nullptr)
+    {
+        auto childNodes = parent->m_childrenNodes;
+        auto it = std::find_if(childNodes.cbegin(), childNodes.cend(), [&child](const auto& item) {return item.get() == child;});
+        const auto index = static_cast<int>(distance(childNodes.cbegin(), it));
+        assert(index >= 0);
+        // we are walking up to the parent, so the path will be backwards
+        path.insert(path.begin(), index);
+        child = parent;
+        parent = parent->m_parent;
+    }
+
+    return winrt::make<::IndexPath>(path);
+}
+
 // For a genuine tree view, we dont know which node is leaf until we 
 // actually walk to it, so currently the tree builds up to the leaf. I don't 
 // create a bunch of leaf node instances - instead i use the same instance m_leafNode to avoid 
@@ -105,7 +126,7 @@ std::shared_ptr<SelectionNode> SelectionNode::GetAt(int index, bool realizeChild
             auto childData = m_dataSource.get().GetAt(index);
             if (childData != nullptr)
             {
-                auto resolvedChild = m_manager->ResolvePath(childData);
+                auto resolvedChild = m_manager->ResolvePath(childData, weak_from_this());
                 if (resolvedChild != nullptr)
                 {
                     child = std::make_shared<SelectionNode>(m_manager, this /* parent */);

dev/Repeater/SelectionNode.h

@@ -17,7 +17,7 @@ enum SelectionState
 // a nested scenario. This would map to one ItemsSourceView/Collection. This node reacts
 // to collection changes and keeps the selected indices up to date.
 // This can either be a leaf node or a non leaf node.
-class SelectionNode final
+class SelectionNode final: public std::enable_shared_from_this<SelectionNode>
 {
 public:
     SelectionNode(SelectionModel* manager, SelectionNode* parent);
@@ -48,6 +48,7 @@ class SelectionNode final
     bool SelectRange(const IndexRange& range, bool select);
     SelectionState EvaluateIsSelectedBasedOnChildrenNodes();
     static winrt::IReference<bool> ConvertToNullableBool(SelectionState isSelected);
+    winrt::IndexPath IndexPath();
 
 private:
     void HookupCollectionChangedHandler();

dev/Repeater/ViewManager.cpp

@@ -118,6 +118,19 @@ void ViewManager::ClearElementToElementFactory(const winrt::UIElement& element)
         context.Element(nullptr);
         context.Parent(nullptr);
     }
+    else
+    {
+        // No ItemTemplate to recycle to, remove the element from the children collection.
+        auto children = m_owner->Children();
+        unsigned int childIndex = 0;
+        bool found = children.IndexOf(element, childIndex);
+        if (!found)
+        {
+            throw winrt::hresult_error(E_FAIL, L"ItemsRepeater's child not found in its Children collection.");
+        }
+
+        children.RemoveAt(childIndex);
+    }
 
     auto virtInfo = ItemsRepeater::GetVirtualizationInfo(element);    
     virtInfo->MoveOwnershipToElementFactory();