swingx
  1. swingx
  2. SWINGX-246

JXTreeTable.TreeTableModelAdapter: Inconsistency firing update for parent row

    Details

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

      Operating System: All
      Platform: All

    • Issuezilla Id:
      246

      Description

      In JXTreeTable.TreeTableModelAdapter.delayedFireTableDataChanged (line 1367 at the time of this
      writing) there is a check to see if the parent node for a path being updated is expended. If the parent is
      expanded, an update event is fired for that row in case its appearance changed due to children being
      updated (issue #82). However, if the parent is expanded only its child nodes are updated. The behavior
      should be consistent,

      Most likely, the correct behavior is that the parent row should be updated in both cases because its
      appearance may have changed even when it's expanded. If this is the case, a fix would be to change the
      code at line 1369 to this:

      int startingRow = tree.getRowForPath(path)+1;
      +++ fireTableRowsUpdated(startingRow,startingRow);
      int min = Integer.MAX_VALUE;

        Activity

        Hide
        kleopatra added a comment -

        I'm inclined to follow the consistency argument as such (always a good thing in
        itself). But would like to see some misbehaviour when not doing so.

        added visual test method to try an see any effect

        • JXTreeTableVisualCheck.interactiveTestInsertNodeAndChangedParentRendering()

        interestingly, the JXTreeTable behaves "better" than the J(X)Tree - the change
        in the parent node after inserting a child is taken up immediately. Hmm ... need
        to dig a bit deeper, any idea what's happening?. Maybe the test setup is crooked?

        Regarding your suggestion - shouldn't be the unconditional rowsUpdated be fired
        on the rowForPath instead of the rowForPath + 1? (as we want to catch any parent
        update vs. the first child).

        Jeanette

        Show
        kleopatra added a comment - I'm inclined to follow the consistency argument as such (always a good thing in itself). But would like to see some misbehaviour when not doing so. added visual test method to try an see any effect JXTreeTableVisualCheck.interactiveTestInsertNodeAndChangedParentRendering() interestingly, the JXTreeTable behaves "better" than the J(X)Tree - the change in the parent node after inserting a child is taken up immediately. Hmm ... need to dig a bit deeper, any idea what's happening?. Maybe the test setup is crooked? Regarding your suggestion - shouldn't be the unconditional rowsUpdated be fired on the rowForPath instead of the rowForPath + 1? (as we want to catch any parent update vs. the first child). Jeanette
        Hide
        reden added a comment -

        Yes, you're right that the unconditional update should be fired on just startingRow. I didn't see the "+1"
        on the previous row.

        I don't have a good test case for this... it's just something I noticed while digging through the code
        looking for other problems (issues 247 & 248). So... take this change with a grain of salt.

        I guess the test case would involve something like a parent node that displays the count of its children.
        If you add a child while the parent is expanded, the parent node shouldn't redisplay (or if it does it
        would only be because the repaint() was sub-optimal and refreshed more than it had been instructed
        to). If you add the child while it's collapsed, it should update.

        Show
        reden added a comment - Yes, you're right that the unconditional update should be fired on just startingRow. I didn't see the "+1" on the previous row. I don't have a good test case for this... it's just something I noticed while digging through the code looking for other problems (issues 247 & 248). So... take this change with a grain of salt. I guess the test case would involve something like a parent node that displays the count of its children. If you add a child while the parent is expanded, the parent node shouldn't redisplay (or if it does it would only be because the repaint() was sub-optimal and refreshed more than it had been instructed to). If you add the child while it's collapsed, it should update.
        Hide
        kleopatra added a comment -

        yeah, the example is along the line I thought as well. But I'm a bit weary about it:

        • Aren't we overreacting when always expecting an update of the parent if
          there's some change in it's children? JTree doesn't (at least not in my test
          setup).
        • if not, how far up the hierarchy do want to go? Driving the example a bit
          further: renderers might decide to show the depth below. Where's the boundary
          between model and renderer responsibility? We are a bit on the borderline,
          strictly speaking, such a scenario is out-of scope for auto-update, the model
          must fire updates for all nodes.
        • how much "better" as a JTree do we want to get? Assuming that JTree's
          behaviour is normative. The parent update if collapsed was introduced because
          the treetable didn't catch the change, whereas the tree did (as far as I
          recall... need to check to be sure)

        Just thinking aloud a bit, needs further consideration.

        Jeanette

        Show
        kleopatra added a comment - yeah, the example is along the line I thought as well. But I'm a bit weary about it: Aren't we overreacting when always expecting an update of the parent if there's some change in it's children? JTree doesn't (at least not in my test setup). if not, how far up the hierarchy do want to go? Driving the example a bit further: renderers might decide to show the depth below. Where's the boundary between model and renderer responsibility? We are a bit on the borderline, strictly speaking, such a scenario is out-of scope for auto-update, the model must fire updates for all nodes. how much "better" as a JTree do we want to get? Assuming that JTree's behaviour is normative. The parent update if collapsed was introduced because the treetable didn't catch the change, whereas the tree did (as far as I recall... need to check to be sure) Just thinking aloud a bit, needs further consideration. Jeanette
        Hide
        reden added a comment -

        Oh, I agree wholeheartedly. IMHO, the extra event when the node is collapsed is the wrong thing to
        do. I just assumed that issue had been discussed thoroughly in the other bug report (since it was
        apparently intentionally put in). The point of my filing this would be that we should be consistent. So, I
        guess there are three options:

        1) Just fire events for the changed nodes. This requires backing out the change for issue #82.
        Advantages: very predictable as to what will happen
        Disadvantages: ensures that we don't fire more events than necessary
        2) Do the same thing whether a node is collapsed or expanded.
        Advantages: programmers can be lazy
        Disadvantages: fires extra events (performance hit). Only fires events one level up (this would need
        to be corrected, I think).
        3) Current behavior: fire extra event when collapsed but not when expanded.
        Advantages: ?? No changes needed?
        Disadvantages: Unpredictable. See disadvantages for #2 when collapsed.

        Show
        reden added a comment - Oh, I agree wholeheartedly. IMHO, the extra event when the node is collapsed is the wrong thing to do. I just assumed that issue had been discussed thoroughly in the other bug report (since it was apparently intentionally put in). The point of my filing this would be that we should be consistent. So, I guess there are three options: 1) Just fire events for the changed nodes. This requires backing out the change for issue #82. Advantages: very predictable as to what will happen Disadvantages: ensures that we don't fire more events than necessary 2) Do the same thing whether a node is collapsed or expanded. Advantages: programmers can be lazy Disadvantages: fires extra events (performance hit). Only fires events one level up (this would need to be corrected, I think). 3) Current behavior: fire extra event when collapsed but not when expanded. Advantages: ?? No changes needed? Disadvantages: Unpredictable. See disadvantages for #2 when collapsed.
        Hide
        kleopatra added a comment -

        there's a

        4) behave exactly like JTree
        Advantages: developer's aren't surprised, because we stick to the norm
        Disadvantages: need to track down how exactly JTree behaves.

        The disadvantage is not so big, just did it: basically, BasicTreeUI.Handler (the
        TreeModelListener part of it) does take into account that the parent might have
        changed due to model events, if it decided that this might be the case, it
        updates the tree (rather crudely, by indirectly revalidating the total).

        • after insert: revalidates if the parent is expanded, or if the parent is
          collapsed and the number of children changed
        • after remove: revalidates if the parent is expanded, or if the parent is
          collapsed and has no children
        • after update: revalidates if the parent is expanded or null

        These revalidates don't make it to the JXTreeTable - because the tree is only
        the renderer - so it has to be solved differently. One option would be a crude
        revalidate the treeTable as well, another would be to fire rowUpdates on the
        parents if the above conditions are met. My preference is the latter: it's in
        analogy to how other event categories (like expansion f.i.) are mapped into
        table vocabulary.

        Jeanette

        Show
        kleopatra added a comment - there's a 4) behave exactly like JTree Advantages: developer's aren't surprised, because we stick to the norm Disadvantages: need to track down how exactly JTree behaves. The disadvantage is not so big, just did it: basically, BasicTreeUI.Handler (the TreeModelListener part of it) does take into account that the parent might have changed due to model events, if it decided that this might be the case, it updates the tree (rather crudely, by indirectly revalidating the total). after insert: revalidates if the parent is expanded, or if the parent is collapsed and the number of children changed after remove: revalidates if the parent is expanded, or if the parent is collapsed and has no children after update: revalidates if the parent is expanded or null These revalidates don't make it to the JXTreeTable - because the tree is only the renderer - so it has to be solved differently. One option would be a crude revalidate the treeTable as well, another would be to fire rowUpdates on the parents if the above conditions are met. My preference is the latter: it's in analogy to how other event categories (like expansion f.i.) are mapped into table vocabulary. Jeanette
        Hide
        reden added a comment -

        I'll defer to you on this one, since I'm sure you know better what should happen. My only concern is the
        number of events fired. In applications such as mine where there are lots of nodes constantly being
        updated, it could greatly increase the number of events if we're also firing a bunch of events for the
        parent nodes.

        Show
        reden added a comment - I'll defer to you on this one, since I'm sure you know better what should happen. My only concern is the number of events fired. In applications such as mine where there are lots of nodes constantly being updated, it could greatly increase the number of events if we're also firing a bunch of events for the parent nodes.
        Hide
        rah003 added a comment -

        Target update.

        Show
        rah003 added a comment - Target update.
        Hide
        kleopatra added a comment -

        TODO (JW): check state, isn't this solved one way or the other?

        Anyway, just filed Issue 727 to solve on a more general basis.

        Show
        kleopatra added a comment - TODO (JW): check state, isn't this solved one way or the other? Anyway, just filed Issue 727 to solve on a more general basis.
        Hide
        kleopatra added a comment -


        same target milestone for all tree-event related issues (postponed)

        Show
        kleopatra added a comment - same target milestone for all tree-event related issues (postponed)

          People

          • Assignee:
            kleopatra
            Reporter:
            reden
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: