Little worried about the ListView/TreeView/TableView classes

[John Hendrikx]

On 10/07/2012 21:55, Jonathan Giles wrote:
> Comments inline.
>
> -- Jonathan
>
> On 11/07/2012 5:38 a.m., John Hendrikx wrote:
>> Hi Jonathan,
>>
>> I'll do what can to help get some of these resolved, so I have filed 
>> three new issues (all using the same sample code), RT-23331,  
>> RT-23332 and RT-23333 which address three of the problems below.  
>> Some of the others I already created issues for, the most critical of 
>> those (causing graphics glitches) is a solution for this one:
>>
>> RT-22555 Unpredictable behaviour with scrollTo and the view classes 
>> (TableView as example)
>>
>> It causes rendering to fail -- for Scenes that are fairly static this 
>> can mean some controls are simply invisible until the user tries 
>> interacting with them.  The provided sample is brute-force code to 
>> get this to occur, but it happens in normal use of my app (especially 
>> when only layout is changed and some controls are swapped out, ie, a 
>> TreeView becomes a TableView to show items in 2 columns -- in those 
>> cases all data is loaded already and the change can happen very fast 
>> -- when more processing still needs to occur (like images being 
>> loaded causing item changes) it doesn't happen.. or it simple hides 
>> the problem because of a redraw occuring).
> I've noted a simplistic workaround to this problem in the jira issue. 
> Basically, you're trying to modify the state of UI controls before 
> they are in a scene, which is not very well defined.

They are part of the Scene, but not rendered yet probably.  It all 
occurs in a ChangeListener on a property that a user can change (they 
can choose different layouts for the screen) so it is all on the 
FX-thread.  Here's a code snippet (with added comments):

        // Inside ChangeListener here
         MediaNode selectedNode = null;  // holds previous select node

         if(layout != null) {  // unbinds/removes old layout from the 
StackPane this Changelistener is part of
           if(currentRoot.equals(layout.getRoot())) {
             selectedNode = layout.getSelectedNode();  // it only makes 
sense to re-select the focused node if the root did not change
           }

           layout.onNodeSelected().set(null);  // unbinding of listeners
           layout.onNodeAlternateSelect().set(null);
           focusedNode.unbind();

           getChildren().remove(layout);  // remove from StackPane
         }

         layout = layoutExtension.createLayout();  // creates the new 
layout, a StackPane with two panels one containing a *View class
         getChildren().add((Node)layout);  // adds it to the Scene (the 
ChangeListener is part of a StackPane that is already visible and 
doesn't change).

         backgroundPane.mediaNodeProperty().bind(layout.mediaNodeBinding());

         layout.onNodeSelected().set(onNodeSelected.get());
         layout.onNodeAlternateSelect().set(onNodeAlternateSelect.get());
         focusedNode.bind(layout.focusedNodeProperty());

         layout.setRoot(currentRoot);  // this causes the Tree/TableView 
to be filled with data (well after it was added to Scene)

         if(selectedNode != null) {
           setSelectedNode(selectedNode);  // this causes the correct 
item to be selected and scrolled to
         }

I haven't tried other "solutions" yet, like binding to sceneProperty() 
and waiting for it to become non-null (but I guess that happens right 
after the getChildren().add() occurs).

Other workarounds might be adding all possible layouts to the Scene 
right from the start, hiding all but the user choosen one.

>>
>> Then there several issues dealing with Cell creation:
>>
>> RT-22946 TreeView creates cells for every item from start to scrollTo 
>> position
>> RT-20892 Layout pass of TreeView can result in unneccessary 
>> recreation of TreeCells
>> RT-20616 TreeView creates new cells every time root is changed
>>
>> The first one is the most problematic because it creates lots of 
>> cells but more importantly.. *different* cells.  The other two are 
>> less of a problem since atleast they donot cause data to be accessed 
>> that is not going to be displayed.
> I've definitely got a few performance issues with 
> TreeView/ListView/TableView, but I wasn't about to try and fix them at 
> the end of the 2.2 development cycle (it's just too risky). Instead, I 
> hope to look at these issues very early in the next release cycle. I'm 
> more than happy to work with others on developing fixes via openjfx 
> though! :-)

Yes, I can understand that -- I don't know if I will be able to 
understand to code well enough and have the time to offer any help here 
-- I've looked through it before and it seems the VirtualFlow class 
which is common to all of these controls is probably where this can be 
resolved.  I haven't looked at openjfx much yet, I don't know yet how to 
go about getting that code base to run locally.

>>
>> The issue with creation of a ListView and using it immediately (with 
>> select/focus/scrollTo calls) is captured a bit in RT-22555 as well I 
>> think, resolving that will probably resolve these issues as well.
> I'm not too sure whether you're referring to another jira issue here?
Not specifically, this is related to manipulating controls before 
they're added to the Scene (or atleast before rendering first time).  
See the code above where I add a new control to a (visible) StackPane 
and set it up correctly with data/selection/scroll position all in the 
same ChangeListener.
>>
>> And there's two minor issues I filed related to these classes as well:
>>
>> RT-20879 After setting new root on TreeView, focus model's focused 
>> item property is not updated
>> RT-22398 Some navigation keys (pg-down/up, etc) donot work until 
>> TreeView/ListView/TableView item has both focus AND is selected
> I've asked for further feedback on these two issues in Jira - I'm 
> awaiting your response.
I commented on the 2nd one, I'll check out the other now.

>> For some of these it is rather hard to create good small test 
>> programs -- there appear to be timing issues with some of them 
>> (whether the ListView has already been rendered atleast once or not 
>> yet, and/or whether it was added to a Scene or not). Sometimes 
>> putting things like select/focus/scrollTo in a Platform.runLater() 
>> makes the issue worse or better.
> Platform.runLater is unfortunately not a solution (as you note). The 
> primary issue really is whether a node is part of a scene before you 
> run operations on it, and whether you're running operations in the 
> JavaFX application thread.
>>
>> I can provide help in the form of trying to write test cases for 
>> problems that you cannot reproduce at all.  The three most annoying 
>> bugs are:
>>
>> RT-22555 (causes graphic glitches)
>> RT-22946 (makes large lists slow to display when scrolling to a high 
>> index in the list when restoring last cursor position)
>> RT-23331 (graphics glitch where user cannot see the highlighted item)
> I've commented about RT-22555.
> RT-22946 falls into the bucket of perf fixes I hope to do early in the 
> next release cycle.
> RT-23331 seems to relate to a custom written skin. I would be very 
> interested if there are any issues when using the provided default skins.
As said, RT-23331 also occurs with the standard skin, just much more 
rare -- the custom skin just makes it easier to reproduce.  I don't 
understand how the skin is even involved in this (especially since it is 
the ScrollBarSkin and not the ListViewSkin itself).  I've not seen it 
happen yet with my fixed custom skin, but it might also be much more 
rare now, just like the default skin.

Could you reproduce it atleast?  It should show up as the wrong cell 
being selected and the real active cell not showing up as selected.

>>
>> The rest are annoying but only waste some resources or are minor gfx 
>> glitches or have work-arounds.
>>
>> If there is anything I can do to make these easier to solve, I can 
>> work on that in the weekend/evenings.  I'll upgrade to the latest 
>> beta tonight to see if any of them happened to have been resolved.
> Another thing you can consider doing is pulling the latest openjfx 
> mercurial repo code and attempting to develop fixes - all the controls 
> source is open. You never know, you might be able to fix some of these 
> bugs :-)

I haven't tried anything like that before... is this code complete 
enough to run?  Do I need tricks to get it loaded (like putting it in 
the 'pre' boot classpath?)

--John