FIX: fire a table model event even if there are no children yet.

[nishihatapalmer]

• The early return of null was added in version 8.2 of the component.
• If we return early from this method, when a node is expanded in a sorted outline component, the children of the node appear in random positions underneath other folders.
• Re-sorting the tree makes the problem go away, so the tree model is fine, but the table model had not updated properly on the tree node expansion.
• Firing the event forces the table model to update properly on node expansion.

The reason this doesn't happen on node expansion in an unsorted tree is that the nodes are always in the order of the table model, so by luck they appear in the right positions in the view. When an outline is sorted, the positions in the view no longer match the order of nodes in the table model, so they display incorrectly.

See: https://issues.apache.org/jira/browse/NETBEANS-3401 and https://github.com/digital-preservation/droid/issues/306 for more details of the bug and this fix.

[nishihatapalmer]

Should be clearer, sorry. This affects the org.netbeans.swing.outline component, which renders a tree table, and translates between tree model and table model events.

[geertjanw]

Any risks or side effects of this, i.e., any idea why this was introduced at all and what removing it might cause?

[junichi11]

@nishihatapalmer Please read this once: https://cwiki.apache.org/confluence/display/NETBEANS/Submitting+Pull+Request+on+Apache+NetBeans

Should add issue number([NETBEANS-3401]) to the PR and commit message titles.

Did you run unit tests? Is it possible to add unit tests? Thanks.

[nishihatapalmer]

I can't see how it's possible to unit test this - the behaviour is exhibited in the UI display rendering only, and the data that controls it is private. I've had to run the DROID UI and manually observe the results each time.

[nishihatapalmer]

From examining the mercurial netbeans repo (which has more history than the github one), this was explicitly introduced in the mercurial commit:

279504:fa614fb07028bfdb4e60e648e5fbff258f2a623a mentlicher, #247557: Do not generate any event on expand/collapse of an empty folder.

So this appears to have been introduced as an optimisation, on the grounds that if something empty is expanding or collapsing we don't have to fire an event. Unfortunately, this isn't always true - we can have nodes expanding which don't yet have the child nodes.

This may not be true for how it's used in netbeans, but the component in DROID loads child nodes dynamically from a database on expansion events (we register our own expansion listener). So it's perfectly possible to trigger an expansion event on a node which (currently) doesn't have any children.

[junichi11]

I don't know about this issue, but it seems that there are unit tests. So I just asked :) Thanks.

[nishihatapalmer]

No problem, it's a good question :) There's only one test for the whole outline component defined currently (checkboxes checked and selected properly).

[nishihatapalmer]

Oops, pressed the wrong button there - reopening the PR.

[nishihatapalmer]

Thinking about it, there might be a way to unit test this.

Maybe if I can set up a sorted tree properly, expand a node with a dynamic expansion listener (as DROID does it). Then the test could observe the values of nodes at different rows and columns using getValueAt(row, column). It's possible that this will then reveal a child node in the wrong row.

[nishihatapalmer]

The other consideration is whether this causes different problems for trees that genuinely don't have any children to expand when the event is fired.

If that's the case, there may be wider design issue, as we allow registering TreeWillExpand listeners, which can dynamically create child nodes on notification. This is how DROID expands nodes on hearing a node expansion event has occurred:

@Override
    public void treeWillExpand(TreeExpansionEvent event) {
        try {
            profileForm.setCursor(Cursor.getPredefinedCursor(Cursor.WAIT_CURSOR));
            DefaultMutableTreeNode expandingNode = (DefaultMutableTreeNode) event.getPath().getLastPathComponent();
            ProfileResourceNode prn = (ProfileResourceNode) expandingNode.getUserObject();
            profileForm.getInMemoryNodes().put(prn.getId(), expandingNode);
            expandingNode.removeAllChildren();

            final List<ProfileResourceNode> childNodes = 
                profileManager.findProfileResourceNodeAndImmediateChildren(
                        profileForm.getProfile().getUuid(), prn.getId());
            if (!childNodes.isEmpty()) {
                expandingNode.setAllowsChildren(true);
                for (ProfileResourceNode node : childNodes) {
                    DefaultMutableTreeNode newNode = new DefaultMutableTreeNode(node, node.allowsChildren());
                    expandingNode.add(newNode);
                    profileForm.getInMemoryNodes().put(node.getId(), newNode);
                }
            }

            if (expandingNode.getChildCount() == 0) {
                expandingNode.setAllowsChildren(false);
            }

            profileForm.getTreeModel().nodeStructureChanged(expandingNode);
        } finally {
            profileForm.setCursor(Cursor.getDefaultCursor());
        }
    }

Of course, if this isn't the correct way to add dynamic nodes to a tree, then maybe we can change our DROID implementation to dynamically insert children by another route - but being notified that a node wants to expand seems the natural place to do this.

[nishihatapalmer]

I noticed in the DROID code above, it fires a node structure changed event after building the new nodes inside the TreeWillExpand event. Wasn't sure if this might cause weird event ordering issues.

It turns out it makes no difference. Running DROID without calling nodeStructureChanged() in this method still builds the tree correctly and refreshs the UI - but the bug is still there with this line removed in DROID.

[nishihatapalmer]

I'm not very experienced with ant... Is there a way to build and test just the swing-outline component and any dependencies it has?

[nishihatapalmer]

I have written a Junit test that shows the issue.

I haven't figured out how to run it with your harness or integrate it with the rest of the ant build.

Shall I add this unit test to the existing PR ?

[junichi11]

@timboudreau Could you please have a look at this if possible?

[nishihatapalmer]

OK, I've figured out how to write a unit test using NbTestCase. It seems to compile on an ant build. However - I'm not sure the test is running - the build still succeeds with the test.

If I run the test standalone, as just a Junit4 test, it gives the expected failure for this issue, but passes when run against versions earlier than 8.2.

[nishihatapalmer]

Ah - can see that the PR fails on the Travis build, but not sure for the right reasons. Someone more experienced than me with netbeans builds needs to look at that.

[nishihatapalmer]

I have come up with a different fix for this issue, if we want to keep the early return if there are no children.

Essentially, the EventBroadcaster listener assumes that the state of the tree is known at the point it runs. So it sees no children and assumes the table model doesn't need updating. Then the next DynamicNodeListener executes and adds some children, but ends up using the wrong layout as the underlying model didn't update.

An alternative fix is to ensure that the EventBroadcaster listener is always the last listener to run. Any previous listeners which update the tree state will have already executed by that point. This can be achieved by modifying TreePathSupport as follows:

    /**
     * Add a TreeWillExpandListener.
     * @param l The tree will expand listener
     */
    public synchronized void addTreeWillExpandListener (TreeWillExpandListener l) {
        weListeners.add(0, l); // insert new listeners at the start.  This ensures the outline listener is always the last listener.
    }

[nishihatapalmer]

Thinking about this issue a little more, the whole point of the EventBroadcaster is to translate tree events into table events.

If we allow any tree modification during tree events, then the EventBroadcaster should always be the last listener to run for all tree events (not the just the one raised in this issue).

The fix above which inserts new listeners at the start of the list works, but it isn't obvious why this needs to be done. It might be better to refactor the code so that the EventBroadcaster is explicitly the last listener to run - so it has a special status, and isn't part of the main list of other registered listeners.

I think this would make the intention of the code clearer.

[timboudreau]

As the author of that code, sounds good to me. Listener ordering bugs are painful to debug (I once spent a couple of days chasing down a bug that only occurred when the look and feel had been changed without a restart, because UI delegate listeners wind up called before instead of after application listeners), so avoiding them is a fine thing.

-Tim

On Fri, Nov 29, 2019 at 4:28 PM nishihatapalmer notifications@github.com wrote:

Thinking about this issue a little more, the whole point of the EventBroadcaster is to translate tree events into table events.

If we allow any tree modification during tree events, then the EventBroadcaster should always be the last listener to run for all tree events (not the just the one raised in this issue).

The fix above which inserts new listeners at the start of the list works, but it isn't obvious why this needs to be done. It might be better to refactor the code so that the EventBroadcaster is explicitly the last listener to run - so it has a special status, and isn't part of the main list of other registered listeners.

I think this would make the intention of the code clearer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/netbeans/pull/1639?email_source=notifications&email_token=AAOQE5Z64MEIUGRQB7NPKODQWGCQXA5CNFSM4JOJLZVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFPSGVA#issuecomment-559883092, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOQE5YEI5AIZBDRIBNSRB3QWGCQXANCNFSM4JOJLZVA .

-- http://timboudreau.com

[junichi11]

@timboudreau Thank you for your review!

[nishihatapalmer]

I've modified the PR, reverting the original fix (to not fire an event if there are no children).

TreePathSupport now explicitly fires EventBroadcaster as the last "listener", not making it part of the list of any other registered listeners.

This ensures that tree events are fully processed, including any tree modifications which occur in them, before they are translated into table events by the EventBroadcaster.

The OutlineSortTest unit test fails on current master (as expected), and passes with these changes.

[mbien]

added NB14 milestone to keep this on the radar.

[neilcsmith-net]

What is the state of this PR? It seems to have been bumped around milestones with no action since, and probably needs squashing and re-reviewing to be merged, as well as tests being run?

[timboudreau]

Inasmuch as I remember the code, the changes - as far as I can tell - seem reasonable, and adding a test is good.

The patch contains a lot of formatting changes that makes it very hard to tell what really is different. But if it works, I'd say go for it.

[timboudreau]

I read it through, and it seems reasonable - but please, when patching stuff, don't reformat the sources - the patch contains a ton of formatting changes that make the code changes difficult to spot. I know, I know, we all compulsively hit ctrl-shift-f :-)

-Tim

On Thu, Jul 14, 2022 at 5:57 AM Neil C Smith @.***> wrote:

What is the state of this PR? It seems to have been bumped around milestones with no action since, and probably needs squashing and re-reviewing to be merged, as well as tests being run?

— Reply to this email directly, view it on GitHub https://github.com/apache/netbeans/pull/1639#issuecomment-1184243952, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOQE5ZPW5XPDLLJ5QURNULVT7QA5ANCNFSM4JOJLZVA . You are receiving this because you were mentioned.Message ID: @.***>

-- http://timboudreau.com

[mbien]

@timboudreau github has a "hide whitespace changes" setting which helps a bit (little cog on the review tab).

In any case, this PR needs rebasing to get a fresh CI run. There are also unresolved comments about missing license headers. If we don't do this we should close this PR i think.

@nishihatapalmer still interested to get this in or should we close it?