<Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Krishna Addepalli

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Krishna Addepalli

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Krishna Addepalli

Hi Sergey,

 

Gentle reminder. Could you review?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, November 8, 2017 8:22 PM
To: [hidden email]
Subject: RE: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Philip Race
Hi,

Whilst I could perhaps provide some very minor tweaks to the wording you
are adding, I would first like to understand the details of why the rendering
becomes broken here since there doesn't seem to be anything intrinsically
wrong - only the model is being updated.
I've found that commenting out one of the other listener methods
BasicTreeUI.Handler.treeNodesInserted - "cures" it.
So what we seem to be saying here, is that when you get notification
of a change in the model, you must not make further changes in the model
until any UI listener has processed the first change .. and throwing this
on the queue via invokeLater is the obvious way to do that.

But I've run out of time to look at quite where the rendering breaks
and I'm not sure if this is as general a problem as the new comment
would imply.

So can you say something about why we miss rendering some nodes
and quite why BasicTreeUI.Handler.treeNodesInserted is breaking it.

-phil.



On 11/12/2017 11:07 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Gentle reminder. Could you review?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, November 8, 2017 8:22 PM
To: [hidden email]
Subject: RE: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Krishna Addepalli

Hi Phil,

 

I have attached the code for your reference. However, the problem is specifically in this function:

 

            @Override
           
public void treeNodesInserted(final TreeModelEvent event) {
                SwingUtilities.invokeLater(() -> {
                   
final TreePath pathToLastInsertedChild =
                           
event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
                   
tree.setSelectionPath(pathToLastInsertedChild);
               
});

//                final TreePath pathToLastInsertedChild =
//                        event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
//                tree.setSelectionPath(pathToLastInsertedChild);
           
}

 

This is part of the TreeModelListener class, which receives callbacks for any changes to the underlying model of the tree – like insertion, deletion, structural changes etc. Now, the catch is even JTree has its own ModelListener which listens to changes so that it can adapt its GUI.

In this case, the listeners seem to be called from last to first – meaning the application registered listener will be called first and then the components own ModelListener.

In the above function, the application (commented out code) tries to highlight the path to the newly inserted node, which JTree has not yet seen since its listener is not called yet. This leads to inconsistent UI rendering, since it would be trying to show some node which is not present.

For such cases, it is recommended to use “SwingUtilities.invokeLater”, so that, the new UI event will be processed after JTree has had a chance to update itself to the changes in the model.

 

Here is the code that is calling the listeners (in DefaultTreeModel.java):

protected void fireTreeNodesInserted(Object source, Object[] path,
                                    int
[] childIndices,
                                    
Object[] children) {
   
// Guaranteed to return a non-null array
   
Object[] listeners = listenerList.getListenerList();
   
TreeModelEvent e = null;
   
// Process the listeners last to first, notifying
    // those that are interested in this event
   
for (int i = listeners.length-2; i>=0; i-=2) {
       
if (listeners[i]==TreeModelListener.class) {
           
// Lazily create the event:
           
if (e == null)
                e =
new TreeModelEvent(source, path,
                                       
childIndices, children);
           
((TreeModelListener)listeners[i+1]).treeNodesInserted(e);
       
}
    }
}

 

 

Thanks,

Krishna

 

 

From: Phil Race
Sent: Saturday, November 18, 2017 3:29 AM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi,

Whilst I could perhaps provide some very minor tweaks to the wording you
are adding, I would first like to understand the details of why the rendering
becomes broken here since there doesn't seem to be anything intrinsically
wrong - only the model is being updated.
I've found that commenting out one of the other listener methods
BasicTreeUI.Handler.treeNodesInserted - "cures" it.
So what we seem to be saying here, is that when you get notification
of a change in the model, you must not make further changes in the model
until any UI listener has processed the first change .. and throwing this
on the queue via invokeLater is the obvious way to do that.

But I've run out of time to look at quite where the rendering breaks
and I'm not sure if this is as general a problem as the new comment
would imply.

So can you say something about why we miss rendering some nodes
and quite why BasicTreeUI.Handler.treeNodesInserted is breaking it.

-phil.


On 11/12/2017 11:07 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Gentle reminder. Could you review?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, November 8, 2017 8:22 PM
To: [hidden email]
Subject: RE: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 

 


MinimalDemoFixed.java (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Philip Race
So you seem to be saying that the listeners installed by the UI component itself
can process events directly on the EDT thread, but application code must use invokeLater ?
This is particularly important for events which cause structural updates to the model and the UI.


May I then suggest the following version of text on the package description:
--

Although it is generally safe to make updates to the UI immediately,
when executing on the event dispatch thread, there is an exception :
if a model listener tries to further change the UI before the UI has been updated to
reflect a pending change then the UI may render incorrectly.

This can happen if an application installed listener needs to update the UI in response
to an event which will cause a change in the model structure. It is important to first allow
component installed listeners to process this change, since there is no guarantee of the order
in which listeners may be called.

The solution is for the application listener to make the change using {@link SwingUtilities.invokeLater}
so that any changes to 
UI rendering will be done post processing all the model listeners
installed by the component.
---

-phil.



On 11/17/2017 06:11 PM, Krishna Addepalli wrote:

Hi Phil,

 

I have attached the code for your reference. However, the problem is specifically in this function:

 

            @Override
           
public void treeNodesInserted(final TreeModelEvent event) {
                SwingUtilities.invokeLater(() -> {
                   
final TreePath pathToLastInsertedChild =
                           
event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
                   
tree.setSelectionPath(pathToLastInsertedChild);
               
});

//                final TreePath pathToLastInsertedChild =
//                        event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
//                tree.setSelectionPath(pathToLastInsertedChild);
           
}

 

This is part of the TreeModelListener class, which receives callbacks for any changes to the underlying model of the tree – like insertion, deletion, structural changes etc. Now, the catch is even JTree has its own ModelListener which listens to changes so that it can adapt its GUI.

In this case, the listeners seem to be called from last to first – meaning the application registered listener will be called first and then the components own ModelListener.

In the above function, the application (commented out code) tries to highlight the path to the newly inserted node, which JTree has not yet seen since its listener is not called yet. This leads to inconsistent UI rendering, since it would be trying to show some node which is not present.

For such cases, it is recommended to use “SwingUtilities.invokeLater”, so that, the new UI event will be processed after JTree has had a chance to update itself to the changes in the model.

 

Here is the code that is calling the listeners (in DefaultTreeModel.java):

protected void fireTreeNodesInserted(Object source, Object[] path,
                                    int[] childIndices,
                                    Object[] children) {
    // Guaranteed to return a non-null array
    Object[] listeners = listenerList.getListenerList();
    TreeModelEvent e = null;
    // Process the listeners last to first, notifying
    // those that are interested in this event
    for (int i = listeners.length-2; i>=0; i-=2) {
        if (listeners[i]==TreeModelListener.class) {
            // Lazily create the event:
            if (e == null)
                e = new TreeModelEvent(source, path,
                                       childIndices, children);
            ((TreeModelListener)listeners[i+1]).treeNodesInserted(e);
        }
    }
}

 

 

Thanks,

Krishna

 

 

From: Phil Race
Sent: Saturday, November 18, 2017 3:29 AM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi,

Whilst I could perhaps provide some very minor tweaks to the wording you
are adding, I would first like to understand the details of why the rendering
becomes broken here since there doesn't seem to be anything intrinsically
wrong - only the model is being updated.
I've found that commenting out one of the other listener methods
BasicTreeUI.Handler.treeNodesInserted - "cures" it.
So what we seem to be saying here, is that when you get notification
of a change in the model, you must not make further changes in the model
until any UI listener has processed the first change .. and throwing this
on the queue via invokeLater is the obvious way to do that.

But I've run out of time to look at quite where the rendering breaks
and I'm not sure if this is as general a problem as the new comment
would imply.

So can you say something about why we miss rendering some nodes
and quite why BasicTreeUI.Handler.treeNodesInserted is breaking it.

-phil.


On 11/12/2017 11:07 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Gentle reminder. Could you review?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, November 8, 2017 8:22 PM
To: [hidden email]
Subject: RE: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Krishna Addepalli

Hi Phil

 

Thank you for your time in understanding the issue and rephrasing the text. I have created a new webrev, with the text you mentioned, and here it is: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev01/

 

Regards,

Krishna

 

From: Phil Race
Sent: Wednesday, December 6, 2017 10:17 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

So you seem to be saying that the listeners installed by the UI component itself
can process events directly on the EDT thread, but application code must use invokeLater ?
This is particularly important for events which cause structural updates to the model and the UI.


May I then suggest the following version of text on the package description:
--

Although it is generally safe to make updates to the UI immediately,
when executing on the event dispatch thread, there is an exception :
if a model listener tries to further change the UI before the UI has been updated to
reflect a pending change then the UI may render incorrectly.

This can happen if an application installed listener needs to update the UI in response
to an event which will cause a change in the model structure. It is important to first allow
component installed listeners to process this change, since there is no guarantee of the order
in which listeners may be called.

The solution is for the application listener to make the change using {@link SwingUtilities.invokeLater}
so that any changes to  UI rendering will be done post processing all the model listeners
installed by the component.
---

-phil.

On 11/17/2017 06:11 PM, Krishna Addepalli wrote:

Hi Phil,

 

I have attached the code for your reference. However, the problem is specifically in this function:

 

            @Override
           
public void treeNodesInserted(final TreeModelEvent event) {
                SwingUtilities.invokeLater(() -> {
                   
final TreePath pathToLastInsertedChild =
                           
event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
                   
tree.setSelectionPath(pathToLastInsertedChild);
               
});

//                final TreePath pathToLastInsertedChild =
//                        event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
//                tree.setSelectionPath(pathToLastInsertedChild);
           
}

 

This is part of the TreeModelListener class, which receives callbacks for any changes to the underlying model of the tree – like insertion, deletion, structural changes etc. Now, the catch is even JTree has its own ModelListener which listens to changes so that it can adapt its GUI.

In this case, the listeners seem to be called from last to first – meaning the application registered listener will be called first and then the components own ModelListener.

In the above function, the application (commented out code) tries to highlight the path to the newly inserted node, which JTree has not yet seen since its listener is not called yet. This leads to inconsistent UI rendering, since it would be trying to show some node which is not present.

For such cases, it is recommended to use “SwingUtilities.invokeLater”, so that, the new UI event will be processed after JTree has had a chance to update itself to the changes in the model.

 

Here is the code that is calling the listeners (in DefaultTreeModel.java):

protected void fireTreeNodesInserted(Object source, Object[] path,
                                    int[] childIndices,
                                    Object[] children) {
    // Guaranteed to return a non-null array
    Object[] listeners = listenerList.getListenerList();
    TreeModelEvent e = null;
    // Process the listeners last to first, notifying
    // those that are interested in this event
    for (int i = listeners.length-2; i>=0; i-=2) {
        if (listeners[i]==TreeModelListener.class) {
            // Lazily create the event:
            if (e == null)
                e = new TreeModelEvent(source, path,
                                       childIndices, children);
            ((TreeModelListener)listeners[i+1]).treeNodesInserted(e);
        }
    }
}

 

 

Thanks,

Krishna

 

 

From: Phil Race
Sent: Saturday, November 18, 2017 3:29 AM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi,

Whilst I could perhaps provide some very minor tweaks to the wording you
are adding, I would first like to understand the details of why the rendering
becomes broken here since there doesn't seem to be anything intrinsically
wrong - only the model is being updated.
I've found that commenting out one of the other listener methods
BasicTreeUI.Handler.treeNodesInserted - "cures" it.
So what we seem to be saying here, is that when you get notification
of a change in the model, you must not make further changes in the model
until any UI listener has processed the first change .. and throwing this
on the queue via invokeLater is the obvious way to do that.

But I've run out of time to look at quite where the rendering breaks
and I'm not sure if this is as general a problem as the new comment
would imply.

So can you say something about why we miss rendering some nodes
and quite why BasicTreeUI.Handler.treeNodesInserted is breaking it.

-phil.



On 11/12/2017 11:07 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Gentle reminder. Could you review?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, November 8, 2017 8:22 PM
To: [hidden email]
Subject: RE: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

Philip Race
Ok.

-phil.

On 12/07/2017 03:39 AM, Krishna Addepalli wrote:

Hi Phil

 

Thank you for your time in understanding the issue and rephrasing the text. I have created a new webrev, with the text you mentioned, and here it is: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev01/

 

Regards,

Krishna

 

From: Phil Race
Sent: Wednesday, December 6, 2017 10:17 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

So you seem to be saying that the listeners installed by the UI component itself
can process events directly on the EDT thread, but application code must use invokeLater ?
This is particularly important for events which cause structural updates to the model and the UI.


May I then suggest the following version of text on the package description:
--

Although it is generally safe to make updates to the UI immediately,
when executing on the event dispatch thread, there is an exception :
if a model listener tries to further change the UI before the UI has been updated to
reflect a pending change then the UI may render incorrectly.

This can happen if an application installed listener needs to update the UI in response
to an event which will cause a change in the model structure. It is important to first allow
component installed listeners to process this change, since there is no guarantee of the order
in which listeners may be called.

The solution is for the application listener to make the change using {@link SwingUtilities.invokeLater}
so that any changes to  UI rendering will be done post processing all the model listeners
installed by the component.
---

-phil.

On 11/17/2017 06:11 PM, Krishna Addepalli wrote:

Hi Phil,

 

I have attached the code for your reference. However, the problem is specifically in this function:

 

            @Override
           
public void treeNodesInserted(final TreeModelEvent event) {
                SwingUtilities.invokeLater(() -> {
                   
final TreePath pathToLastInsertedChild =
                           
event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
                   
tree.setSelectionPath(pathToLastInsertedChild);
               
});

//                final TreePath pathToLastInsertedChild =
//                        event.getTreePath().pathByAddingChild(event.getChildren()[event.getChildren().length - 1]);
//                tree.setSelectionPath(pathToLastInsertedChild);
           
}

 

This is part of the TreeModelListener class, which receives callbacks for any changes to the underlying model of the tree – like insertion, deletion, structural changes etc. Now, the catch is even JTree has its own ModelListener which listens to changes so that it can adapt its GUI.

In this case, the listeners seem to be called from last to first – meaning the application registered listener will be called first and then the components own ModelListener.

In the above function, the application (commented out code) tries to highlight the path to the newly inserted node, which JTree has not yet seen since its listener is not called yet. This leads to inconsistent UI rendering, since it would be trying to show some node which is not present.

For such cases, it is recommended to use “SwingUtilities.invokeLater”, so that, the new UI event will be processed after JTree has had a chance to update itself to the changes in the model.

 

Here is the code that is calling the listeners (in DefaultTreeModel.java):

protected void fireTreeNodesInserted(Object source, Object[] path,
                                    int[] childIndices,
                                    Object[] children) {
    // Guaranteed to return a non-null array
    Object[] listeners = listenerList.getListenerList();
    TreeModelEvent e = null;
    // Process the listeners last to first, notifying
    // those that are interested in this event
    for (int i = listeners.length-2; i>=0; i-=2) {
        if (listeners[i]==TreeModelListener.class) {
            // Lazily create the event:
            if (e == null)
                e = new TreeModelEvent(source, path,
                                       childIndices, children);
            ((TreeModelListener)listeners[i+1]).treeNodesInserted(e);
        }
    }
}

 

 

Thanks,

Krishna

 

 

From: Phil Race
Sent: Saturday, November 18, 2017 3:29 AM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi,

Whilst I could perhaps provide some very minor tweaks to the wording you
are adding, I would first like to understand the details of why the rendering
becomes broken here since there doesn't seem to be anything intrinsically
wrong - only the model is being updated.
I've found that commenting out one of the other listener methods
BasicTreeUI.Handler.treeNodesInserted - "cures" it.
So what we seem to be saying here, is that when you get notification
of a change in the model, you must not make further changes in the model
until any UI listener has processed the first change .. and throwing this
on the queue via invokeLater is the obvious way to do that.

But I've run out of time to look at quite where the rendering breaks
and I'm not sure if this is as general a problem as the new comment
would imply.

So can you say something about why we miss rendering some nodes
and quite why BasicTreeUI.Handler.treeNodesInserted is breaking it.

-phil.



On 11/12/2017 11:07 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Gentle reminder. Could you review?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, November 8, 2017 8:22 PM
To: [hidden email]
Subject: RE: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi Sergey,

Could you review this and let me know your feedback?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Friday, October 27, 2017 4:43 PM
To: [hidden email]
Subject: [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.

 

Hi All,

Please review the help text that is updated for this bug:

 

Bug: JDK-8187936: https://bugs.openjdk.java.net/browse/JDK-8187936

 

Webrev: http://cr.openjdk.java.net/~kaddepalli/8187936/webrev00/

 

Summary:

As the bug title mentions, this is an unrecommended way of using the model listeners. The code posted in the bug tries to update the JTree path in the model listener callback, and since the JTree is yet to change itself to the underlying model, it results in weird UI behavior.

Attached code in the bug is corrected and re-uploaded.

 

This typically happens since listeners are called in a particular order (either last to first or first to last in the order of registration), and if the model listener tries to change the GUI before the GUI has had a chance to react itself to the changes in the underlying model. For example, highlighting a selection path for a node added in the JTree, in the TreeModelListener callback could lead to an extra node being added or existing nodes not being shown, since JTree would not have yet updated its state based on the model changes.

 

In such cases it is recommended to wrap the callback function contents into a lambda, and invoke it through “SwingUtilities.invokeLater”. This ensures that all the UI elements have had a chance to react to the model changes, and any UI actions will be guaranteed to operate on the updated widgets.

 

Similar update has been done in package-info.java for Swing, so that it acts as a reference.

 

Thanks,

Krishna