Community
Participate
Working Groups
The batch expand and collapse functionality is extremely slow, even with a limited data set. A tree with 100 parents each with 10 children takes around 20 seconds to expand using the expandAll() or expandToLevel(int) methods on the viewer. While it is expanding, it flashes and flickers as well. Simply disabling repainting on the tree fixes the issue, and causes the expand to take less than a second. viewer.getTree().setRedraw(false); viewer.expandAll(); viewer.getTree().setRedraw(true); I cannot see any reason why this code shouldn’t be included in the viewer method, as the intermediate repaint events are so costly in terms of time and CPU resources and provide no benefit that I can determine.
Created attachment 43642 [details] Snippet demonstrating the expand and collapse issues
Created attachment 44093 [details] Patch for 3.2 RC 7 This is a patch for version 3.2 RC 7. It simply disables redraw during the expandToLevel, collapseToLevel, and setExpandedElements calls in AbstractTreeViewer. This fix takes care of all of the performance issues that I had. It would be great if we could get this into a future 3.2 release candidate rather than waiting for 3.3.
In the JFace viewers, we try not to turn redraw off and on for clients because of problems with nesting, and because it can cause flicker. We recommend that clients call setRedraw() as they see fit. In general, the call setRedraw(true) should be in a finally block like this: viewer.getTree().setRedraw(false); try { viewer.expandAll(); } finally { viewer.getTree().setRedraw(true); }
(In reply to comment #3) > In the JFace viewers, we try not to turn redraw off and on for clients because > of problems with nesting, The behavior of the setRedraw method was particularly designed for nesting, was it not? I am having trouble seeing how this could be an issue, particularly after your excellent suggestion to put the setRedraw(true) in a finally block. > and because it can cause flicker. One of the primary things this bug and patch address are flicker. A normal expandAll causes multiple intermediate redraws, which apart from taking an extremely unreasonable amount of time for the operation also produces a great deal of what I would classify as flicker. Please think about it in these terms: there are no circumstances in which redraw should be enabled for an expandAll. For a small, known number of elements it might have acceptable performance, but for any moderate number, the performance is abysmal. So if the current solution is to advise the consumers to wrap up their call in a setRedraw(false) try/finally block, why would we not simply do this for them? If they have already wrapped the call at this or a higher level, well, no harm done. If they have not, then we have greatly increased their performance, with no harm done. To expect the users to have the deep knowledge of the framework to even know to do this themselves does not seem to be very reasonable. We should be endeavoring to make the framework as easy to master and consume as possible. I ask you to please take a look at the snippet one more time, and reconsider addressing the issue. If this is not the solution you would like to pursue, I'm sure there are other ways to address it. I think the important thing is to recognize that it is an issue that needs to be addressed.
(In reply to comment #4) > ... > If they have already wrapped the call at this or a > higher level, well, no harm done. If they have not, then we have greatly > increased their performance, with no harm done. Unfortunately not - on Windows (not sure about the other Platforms right now), wrapping in setRedraw(false)/setRedraw(true) causes a full repaint of the tree control. This is the flicker that I was referring to. > To expect the users to have > the deep knowledge of the framework to even know to do this themselves does not > seem to be very reasonable. We should be endeavoring to make the framework as > easy to master and consume as possible. I understand, but don't think it would be right to put the setRedraw() calls in the expand* methods. By design, JFace is not an abstraction layer on top of SWT, it does not "hide" SWT in any way, and thus should not try to compensate for the performance characteristics of SWT. At least not without being asked for it - we could consider new API for 3.4 that allows clients to specify that they want the viewer to use setRedraw() calls for expensive operations.
(In reply to comment #5) > [..] > At least not without being asked for it - we could consider new API for 3.4 > that allows clients to specify that they want the viewer to use setRedraw() > calls for expensive operations. I'd appreciate that approach. Viewers are not intended to be subclassed outside the framework, so alternatively they should in fact provide a means for clients to decide themselves if they want the viewer to use setRedraw().
