Requesting TreeItem::removeItem

[tiktak]

I took notice of this project a short while ago (starting with 0.6.0 ^.^), and I am really impressed so far. But just now I wanted to implement a nice hierarchy browser using the CEGUI::Tree widget, and I hit a limitation: it seems TreeItem has currently no means of removing child items!

Lamentably, Tree and TreeItem do not have a unified interface with respect to their child items (sadly even for a generalized function using addItem() this also implies writing 2 functions with just different class names for the parameters / or possibly a template).
In any case, Tree has this remove functionality (Tree::removeItem() ) but it works only on TreeItems in the highest level, a call on a child-items will not remove anything (aside from the fact that this required some dirty const-removal casting and getOwnerWindow() abuse to even test this..).

Correct me if I am wrong and there is another, less obvious, way of removing child items! Otherwise, a TreeItem::removeItem(const TreeItem *item) would be really useful and needed for any decent tree implementation

[CrazyEddie]

Hi, welcome, and thanks for raising this issue/request.

Somebody will investigate the possibilities shortly

CE.

[tiktak]

Like dewyatt pointed out, the code was very easy to read and I managed to add the functionality with the following patches (they seem to run fine in my cases (removing children with open child items, etc) but of course I may have missed some test cases).
The removing-all-items function (called resetList() and resetList_impl() in Tree) should be also easy to transfer (but resetList() seems like an imperfect name to me for child items, so I did not touch it )

Functions are transferred from Tree, and i used the rationale comparing addItem() in both variants seemed to suggest.

src/elements/CEGUITreeItem.cpp

Code: Select all

186a187,214
>    void TreeItem::removeItem(const TreeItem* item)
>    {
>       if (item != 0)
>       {
>          Tree *parentWindow = (Tree *)getOwnerWindow();
>          LBItemList::iterator pos = std::find(d_listItems.begin(), d_listItems.end(), item);
>          if (pos != d_listItems.end())
>          {
>             (*pos)->setOwnerWindow(0);
>
>             d_listItems.erase(pos);
>
>             if (item == parentWindow->d_lastSelected)
>             {
>                parentWindow->d_lastSelected = 0;
>             }
>
>             if (item->isAutoDeleted())
>             {
>                delete item;
>             }
>
>             WindowEventArgs args(parentWindow);
>             parentWindow->onListContentsChanged(args);
>          }
>       }
>    }
>


include/elements/CEGUITreeItem.h

Code: Select all

479a480
>    void  removeItem(const TreeItem* item);


EDIT: btw, patches are against the 0.6.1 tree

[CrazyEddie]

Hi,

Thanks for the patches. I'll probably add them over the weekend. I've actually marked the existing Tree as deprecated recently, although it will remain until a replacement is ready

CE.

[tiktak]

Thanks, I think a more consistent Tree implementation would be a real gain.
Another suggestion to make the current one more usable:

Move the const TreeItem* getItemAtPoint(const Point& pt) const from protected to public. It makes sense to export this function, for example to enable item-based popups or item-dependent context menu with the right mouse button (my use-case).

Far from wanting to save a deprecated class - but with those changes you can at least do most things you can do with a normal Gtk/Qt/whatever Tree

include/elements/CEGUITree.h

Code: Select all

491a492,502
>
>
>       /*!
>       \brief
>               Return the TreeItem under the given window local pixel co-ordinate.
>
>       \return
>               TreeItem that is under window pixel co-ordinate \a pt, or NULL if no
>               item is under that position.
>       */
>       TreeItem*       getItemAtPoint(const Point& pt) const;
637,645d647
<       /*!
<       \brief
<               Return the TreeItem under the given window local pixel co-ordinate.
<
<       \return
<               TreeItem that is under window pixel co-ordinate \a pt, or NULL if no
<               item is under that position.
<       */
<       TreeItem*       getItemAtPoint(const Point& pt) const;


[CrazyEddie]

Hi,

Thanks again

I did not get around to committing the patches over the weekend, but will do it now (well, in a few minutes or so ).

CE.

[CrazyEddie]

Ok, I have committed these patches now. The only thing is that the second patch got put into trunk as opposed to branches/v0-6, this is because the change would break the binary interface (at least for some compilers) which I'm trying very hard to avoid on the v0-6 line of code

Btw, if you could make any future patches in unified format, that would be excellent

CE.

[tiktak]

Yeah... way to followup on an old thread - sorry in advance

I made another patch for the "old" (=0.6.1) Tree, which fixes the setItemSelectState(bool) function (in the Trunk, the check whether the item is in the Tree is non-recursive, hence the setItemSelectState fails on non-firstlevel sub-children with an Exception).

This could now be properly used to select a TreeItem programmatically (e.g. search result highlighting, or highlighting an item clicked by mouse button different from the left one).

I think it should not break ABI since I just added a new protected function and changed some actual code. If you reject it for the trunk - I will just use it as my own private patched version

<3 ,
tiktak

P.S. The existing provision that only visible items be considered for selection is still valid after this patch. And, I have not had anyone proofread this yet (SORRY) but it seemed to work great for me.

Code: Select all

--- src/elements/CEGUITree.cpp.orig   2008-09-17 04:56:45.000000000 +0200
+++ src/elements/CEGUITree.cpp   2008-09-17 06:08:53.000000000 +0200
@@ -550,16 +550,38 @@
    *************************************************************************/
    void Tree::setItemSelectState(TreeItem* item, bool state)
    {
-      LBItemList::iterator pos = std::find(d_listItems.begin(), d_listItems.end(), item);
-
-      if (pos != d_listItems.end())
-      {
-         setItemSelectState(std::distance(d_listItems.begin(), pos), state);
+      bool found = containsOpenItemRecursive(d_listItems, item);
+      if (found) {
+         TreeEventArgs args(this);
+         args.treeItem = item;
+         if (state && !d_multiselect) {
+            clearAllSelections_impl();
+         }
+         item->setSelected(state);
+         d_lastSelected = item->isSelected() ? item : 0;
+         onSelectionChanged(args);
+      } else {
+         throw InvalidRequestException((utf8*)"Tree::setItemSelectState - the specified TreeItem is not attached to this Tree or not visible.");
       }
-      else
+   }
+
+   bool Tree::containsOpenItemRecursive(const LBItemList& itemList, TreeItem* item) {
+      size_t itemCount = itemList.size();
+      for (size_t index = 0; index < itemCount; ++index)
       {
-         throw InvalidRequestException((utf8*)"Tree::setItemSelectState - the specified TreeItem is not attached to this Tree.");
+         if (itemList[index] == item) {
+            return true;
+         }
+         if (itemList[index]->getItemCount() > 0)
+         {
+            if (itemList[index]->getIsOpen())
+            {
+               bool found = containsOpenItemRecursive(itemList[index]->getItemList(), item);
+               if (found) return true;
+            }
+         }
       }
+      return false;
    }
 
 
--- include/elements/CEGUITree.h.orig   2008-09-17 04:57:00.000000000 +0200
+++ include/elements/CEGUITree.h   2008-09-17 04:47:40.000000000 +0200
@@ -589,6 +589,13 @@
    /*************************************************************************
       Implementation Functions
    *************************************************************************/
+   
+   /*!
+   \brief
+      Checks if a tree item is visible (searches sub-items)
+   */
+   bool containsOpenItemRecursive(const LBItemList& itemList, TreeItem* item);
+   
    /*!
    \brief
       Add list box specific events


[CrazyEddie]

Hi,

Thanks for the new patch I'll look it over at the weekend with a view to committing it to the v0-6 branch of code. You're right that the changes made here should not affect the ABI.

CE.

[CrazyEddie]

Hi,

I finally applied the latest patch in branches/v0-6 at rev. 1844. It should get merged into trunk in a few days.

Thanks again

CE.