<Swing Dev> [10][JDK-8187936] Automatically selecting a new JTree node in a model listener can cause unusual behavior.
Phil Race
philip.race at oracle.com
Mon Dec 11 16:52:13 UTC 2017
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/
> <http://cr.openjdk.java.net/%7Ekaddepalli/8187936/webrev01/>
>
> Regards,
>
> Krishna
>
> *From:*Phil Race
> *Sent:* Wednesday, December 6, 2017 10:17 PM
> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>;
> swing-dev at openjdk.java.net
> *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 <krishna.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
> <mailto:swing-dev at openjdk.java.net>
> *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:* swing-dev at openjdk.java.net
> <mailto:swing-dev at openjdk.java.net>
> *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:* swing-dev at openjdk.java.net
> <mailto:swing-dev at openjdk.java.net>
> *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/
> <http://cr.openjdk.java.net/%7Ekaddepalli/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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20171211/5b865cb6/attachment.html>
More information about the swing-dev
mailing list