8389653: Enhance JavaFX with virtual layout containers, and allow ordinary Nodes to participate - #2241
8389653: Enhance JavaFX with virtual layout containers, and allow ordinary Nodes to participate#2241hjohn wants to merge 5 commits into
Conversation
|
👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
Webrevs
|
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Not a review (yet), but a question: would it be better to include two panes instead of one, to gauge the amount of common code?
The BorderPane perhaps?
|
Since this is now /csr needed |
|
/reviewers 2 reviewers |
|
@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. @hjohn please create a CSR request for issue JDK-8389653 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
@kevinrushforth |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
some initial comments, for now.
| /** | ||
| * A default snapper for 1.0 scaling. | ||
| */ | ||
| static final Snapper DEFAULT = new Snapper() { |
There was a problem hiding this comment.
DEFAULT might be too generic. IDENTITY_SNAPPED or something like that?
There was a problem hiding this comment.
Yeah, maybe just IDENTITY or SCALE_1X or UNIT_SCALE
| * | ||
| * @return offset of text baseline from layoutBounds.minY for non-resizable Nodes or {@link #BASELINE_OFFSET_SAME_AS_HEIGHT} otherwise | ||
| */ | ||
| @Override |
There was a problem hiding this comment.
L3556: getLayoutBounds() needs an @Override
| * <li>{@code max} takes precedence over {@code pref}: If {@code max} is smaller | ||
| * than {@code pref}, the {@code max} value must be used. | ||
| * </ul> | ||
| * The preferred size should therefore be clamped within the range {@code [min, max]}. |
| * {@link #resizeRelocate(double, double, double, double)} resize and/or | ||
| * reposition an element to specific dimensions and coordinates. | ||
| * | ||
| * @see Measurable |
There was a problem hiding this comment.
all new public classes need @since 28 (or @since TBD, depending on how long this thing will marinade)
| /** | ||
| * The 'alphabetic' (or 'roman') baseline offset from the element's top boundary | ||
| * that should be used when this element is being vertically aligned by baseline with | ||
| * other elements. By default this returns {@link #BASELINE_OFFSET_SAME_AS_HEIGHT} for resizable elements |
There was a problem hiding this comment.
-> "By default, ..."
There was a problem hiding this comment.
Yeah, it was poorly worded in the original. Can adjust :)
| * @param snapScaleY a positive vertical scale factor, expressed as the | ||
| * ratio of device pixels to logical pixels on the Y axis | ||
| */ | ||
| public record RenderScaleContext(double snapScaleX, double snapScaleY) { |
There was a problem hiding this comment.
minor: just RenderScale maybe? or do you plan to add more context?
There was a problem hiding this comment.
No, no further plans. I see this type as a way to provide facts about the rendering surface that are relevant for layouts. If we ever did want to extend it with additional details, the name would be better more generalized to allow for that later, so instead of RenderScale or RenderScaleContext it could be RenderContext, allowing for future extension.
However, I did some exploration here, and I don't see anything we could possibly add here that are "facts" about the display that would interest layouts. I investigated: touch screens, screen rotations, LCD pixel order, etc, none of which I think is something layouts need or should know about.
So, I'm happy to keep its name scale specific (RenderScale or RenderScaleContext) but also fine to make it more generic (just in case).
| * it may be resized beyond its preferred size to fill whatever space is assigned | ||
| * to it. | ||
| */ | ||
| class StackPaneLayout implements Layoutable { |
There was a problem hiding this comment.
the intent is to make it public eventually, right?
There was a problem hiding this comment.
Yes, that's the end goal :)
| private static final Callback<Layoutable, Insets> MARGIN_LOOKUP = child -> getMargin((Node) child); | ||
| private static final Callback<Layoutable, Pos> ALIGNMENT_LOOKUP = child -> StackPane.getAlignment((Node) child); | ||
|
|
||
| private final StackPaneLayout stackPaneLayout = new StackPaneLayout(MARGIN_LOOKUP, ALIGNMENT_LOOKUP); |
There was a problem hiding this comment.
would it make more sense to make StackPaneLayout an abstract class instead of using callbacks?
There was a problem hiding this comment.
There are multiple ways to go about it; the reason the callbacks exist is that we need to access/track child constraints. Options include:
- Just provide callbacks to get at the constraints (the callbacks already existed in
StackPane, and the solution doesn't look to bad) - Instead of delegating this functionality back to the owner, we could make this accessible on the
Layoutableinterface (add constraint get/set methods) -- such methods would however introduce new API onNodeso I've left them out of this proposal (their implementation could just store them in the properties map, so trivial implementation, just more formalized then usinggetProperties)- Special constraint methods could also live in a subinterface (to keep them separate as they're a bit specific), so you could have
Constrainable->Layoutable->Measurable.
- Special constraint methods could also live in a subinterface (to keep them separate as they're a bit specific), so you could have
- Expose
getPropertieson theLayoutableinterface; this would be API compatible, but would put a generic method onLayoutablethat seems out of place
The reason I wouldn't make it abstract is that these callbacks will be optional. They are primarily needed for the original heavy-weight containers (ie. StackPane) to provide the full functionality they offered before. However, for embedded use, you can drop these providers if you don't need per-child constraints. Ideally, you can then just write: StackPaneLayout.of(child1, child2, child3) or:
StackPaneLayout.of(
child1,
HBoxLayout.of(
VBoxLayout.of(
child2, child3
),
VBoxLayout.of(
child4, child5
)
)
);
There is a gap still however, which we should address: constraints can only be put on Nodes in this proof of concept; if I wanted to put a stackpane-related constraint on the HBoxLayout above, I can't as it isn't a Node.
There was a problem hiding this comment.
yep, one more reason for the constraints to be a part of the layout and not the node.
anyway, callbacks are fine (only one static pointer added), especially if they are optional.
There was a problem hiding this comment.
yep, one more reason for the constraints to be a part of the layout and not the node.
This was decided long ago already though. Changing this now will break existing applications, so I think the clean path forward is to keep constraints with the child. This also means children can move between containers without losing constraint information, and that containers don't need to have clean-up for child constraints.
|
|
||
| @Override | ||
| public double minWidth(double height) { | ||
| // TODO pre-existing bug, insets not snapped anywhere |
There was a problem hiding this comment.
probably not a bug: these methods should not return snapped values (as they might come from properties). the snapping should be done by the caller.
There was a problem hiding this comment.
I think it is a bug; other containers (including the "big" ones like HBox/VBox) do snap these.
I think it would lead to subtle issue when mixing snapped/unsnapped content as well: for an unsnapped container, you can provide unsnapped sizes, but a snapped one should include the space it needs to do correct snapping (and not rely on the parent container to also be snapped which is why this problem is hidden usually now).
So if a StackPane has a child of 50 pixels wide, and insets of 0.6 then minWidth(-1) should return:
- 0.6 + 50 + 0.6 = 51.2 (unsnapped)
- 1 + 50 + 1 = 52.0 (snapped)
When placed inside a snapped container, both will ceil() to 52, but if StackPane is snapped and its container isn't, it would get only 51.2 pixels assigned to it, but it will still try to place things at pixel offsets, meaning 0.8 pixels of space (or border decoration) would get clipped.
Also I think that if you use a StackPane as root for a Scene (quite common) which doesn't do snapping of its own, and give it insets (not uncommon) you may find that the Window is one pixel too small (in either or both directions). Usually this is unnoticable as it just crops one pixel of empty space, but it could show up as a subtle difference between the left/top and right/bottom spacing.
There was a problem hiding this comment.
makes sense.
so the rule should be - if the value comes from a property it can (and should) not be snapped (because set == get == property.get), but if there is one or more entities involved then we better snap each constituent.
There was a problem hiding this comment.
Yeah, the user can set whatever value they want. So if they spacing to 0.7 or Insets to some non-aligned values, that should be returned as well.
In calculations though they need to follow the snapping rules (snapSpace in this case). Some containers will even cache these values (in a separate field, not the property) to avoid snapping each time, or calculate them once at the start of layoutChildren.
| } | ||
|
|
||
| @Override | ||
| public RenderScaleContext getRenderScaleContext(Scene scene) { |
There was a problem hiding this comment.
minor: would it make sense to combine Snapper and RenderScaleContext -> RenderContext?
There was a problem hiding this comment.
I think it is best not to (I went back and forth a lot on this while building it). The reason I think we're better off this way is that I feel that providing a Layout with render scale information is a better fit than providing it with a way it should adjust its calculations (layouts should be able to determine themselves how they want to deal with device pixels).
Where render scale is a simple fact of the rendering surface (ratio of logical to device pixels), snapper is just a potential way to deal with that.
I also think (when this becomes public API) that RenderScaleContext or RenderScale is a nicer API to have users deal with than Snapper. Snapper can still become public API if you feel that we should provide this to Layout creators so they don't have to roll their own -- it is just not required for a minimal implementation.
There was a problem hiding this comment.
this makes sense. I would also like to learn what other people think.
I checked out The API for Any positions that are not occupied (or are not for managed children as layouts don't deal with those) should be set to |
yes, that's my point - it will help identify which code needs to be extracted into the common class(es). the main danger here, I think, is sometime down the road we might discover that the API needed to be different because of some reason. this PR is going to marinate for a while I am sure, so it's up to you. edit: also, |
Yeah, I understand that last one. I can add it when you're ready to start testing -- I just need to move a few more functions from Region to LayoutSupport then (to avoid code duplication). The end game is that almost all of those functions will be moved there anyway -- only the protected/public stuff can't be touched. |
given our current PR backlog, it might take a while to review this one, so you have some time. I would suggest to include the BorderPane because it will be easier to come up with nested layouts. In parallel, you might want to extend the test coverage by adding tests that deal with Layoutable, and also check if we are missing interesting scenarios. |
Maran23
left a comment
There was a problem hiding this comment.
really like the concept.
Some initial high level comments, will review this in more detail later (count me in as reveiwer for this one!)
| double snapPositionX(double value); | ||
| double snapPositionY(double value); | ||
| double snapSpaceX(double value); | ||
| double snapSpaceY(double value); |
There was a problem hiding this comment.
could we perhaps have just snapPositionX/Y? Since snapSpaceX/Y is always doing the same, I don't see any point to have both (and I already disliked that in the current Region implementation)
There was a problem hiding this comment.
It's possible, but they do have distinct purposes:
- Position for snapping coordinates (x/y)
- Rounded to align controls to the closest possible display position
- Space for snapping "empty" areas (borders, spacing, margins)
- Rounded because they don't display important content
- Size for snapping "content" areas (text, graphics)
- Important, we don't want a text character or icon to be truncated by one pixel
So the type of snap function you use also tells you something about what you're snapping (and if you're passing a width of a piece of text to position or space, then that's a clear bug).
| * Turns a {@link RenderScaleContext}'s raw scale factors into the actual snapping | ||
| * operations used by layout math. | ||
| */ | ||
| public interface Snapper { |
There was a problem hiding this comment.
Really like the idea of the Snapper.
What I would really like to see documented is what values developers should snap.
Maybe we could add all the information we gathered over the years here.
So the conclusion of the mailing list entries, #1948, #1111 (maybe even revive this one after) and there are probably more.
Especially: Snap only final values once (before they are returned or used as x/y/w/h (If I understood that right).
There was a problem hiding this comment.
Yeah, we can add documentation here, just like how Measurable explains the bias system a bit.
Snap only final values once (before they are returned or used as x/y/w/h (If I understood that right).
That's probably best indeed; it depends on what's using those values again whether or not the snapping proved important or not (often the value gets resnapped again, depending on the container, but you shouldn't rely on that).
I also discovered a slight bug in how ceil works. We shouldn't subtract 1 ulp from the values, as 1 ulp (at Double.MAX_VALUE) can be a huge number. I was wrong when I implemented that (although it works for most "normal" values).
Instead I propose that we subtract 1 millionth of a pixel. At Double.MAX_VALUE that rounds to 0, while at more reasonable values it will remove any slight floating point errors that could cause a small 1 pixel misalignment.
| availableHeight - top - bottom; | ||
| alt = computedBoundedHeight(snapper, child, fillHeight, contentHeight); | ||
| } | ||
| return left + snapper.snapSizeX(child.minWidth(alt)) + right; |
There was a problem hiding this comment.
What I was always wondering here (since this is the same as in Region: Why do we not snap the final value? We especially would also save some cycles if we do not snap intermediate values and snap them again later. Same on the other methods - maybe something to improve later?
There was a problem hiding this comment.
You are right that the final value should be snapped, although in this case the error will be tiny (I think it snaps left and right so at least we're not summing snapped and unsnapped values here).
However, in the interest of proving that this PR doesn't have any functional changes, I didn't do this (slight errors do make some tests fail and they'd need adjusment).
For similar reasons I haven't switched Math.round to Math.rint in this PR, even though we really should do that soon (Math.round will mess up large double values, and does a conversion to long that we really don't need).
There was a problem hiding this comment.
There was a discussion about snapping only final values or also intermediate values, but I can't recall where. Maybe it was in #445.
There was a problem hiding this comment.
I don't mind doing a 2nd pass fixing these small things. I can mark them with a TODO and then fix in another PR while adjusting the small test deviations that are likely to occur then. Things I'm already aware of:
- StackPane (and BorderPane) not snapping the insets (major, adding snapped + unsnapped)
- The compute methods not doing a final snap when adding several snapped values together (minor)
- Some compute methods taking shortcuts assuming unbiased calculations (medium)
- Use of
roundinstead ofrintwhich doesn't do well with values beyond thelongrange (minor) - Use of
Math.downorMath.ulpinScaledMath.ceil-- this should be a fixed epsilon (like 1 millionth of a pixel) (minor)
I can also consolidate all the computeChildMin/Pref/MaxAreaWidth/Height methods into two methods (I did this for my own layout) with the same semantics. It looks something like this:
default double computeSpan(SizeQuery query, Measurable child, double baselineComplement, Insets margin, double extent, boolean fillExtent) {
boolean usesBaseline = baselineComplement != -1 && orientation() == Orientation.VERTICAL;
if(usesBaseline) {
double baseline = child.getBaselineOffset();
if(baseline != BASELINE_OFFSET_SAME_AS_HEIGHT) {
return baseline + baselineComplement;
}
}
double dependentExtent = -1;
if(extent != -1 && child.getContentBias() == cross().orientation()) { // span depends on cross span
double areaExtent = baselineComplement != -1 && orientation() == Orientation.HORIZONTAL && child.getBaselineOffset() == BASELINE_OFFSET_SAME_AS_HEIGHT
? extent - baselineComplement : extent;
dependentExtent = cross().computeDependentExtent(child, margin, areaExtent, fillExtent);
}
return margin(margin)
+ snapSize(query.compute(this, child, dependentExtent))
+ (usesBaseline ? baselineComplement : 0);
}
default double computeDependentExtent(Measurable child, Insets margin, double areaExtent, boolean fillExtent) {
double contentExtent = areaExtent - margin(margin);
return fillExtent
? snapSize(boundedSize(min(child, -1), contentExtent, max(child, -1)))
: snapSize(boundedSize(min(child, -1), pref(child, -1), Math.min(max(child, -1), contentExtent)));
}
Basically a single function (computeSpans) that does what the computeChild* methods do. I'm not entirely sure it will fit well in JavaFX or if it is worth it. But for me the above is alot easier to maintain (the code is equivalent even though it has a bit of a different shape).
There was a problem hiding this comment.
There was a discussion about snapping only final values or also intermediate values, but I can't recall where. Maybe it was in #445.
Intermediate values don't need snapping, and, if you use snapSize (which uses ceil) this can even be detrimental as a tiny float errors can then get rounded up multiple times (10.00000000000001 -> 11). The snap functions which do rounding are a lot less dangerous and can basically be applied as often as you want (although best only for initial and final values).
nlisker
left a comment
There was a problem hiding this comment.
I took a quick look. I assume some of my comments are on code that was left as-is, so you can ignore them.
| * the minimum width should be based on. For a horizontal or null content-bias | ||
| * the caller should pass in -1. |
There was a problem hiding this comment.
I haven't looked much at the original code, but is there a way to not give the caller a chance to pass the wrong value?
For example, if this method checks itself the content bias, and if it's horizontal it treats the input as -1.
Usually when you want the user to use a method in some way, it's best to force it. So, if a method says "the super method must be called first", one way of doing it would be:
void calc() {
super.calc();
finish();
}
abstract void finish();and the user doesn't need tp implement calc() themselves.
There was a problem hiding this comment.
The caller has some control here based on its own content bias. For example, an HBox containing both horizontal and vertically biased controls has to pick one or the other (it will favor horizontal). For the children that were vertically biased, that means they should do a normal calculation -- if such children would internally decide that because they have a bias they should first calculate the other axis, then the layout wouldn't be correctly horizontally biased overall.
Also, callers may not have the information of the size of the other axis (or deliberately omitted it due to their own bias), or may want to make adjustments to the value queried of a child (ensuring it is within min/max range, or filling it out with a larger value if filling is allowed). A biased control on its own just doesn't have enough context to decide what the other axis value should be for a biased calculation.
This is also a pretty fundamental part of any Node that implements the computeMin/Pref/Max/Width/Height methods -- it can't be changed now to work differently even if we wanted to.
There was a problem hiding this comment.
Also, the "helper" computeChildMin/Pref/MaxAreaWidth/Height methods basically are what you are suggesting -- they take care of handling this correctly, it just can't be baked into, say, all Region subtypes directly.
| * If {@link #maxWidth(double)} is lower than this number, {@code minWidth} takes | ||
| * precedence. |
There was a problem hiding this comment.
Is this guaranteed or a requirement?
There was a problem hiding this comment.
As it depends on the cooperation of whoever is reading these values and is later basing a resize call on them, we can't guarantee anything here. Is it more that a correctly implemented layout should respect these values in that specific order, and that it should never call resize with values that are out of range -- it is however not so strict that a control could throw an exception if values are out of range -- there will just be some clipping or dead space.
So I think "specification" or "requirement"? What would you call it? :)
The above is basically a copy of the original documentation, but as long as we are not adding new requirements or specifications that didn't exist before, we can adjust the wording.
There was a problem hiding this comment.
I was thinking that an @implSpec tag would be appropriate for all the requirements from an implementer of these methods (here and maybe in Layoutable, haven't look deep there yet). This also relates to the previous comment ("the caller should pass in -1"). It might require more explanation as to what the user is expected to implement.
| Bounds getLayoutBounds(); | ||
|
|
||
| /** | ||
| * If this element is resizable, sets its layout bounds to the specified |
There was a problem hiding this comment.
What is an element? A Node? A Layoutable?
There was a problem hiding this comment.
Node would be too specific, as Layoutable can be implemented by things other than nodes (like StackPaneLayout but not limited to layouts either). So Layoutable would be the most accurate here, but the neutral "element" I think reads a bit better than repeating Layoutable here every time.
| default void resizeRelocate(double x, double y, double width, double height) { | ||
| resize(width, height); | ||
| relocate(x,y); | ||
| } |
There was a problem hiding this comment.
Is there a reason to override this? This looks more like a convenience method that would have otherwise be final.
There was a problem hiding this comment.
Yes, there is a reason you may want to override this. While for Nodes you don't need to as relocate doesn't require children traversal (a Node just sets its layout translation which applies to all its children) -- non-Node layoutables however don't have a layout translation, and so relocate must change the positions of all its children to match the passed in position.
For this reason you may want to override resizeRelocate to make this updating of children one pass instead of two passes (ie. resize first adjusting all children, then relocate adjusting all children again).
See how this is done in StackPaneLayout.
| if (max == Double.MAX_VALUE) { | ||
| return max; | ||
| } |
There was a problem hiding this comment.
This is a short-cut that is in the original code, but actually breaks the contract for biased controls (computeChildMaxAreaHeight also has it).
This PR shows how virtual layouts could be implemented, as discussed on the mailinglist: https://mail.openjdk.org/archives/list/openjfx-dev@openjdk.org/thread/PLNSQ3ZI63AVKEFKNT5GT6WHAD2GYMPC/
It would work by making new non-Node containers called Layouts which can contain a mix of either Nodes or other Layouts. Allowing Layouts to nest makes it possible to create a substructure similar to how nesting StackPane/HBox/VBox etc works today. An example is a simple control that has a graphic with a title and subtitle stacked vertically next to it:
For a control to model this, it would need a VBox containing the Title and Subtitle, and an HBox containing the Graphic and the VBox. The two containers are heavy-weight Nodes and this incurs sufficiently large memory and performance penalties that most standard JavaFX controls will opt to instead roll their own layout code to avoid paying the cost for these.
With virtual layouts, the control could make use of non-Node containers. The control would add its three children (Graphic, Title and Subtitle) as direct children for display in the scene graph, but would offload their positioning to a virtual layout. This roughly looks like this:
Note: this PR does not offer the
VBoxLayoutorHBoxLayout; to keep this PR minimal,StackPanewas chosen as an example layout.Changes made:
Layoutableinterface.Measurableinterface to cleanly segregate the concerns of the measuring pass and the actual layout passNodehas been retrofitted to implementLayoutable(no new methods or API changes)StackPaneLayout, containing theStackPanelayout algorithm; this uses theLayoutSupporthelper containing most of the (package private) helper logic ofRegionwith small adjustment to acceptLayoutables instead ofNodesStackPanehas been adjusted to make use of theStackPaneLayoutScenechanges)No test changes, all test pass as before, including the
StackPaneTest.The
SnappersystemIn order to allow the layout helpers to do correct render scale aware calculations, the
Snappertype has been created. It holds the typicalsnapSizeX/Yfunctions but without needing to a do a look-up of the render scale on each call. As such it is likely a performance preserving change. There is usually only oneSnapperfor an entire scene graph. The snapper with the current render scale that is in use for aWindowis accessible via theRegionsnapper()method.Because
Snappers are reusable, there are some optimizations here as well. For render scale 1.0/1.0 there is a snapper that does no calculations, only rounding or ceiling. For all other render scales, the division is avoided by pre-calculating the reciprocal.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2241/head:pull/2241$ git checkout pull/2241Update a local copy of the PR:
$ git checkout pull/2241$ git pull https://git.openjdk.org/jfx.git pull/2241/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2241View PR using the GUI difftool:
$ git pr show -t 2241Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2241.diff
Using Webrev
Link to Webrev Comment