I haven’t done much in terms of custom controls, but I do see this issue a lot in my primary application. It would be nice to have it fixed as the layout looks rather sloppy without it. Any time I see the layout “correct” itself when I interact with a control or tweak the size of a window it is also distracting. When I saw this fix proposed here my first thought was, “finally!” So I for one am looking forward to this. It seems that the number of iterations will be very low for most cases, so I’m not terribly concerned about any sort of performance impact. But that is my only worry, as I have had issues with layout/CSS taking a lot of time in the past, so in some cases even a single extra iteration may lead to a perceptible delay. (I was mostly working with FX 8 when I had those issues, so hopefully things are better in recent versions, I haven’t measured.) I agree that the default limit could probably be reduced, 10 iterations would likely be plenty more than needed. As you say, something is probably wrong with your control if it goes that far.
Regards, Scott > On Apr 2, 2021, at 10:51 AM, Kevin Rushforth <kevin.rushfo...@oracle.com> > wrote: > > Circling back to this: Do any JavaFX application developers on this list > have any thoughts? I think this is a good idea, and it seems worth moving > this forward, but it will require a fair bit of effort. I'd like to hear > buy-in from other developers -- particularly those who develop or use custom > controls. > > A few additional comments: > > * Regarding the threshold, I like the idea of logging at a fine (or finer) > level if it goes beyond a smaller number, say 3 or 4, passes, but continuing > further to iterate. I wonder if 100 might be too large for the default limit, > though. It seems likely that the control is unstable and will never converge > if it gets to that many layout passes. If there were a way to analyze what a > reasonable limit is that would be better (if not, we can probably live with a > fairly high value, if it really is an unlikely pathological case). Assuming a > reasonably high threshold, logging at a warning level would be good if it > doesn't converge. > > * Documentation: I do think we will want to document the overall concept of > layout needing multiple passes to converge in some cases, along with > documenting the threshold. I also think that Node::getBaselineOffset should > mention the preference of text nodes. The concept of "text" nodes needs to be > on Node, if for no other reason than that Text doesn't inherit from Parent. > > * This will need to be very well tested, both to ensure no regressions in > behavior, and with many new tests added to validate the new functionality, > including testing the threshold logic. > > -- Kevin > > >> On 3/20/2021 6:42 AM, Michael Strauß wrote: >> 1) Pathological cases: So far, I haven't been able to produce >> pathological cases that exceed 3 layout passes by using standard >> layout containers and controls. If we need to have a threshold >> substantially higher than that depends on whether or not there are >> indeed legitimate cases where some kind of layout requires more passes >> to converge. Maybe we could log at a very fine level when a control >> takes more than 3 passes to converge, but still continue processing up >> to a substantially higher number of passes. This would allow >> developers to debug and optimize their custom controls, but still >> produce a valid solution if a particular control is generally stable, >> but just very poorly optimized. >> >> 2) I believe that the multi-pass layout algorithm shouldn't need >> additional documentation since it could be considered an >> implementation detail. There is no 1:1 correspondence between pulses >> and layout passes currently, and there won't be with the new >> algorithm. Also, any layout pass with the current algorithm can >> potentially trigger another layout pass, so that's not fundamentally >> different either. For control authors, we might need to document that >> it is generally okay to rely on circular dependencies between size >> hints, baseline offset and child nodes, as long as those dependencies >> allow the layout to settle. The fact that text nodes are preferred >> with respect to baseline computation is documented in >> Parent.getBaselineOffset(), maybe Node.getBaselineOffset() should also >> mention this. We might also add a section on baseline computation to >> the javafx.scene.layout package documentation, where I think it is >> most relevant (and currently entirely undocumented). >> >> 3) New API on Node: in order for this idea to work, there needs to be >> an abstract concept of "text node". Since the "baseline" concept is >> introduced on Node, I thought this would also be the appropriate place >> to introduce the "text node" concept; however, it could also be >> introduced on Parent instead. Note that the "text node" concept is >> introduced at a lower level than actual text controls like Labeled, >> because layout containers (which are not controls) need this >> information. Also, the definition of what constitutes "text" is left >> to control authors. In a very contrived example, a control might >> display an image, but declare itself to be "text". >> >> >> >>> Am Fr., 19. März 2021 um 23:26 Uhr schrieb Kevin Rushforth >>> <kevin.rushfo...@oracle.com>: >>> I'd be interested to hear from app developers on this list. >>> >>> Here are a few quick thoughts I have. >>> >>> As you note, this is a long-standing problem with layout in FX. You >>> mention in the performance considerations that for most cases this will >>> iterate quickly. It would be interesting to know what some of the corner >>> cases are, so we can see how bad the pathological case will be. I see >>> that you propose to iterate up to 100 times. Maybe a lower threshold >>> would be better? We already run layout passes 3 times in many cases. So >>> also running 3 (or maybe 4 or 5) times seems reasonable, especially when >>> your testing shows that most of the time it converges in <= 2 passes. So >>> a smaller threshold than 100 would seem to make sense. If a control >>> doesn't converge, you might consider logging it (in case an app >>> developer wants to debug it) perhaps at a "fine" level so it isn't shown >>> by default. >>> >>> How do you propose to document this behavioral change -- not so much the >>> fact that it will (usually) get the right answer now instead of the >>> wrong one, but more about the multi-pass nature of it, and the fact that >>> nodes with text will be "preferred". Related to that, how necessary is a >>> new public API on Node? >>> >>> -- Kevin >>> >
