swingx
  1. swingx
  2. SWINGX-1456

AbstractMutableTreeTableNode insert() logic inserts at the wrong location

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.6.2
    • Fix Version/s: None
    • Component/s: TreeTable
    • Labels:
      None

      Description

      Convenient link to the source

      Logic to insert a child node checks if the node already exists, but it does so like this:

      if (children.contains(child)) {
          children.remove(child);
          index--;
      }
      

      However, decrementing the index is only correct if the old index of the node is lower than the index you are trying to insert at. In the best case, the code above will result in the node being reinserted into the wrong position in the tree. In the worst case, you might have been trying to insert at index 0, and will now get an IndexOutOfBoundsException when you try to insert.

      I find it weird that this class handles the condition of the node already existing as a child anyway... it lead me to discover very confusing bugs when I had assumed that insert() would always insert at the location I specified - so I was firing events out assuming it had worked.

        Activity

        Hide
        kleopatra added a comment -

        agreed, that looks a bit fishy. Not entirely sure why it tries to remove in the first place, core defaultTreeNode does nothing to guarantee no duplicates. Think that the default implementation of treeTableNode should behave as much as core as possible.

        re-assigning to Karl

        Show
        kleopatra added a comment - agreed, that looks a bit fishy. Not entirely sure why it tries to remove in the first place, core defaultTreeNode does nothing to guarantee no duplicates. Think that the default implementation of treeTableNode should behave as much as core as possible. re-assigning to Karl

          People

          • Assignee:
            Karl Schaefer
            Reporter:
            trejkaz
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: