Fixed unparenting a layout item from a layout group 34/183134/4
authorDavid Steele <david.steele@samsung.com>
Mon, 2 Jul 2018 15:42:11 +0000 (16:42 +0100)
committerDavid Steele <david.steele@samsung.com>
Wed, 4 Jul 2018 09:52:47 +0000 (10:52 +0100)
Unparenting a layout item from a layout group did not actually remove the
child from the parent, which if a new layout were set on a container, resulted
in the container having 2 separate layouts in it's parent layout.

Changed the Layout Parent/Child interfaces to allow LayoutItem
to remove itself from a LayoutGroup without explicitly referencing it's subclass.

Change-Id: Idf6d7813ab5a440dfbf44a8bbe46df2ca3698d64
Signed-off-by: David Steele <david.steele@samsung.com>
automated-tests/src/dali-toolkit/utc-Dali-Layouting.cpp
dali-toolkit/devel-api/file.list
dali-toolkit/devel-api/layouting/layout-child-impl.h [new file with mode: 0644]
dali-toolkit/devel-api/layouting/layout-group-impl.cpp
dali-toolkit/devel-api/layouting/layout-group-impl.h
dali-toolkit/devel-api/layouting/layout-item-impl.cpp
dali-toolkit/devel-api/layouting/layout-item-impl.h
dali-toolkit/devel-api/layouting/layout-parent-impl.h

index a9712d0..f31b2e6 100644 (file)
@@ -23,6 +23,8 @@
 #include <dali-toolkit/devel-api/controls/control-devel.h>
 #include <dali-toolkit/devel-api/layouting/absolute-layout.h>
 #include <dali-toolkit/devel-api/layouting/linear-layout.h>
+#include <dali-toolkit/devel-api/layouting/layout-item-impl.h>
+#include <dali-toolkit/devel-api/layouting/layout-group-impl.h>
 
 #include <../custom-layout.h>
 
@@ -1448,7 +1450,6 @@ int UtcDaliLayouting_HboxLayout_TargetSize(void)
   END_TEST;
 }
 
-
 int UtcDaliLayouting_RemoveLayout01(void)
 {
   ToolkitTestApplication application;
@@ -1502,3 +1503,138 @@ int UtcDaliLayouting_RemoveLayout01(void)
 
   END_TEST;
 }
+
+int UtcDaliLayouting_LayoutChildren01(void)
+{
+  ToolkitTestApplication application;
+  tet_infoline(" UtcDaliLayouting_LayoutChildren01");
+
+  Stage stage = Stage::GetCurrent();
+
+  auto rootControl = Control::New();
+  auto absoluteLayout = AbsoluteLayout::New();
+  DevelControl::SetLayout( rootControl, absoluteLayout );
+  stage.Add( rootControl );
+
+  auto hbox = Control::New();
+  auto hboxLayout = LinearLayout::New();
+  DevelControl::SetLayout( hbox, hboxLayout );
+  rootControl.Add( hbox );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 1, TEST_LOCATION );
+
+  tet_infoline("Test removal by setting empty layout to child container" );
+  DevelControl::SetLayout( hbox, LayoutItem{} );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 0, TEST_LOCATION );
+
+  auto& hboxImpl = GetImplementation( hboxLayout );
+  Handle empty;
+  DALI_TEST_EQUALS( hboxLayout.GetOwner(), empty, TEST_LOCATION );
+  DALI_TEST_EQUALS( (void*)hboxImpl.GetParent(), (void*)nullptr, TEST_LOCATION );
+
+  END_TEST;
+}
+
+int UtcDaliLayouting_LayoutChildren02(void)
+{
+  ToolkitTestApplication application;
+  tet_infoline(" UtcDaliLayouting_LayoutChildren02");
+
+  Stage stage = Stage::GetCurrent();
+
+  auto rootControl = Control::New();
+  auto absoluteLayout = AbsoluteLayout::New();
+  DevelControl::SetLayout( rootControl, absoluteLayout );
+  stage.Add( rootControl );
+
+  auto hbox = Control::New();
+  auto hboxLayout = LinearLayout::New();
+  DevelControl::SetLayout( hbox, hboxLayout );
+  rootControl.Add( hbox );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 1, TEST_LOCATION );
+
+  tet_infoline("Test removal by removing child actor from parent container" );
+  hbox.Unparent();
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 0, TEST_LOCATION );
+
+  auto& hboxImpl = GetImplementation( hboxLayout );
+  tet_infoline("Test child actor still has hbox layout " );
+  DALI_TEST_EQUALS( (bool)hboxLayout.GetOwner(), true, TEST_LOCATION );
+
+  tet_infoline("Test hbox layout has no parent " );
+  DALI_TEST_EQUALS( (void*)hboxImpl.GetParent(), (void*)nullptr, TEST_LOCATION );
+
+  END_TEST;
+}
+
+int UtcDaliLayouting_LayoutChildren03(void)
+{
+  ToolkitTestApplication application;
+  tet_infoline(" UtcDaliLayouting_LayoutChildren02");
+
+  Stage stage = Stage::GetCurrent();
+
+  auto rootControl = Control::New();
+  auto absoluteLayout = AbsoluteLayout::New();
+  DevelControl::SetLayout( rootControl, absoluteLayout );
+  stage.Add( rootControl );
+
+  auto hbox = Control::New();
+  auto hboxLayout = LinearLayout::New();
+  DevelControl::SetLayout( hbox, hboxLayout );
+  rootControl.Add( hbox );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 1, TEST_LOCATION );
+
+  tet_infoline("Test removal by removing child layout from parent layout" );
+  absoluteLayout.Remove( hboxLayout );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 0, TEST_LOCATION );
+
+  auto& hboxImpl = GetImplementation( hboxLayout );
+
+  tet_infoline("Check child actor has orphaned layout (Moving child keeps old layout)");
+  DALI_TEST_EQUALS( hboxLayout.GetOwner(), hbox, TEST_LOCATION );
+  DALI_TEST_EQUALS( DevelControl::GetLayout(hbox), hboxLayout, TEST_LOCATION );
+
+  tet_infoline("Check orphaned layout has no parent");
+  DALI_TEST_EQUALS( (void*)hboxImpl.GetParent(), (void*)nullptr, TEST_LOCATION );
+
+  END_TEST;
+}
+
+
+int UtcDaliLayouting_LayoutChildren04(void)
+{
+  ToolkitTestApplication application;
+  tet_infoline(" UtcDaliLayouting_LayoutChildren03");
+
+  Stage stage = Stage::GetCurrent();
+
+  auto rootControl = Control::New();
+  auto absoluteLayout = AbsoluteLayout::New();
+  DevelControl::SetLayout( rootControl, absoluteLayout );
+  stage.Add( rootControl );
+
+  auto hbox = Control::New();
+  tet_infoline("Test unparenting by adding child with no layout to parent (should auto-generate LayoutItem) ");
+  auto hboxLayout = LinearLayout::New();
+  rootControl.Add( hbox );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 1, TEST_LOCATION );
+
+  tet_infoline("Then setting a layout on the child container");
+  DevelControl::SetLayout( hbox, hboxLayout );
+
+  DALI_TEST_EQUALS( absoluteLayout.GetChildCount(), 1, TEST_LOCATION );
+
+  auto& hboxImpl = GetImplementation( hboxLayout );
+  auto& absImpl = GetImplementation( absoluteLayout );
+  DALI_TEST_EQUALS( hboxLayout.GetOwner(), Handle(hbox), TEST_LOCATION );
+  DALI_TEST_EQUALS( hboxImpl.GetParent(), (Dali::Toolkit::Internal::LayoutParent*)&absImpl, TEST_LOCATION );
+
+  END_TEST;
+}
index dadce6e..411afe1 100755 (executable)
@@ -85,15 +85,16 @@ devel_api_layouting_header_files = \
   $(devel_api_src_dir)/layouting/child-layout-data.h \
   $(devel_api_src_dir)/layouting/flex-layout.h \
   $(devel_api_src_dir)/layouting/linear-layout.h \
-  $(devel_api_src_dir)/layouting/layout-item.h \
-  $(devel_api_src_dir)/layouting/layout-item-impl.h \
+  $(devel_api_src_dir)/layouting/layout-child-impl.h \
   $(devel_api_src_dir)/layouting/layout-controller.h \
   $(devel_api_src_dir)/layouting/layout-group.h \
   $(devel_api_src_dir)/layouting/layout-group-impl.h \
+  $(devel_api_src_dir)/layouting/layout-item.h \
+  $(devel_api_src_dir)/layouting/layout-item-impl.h \
   $(devel_api_src_dir)/layouting/layout-length.h \
+  $(devel_api_src_dir)/layouting/layout-parent-impl.h \
   $(devel_api_src_dir)/layouting/layout-size.h \
   $(devel_api_src_dir)/layouting/measured-size.h \
-  $(devel_api_src_dir)/layouting/layout-parent-impl.h \
   $(devel_api_src_dir)/layouting/measure-spec.h
 
 devel_api_magnifier_header_files = \
diff --git a/dali-toolkit/devel-api/layouting/layout-child-impl.h b/dali-toolkit/devel-api/layouting/layout-child-impl.h
new file mode 100644 (file)
index 0000000..79cea9c
--- /dev/null
@@ -0,0 +1,61 @@
+#ifndef DALI_TOOLKIT_INTERNAL_LAYOUTING_LAYOUT_CHILD_H
+#define DALI_TOOLKIT_INTERNAL_LAYOUTING_LAYOUT_CHILD_H
+
+/*
+ * Copyright (c) 2018 Samsung Electronics Co., Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <dali-toolkit/public-api/dali-toolkit-common.h>
+
+namespace Dali
+{
+namespace Toolkit
+{
+namespace Internal
+{
+class LayoutParent;
+
+/**
+ * Interface that allows a layout to determine its layout parent.
+ *
+ * This is useful for LayoutItem to determine it's parent, without accessing
+ * via LayoutGroup, which is a subclass of LayoutItem (Super classes shouldn't
+ * know / care about derived classes)
+ */
+class DALI_TOOLKIT_API LayoutChild
+{
+public:
+  /**
+   * Set the parent of this layout.
+   */
+  virtual void SetParent( LayoutParent* parent ) = 0;
+
+  /**
+   * Get the parent of this layout.
+   */
+  virtual LayoutParent* GetParent() = 0;
+
+protected:
+  virtual ~LayoutChild()
+  {
+  }
+};
+
+
+} // namespace Internal
+} // namespace Toolkit
+} // namespace Dali
+
+#endif //DALI_TOOLKIT_INTERNAL_LAYOUTING_LAYOUT_CHILD_H
index d28ec90..f3856de 100644 (file)
@@ -100,7 +100,7 @@ void LayoutGroup::Remove( Toolkit::LayoutGroup::LayoutId childId )
   {
     if( iter->layoutId == childId )
     {
-      OnChildRemove( *iter->child.Get() );
+      RemoveChild( *iter->child.Get() );
       mImpl->mChildren.erase(iter);
       break;
     }
@@ -114,7 +114,7 @@ void LayoutGroup::Remove( LayoutItem& child )
   {
     if( iter->child.Get() == &child )
     {
-      OnChildRemove( *iter->child.Get() );
+      RemoveChild( *iter->child.Get() );
       mImpl->mChildren.erase(iter);
       break;
     }
@@ -126,7 +126,7 @@ void LayoutGroup::RemoveAll()
 {
   for( auto iter = mImpl->mChildren.begin() ; iter != mImpl->mChildren.end() ; )
   {
-    OnChildRemove( *iter->child.Get() );
+    RemoveChild( *iter->child.Get() );
     iter = mImpl->mChildren.erase(iter);
   }
 }
@@ -434,17 +434,12 @@ void LayoutGroup::OnUnparent()
 {
   // Remove children
   RemoveAll();
+}
 
-  // Remove myself from parent
-  LayoutParent* parent = GetParent();
-  if( parent )
-  {
-    LayoutGroupPtr parentGroup( dynamic_cast< LayoutGroup* >( parent ) );
-    if( parentGroup )
-    {
-      parentGroup->Remove( *this );
-    }
-  }
+void LayoutGroup::RemoveChild( LayoutItem& item )
+{
+  item.SetParent( nullptr );
+  OnChildRemove( item );
 }
 
 void LayoutGroup::ChildAddedToOwner( Actor child )
index 90c40fa..36e0312 100644 (file)
@@ -23,6 +23,7 @@
 #include <dali/public-api/signals/connection-tracker.h>
 #include <dali-toolkit/devel-api/layouting/child-layout-data.h>
 #include <dali-toolkit/devel-api/layouting/layout-group.h>
+#include <dali-toolkit/devel-api/layouting/layout-parent-impl.h>
 #include <dali-toolkit/devel-api/layouting/layout-item-impl.h>
 
 namespace Dali
@@ -55,6 +56,7 @@ using LayoutGroupPtr = IntrusivePtr<LayoutGroup>;
  * position and size; it should then call Layout() on the child layout to layout the child and it's hierarchy.
  */
 class DALI_TOOLKIT_API LayoutGroup : public LayoutItem,
+                                     public LayoutParent,
                                      public ConnectionTracker
 {
 public:
@@ -79,19 +81,19 @@ public:
    * @param[in] layoutChild The child to add
    * @return The layout id of this child.
    */
-  Toolkit::LayoutGroup::LayoutId Add( LayoutItem& layoutChild );
+  Toolkit::LayoutGroup::LayoutId Add( LayoutItem& layoutChild ) override;
 
   /**
    * @brief Remove a layout child from this group.
    * @param[in] childId The layout child id
    */
-  void Remove( Toolkit::LayoutGroup::LayoutId childId );
+  void Remove( Toolkit::LayoutGroup::LayoutId childId ) override;
 
   /**
    * @brief Remove a layout child from this group
    * @param[in] child The layout child
    */
-  void Remove( LayoutItem& child );
+  void Remove( LayoutItem& child ) override;
 
   /**
    * @brief Remove all layout children.
@@ -245,6 +247,11 @@ private:
   void OnUnparent() override final;
 
   /**
+   * Method to remove a child from this group
+   */
+  void RemoveChild( LayoutItem& item );
+
+  /**
    * Callback when child is added to owner
    */
   void ChildAddedToOwner( Actor child );
index b5e7720..4c1e6b9 100644 (file)
@@ -20,6 +20,7 @@
 #include <dali/public-api/object/type-registry-helper.h>
 #include <dali-toolkit/public-api/controls/control.h>
 #include <dali-toolkit/devel-api/layouting/layout-item-impl.h>
+#include <dali-toolkit/devel-api/layouting/layout-group-impl.h>
 #include <dali-toolkit/internal/layouting/layout-item-data-impl.h>
 
 namespace
@@ -78,6 +79,16 @@ void LayoutItem::Unparent()
   // Enable directly derived types to first remove children
   OnUnparent();
 
+  // Remove myself from parent
+  LayoutParent* parent = GetParent();
+  if( parent )
+  {
+    parent->Remove( *this );
+  }
+
+  // Remove parent reference
+  SetParent(nullptr);
+
   // Last, clear owner
   mImpl->mOwner = NULL;
 }
index ce5250a..cfbe8ea 100644 (file)
@@ -24,7 +24,7 @@
 #include <dali/public-api/actors/actor-enumerations.h>
 #include <dali-toolkit/devel-api/layouting/child-layout-data.h>
 #include <dali-toolkit/devel-api/layouting/layout-item.h>
-#include <dali-toolkit/devel-api/layouting/layout-parent-impl.h>
+#include <dali-toolkit/devel-api/layouting/layout-child-impl.h>
 #include <dali-toolkit/devel-api/layouting/layout-controller.h>
 #include <dali-toolkit/devel-api/layouting/layout-size.h>
 #include <dali-toolkit/devel-api/layouting/measure-spec.h>
@@ -45,7 +45,7 @@ using LayoutItemPtr = IntrusivePtr<LayoutItem>;
  * Base class for layouts.
  */
 class DALI_TOOLKIT_API LayoutItem : public BaseObject,
-                                    public LayoutParent
+                                    public LayoutChild
 {
 public:
   /**
@@ -94,7 +94,8 @@ public:
   Handle GetOwner() const;
 
   /**
-   * @brief Unparent this layout from it's owner, and remove any layout children in derived types
+   * @brief Unparent this layout from it's parent, remove it from it's owner
+   * and remove any layout children in derived types.
    */
   void Unparent();
 
@@ -158,14 +159,14 @@ public:
   static LayoutLength GetDefaultSize( LayoutLength size, MeasureSpec measureSpec );
 
   /**
-   * @copydoc LayoutParent::SetParent
+   * @copydoc LayoutChild::SetParent
    */
-  virtual void SetParent( LayoutParent* parent ) override;
+  void SetParent( LayoutParent* parent ) override;
 
   /**
-   * @copydoc LayoutParent::GetParent
+   * @copydoc LayoutChild::GetParent
    */
-  virtual LayoutParent* GetParent() override;
+  LayoutParent* GetParent() override;
 
   /**
    * @brief Request that this layout is re-laid out.
index d55fb42..677a8ff 100644 (file)
@@ -25,25 +25,32 @@ namespace Toolkit
 {
 namespace Internal
 {
+class LayoutItem;
 
 /**
- * Interface that allows a layout to determine its layout parent.
- *
- * Needed to prevent circular inheritance - most LayoutBases have a parent,
- * but parenting is provided by LayoutGroup, which is a sub-class of LayoutBase.
+ * Interface that defines a layout Parent. Enables a layout child to access
+ * methods on its parent, e.g. Remove (during unparenting)
  */
 class DALI_TOOLKIT_API LayoutParent
 {
 public:
   /**
-   * Set the parent of this layout.
+   * @brief Add a child to the parent
+   * @param[in] item The item to add to this layout parent
    */
-  virtual void SetParent( LayoutParent* parent ) = 0;
+  virtual Toolkit::LayoutGroup::LayoutId Add( LayoutItem& item ) = 0;
 
   /**
-   * Get the parent of this layout.
+   * @brief Remove a layout child from this group.
+   * @param[in] childId The layout child id
    */
-  virtual LayoutParent* GetParent() = 0;
+  virtual void Remove( Toolkit::LayoutGroup::LayoutId childId ) = 0;
+
+  /**
+   * @brief Remove a child from this parent
+   * @param[in] item The item to remove from this layout parent
+   */
+  virtual void Remove( LayoutItem& item ) = 0;
 
 protected:
   virtual ~LayoutParent()
@@ -52,6 +59,7 @@ protected:
 };
 
 
+
 } // namespace Internal
 } // namespace Toolkit
 } // namespace Dali