• Type: Bug
• Component: javafx
• Sub-Component: scenegraph
• Affected Version: 8

• Priority: P4
• Status: Resolved
• Resolution: Fixed

• Submitted: 2013-05-13
• Updated: 2015-06-17
• Resolved: 2013-06-13

JDK 8
8Fixed

Yes, probably best to do in the controls repo if at all possible. Assuming you test the changes you make in TableView on HelloTableView then I am happy for you to push. 12-06-2013 OK, will do. I guess it will be better to fix the TableView and revert this change also in controls repo, to avoid integration problems. I will attach the diff to the JIRA issue if you don't insist on other review process. 12-06-2013 Were you going to also push this double layout patch to the controls repo? 12-06-2013 OK, attaching the diff that needs to be applied to allow double layouts. I will reopen this issue so we won't for get to revert it once the TableView/TableTreeView headers are fixed. 12-06-2013 @Martin: Is it worth getting your double layout 'fix' into the controls repo prior to our integration tomorrow, so that b94 doesn't have to have the same level of regressions as b93? Regarding a proper fix - if you can get a patch for the known issues in TableView, then I can review that and try to apply the same fix elsewhere. Right now I have not gone through all the UI controls looking for regressions, but I thought I would at least respond with this suggestion whilst our timezones briefly overlap. 12-06-2013 While the final diff contains more changes, the "fix" for the symptoms we now see in controls should be a one-line change, that would allow double layout in one pulse (I've tried it and TableView reordering/sorting/resizing works fine). But I still think the real cause here is that we misuse the fact we now do two layout passes per pulse, causing most of the scenegraph to recompute it's min/pref size and layoutBounds, although most of them will stay the same. What also troubles me on this pattern is that it might fall into a infinite layout triggering, if not done carefully enough. Not to mention SceneBuilder and features like priniting need to do layout pass arbitrary number of times, hoping the layout will be done in the end. I understand that fixing these new bugs based on the new rules of the game are just putting more stress on your team and that you have other bugs to fix, so I'm happy to help you to get the things right in controls. Can you do a quick analysis on what other controls also contain this pattern ( effectively changing min/pref size of a Parent/Region/Control while in it's layoutChildren method)? If it's only TableView/TableTreeView header, than it should be hopefully fixed in a few days. Ad performance gains: while the latest performance results showed only a slight improvement on the desktop (where the time of the layout pass is rather marginal), I wonder what would be the results on the embedded as they seems to be concerned with the layout performance, esp. excessive layout triggering. 12-06-2013 The original 3 diffs were pretty self contained within Region class and just effected how the Regions properties called requestLayout(). This is quite a simple problem to think though and have a good level of confidence that they are correct. The final diff "rt-30363.diff" is much wider spread across Node,Parent and Group and has really far reaching impact on the way layout is applied in JavaFX. This change should have had its own bug with much more detailed explanation of what its doing and also much more though testing as it has very far reaching impact. Which has been shown to break a good deal of controls and applications. I am not sure if we should back this out or just work on fixing it so that everything works again. But it looks like we will have had at last 2 builds 93,94 that are not really usable by app developers. 11-06-2013 I think Jonathan raises a fair point. Perhaps Jasper or Richard can comment on the benefits? 11-06-2013 Sorry to yet again come back to this issue (I won't reopen it this time), but it appears that TableView and TreeTableView are also very broken by this changeset. In short, column reordering, column resizing and sorting are all broken. There is a partial patch in RT-31016 with regard to the earlier issue with column headers not appearing at all, but I fear that this issue is becoming greater, not lesser, as more time is spent triaging incoming bug reports (and also in hearing the analysis from Martin in RT-31016). In short, there was previously a pattern we used to delay work until the layoutChildren() method (so that work was not done needlessly inbetween pulses), and it appears that this pattern is now becoming deprecated and will soon be unavailable. I worry that in every place we used this pattern (which is quite frequently) we may have subtle (and not so subtle) breakages. I am very worried that the controls team may be staring down the barrel of relatively major breakages and the need to refocus our effort on fixing these regressions (in place of our other bug fixing work that we are focused on). I guess my question is whether the performance gains from this changeset outweigh the likelihood of considerable work needing to be done to refactor UI controls to play within the new rules? 11-06-2013 Sorry to reopen this issue again (tell me if you would rather I didn't), but RT-31016 is also caused by this changeset - I tested locally pre- and post- application of this changeset and it is definitely appears to be caused by this optimisation. 09-06-2013 RT-30947 is caused by this changeset. 06-06-2013 Similarly, RT-30918 appears to be due to this changeset. 06-06-2013 I am reopening this issue as during controls team integration David noticed two regressions (RT-30936 and RT-30938). Certainly RT-30936 appears to be directly related to this changeset, and I would presume RT-30938 is also related. I have linked both issues to this issue and assigned them to Martin. Let me know if I can help any further. 05-06-2013 I had to reject some parts of these patches, because they would not work in all cases or updated them with proper calls. I also optimized the case I described in the previous comment, but I need your approval for Region#requestParentLayout(). It's basically a call that says the min/pref/max of the current node changed as oposed to Region#requestLayout that requests layout of the current Pane because of layout configuration of one of the childs changed (called by requestParentLayout in the latter case). Together with this, I also changed non-standard Group computation of pref/min size, because the Group use old layoutBounds and relied on the fact that everything gets flushed again during the layout and the second layout pass in the pulse fix the min/pref size of the Group (yes, we have 2 layout passes per pulse, I wonder if this was the only reason). This was the reason why the ScrollPane didn't work, because I didn't flush the Group's sizes and the second layout pass could not recompute and use it's correct pref size. I've tried some samples and Ensemble + Ensemble8 and evething worked fine. Well OK, Ensemble had some strange code that computed pref height of scrollpane's content using the size of the scrollpane (i.e. parent) in order to stretch the content to the height of the scrollpane, which is wrong. I fixed this by setting setFitToHeight on the scrollpane instead :-). 17-05-2013 Thanks, I already have something like this :-) But currently, I'm trying to investigate whether we could replace some requestLayout() calls with something else. Esp. when you don't want to flush min/pref size cache of the current Region and/or it's parents. I've done some refactoring and create a new protected final void Region#requestParentLayout(), that would request layout of the parent (or add this Region to the scene as a layout root), but won't touch the current cache. Also, I've modified it so the caches of the parent are not flushed during the parent's layout. Because the current code does call requestLayout() on resize(), flushing all the sizes to the current layout root, although all of them have been just layouted and are correct. I still have a problem with ScrollPane thought. I'm trying to investigate what's wrong with it. I understand it's an API change and we are almost at feature freeze, but it might be worth the performance gain, as we'd save lot's of min/pref recomputations. 17-05-2013 The combination of these 3 patches I think fixes this issue, but you should do a complete review. 16-05-2013 I have 3 patches from the embedded fest repo for this. 16-05-2013