-
Notifications
You must be signed in to change notification settings - Fork 549
8372530: Easier placement of windows with positioning anchor #1986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| * | ||
| * @since 26 | ||
| */ | ||
| public static final class Anchor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a record too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it could, but I don't like the boolean parameter that it would expose. Boolean parameters are usually not a good idea, as they lead to "boolean blindness" at the call site. I've moved this entire class to javafx.geometry, as it might be useful for other code in the future, and I don't want to proliferate two nested versions of an anchor point (one in Stage, the other in PopupWindow).
| 0, 0); | ||
| } | ||
|
|
||
| // Give subclasses a chance to adjust the window bounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this Enhancement, but I wonder if the centerOnScreen line before could also be rewritten using this system (at one point). Now, it looks like he might be centering the Window, and then fix the bounds to something else, wasting some time.
| * @throws NullPointerException if {@code anchor} is {@code null} | ||
| * @since 26 | ||
| */ | ||
| public final void show(double anchorX, double anchorY, AnchorPoint anchor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make more sense to move these new methods to Window, so they can also work for the Popup and its hierarchy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PopupWindow works differently, it has a anchorLocation property which is a bit limited. We also couldn't move PopupWindow.AnchorLocation anywhere else (as this would be a breaking change), which would make the signature of the show() method very awkward: On Stage.show(x, y, PopupWindow.AnchorLocation) you'd have a parameter that is declared on an unrelated class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is unfortunate.
Can we do anything to make the positioning of Popups and its hierarchy easier? Maybe add another show() with a ClampPolicy argument?
Use case: I want to open a PopupWindow below the owner node, but: if the popup cannot be placed there because the whole thing is too close to the bottom of the screen, I want the popup to be above the node, and not on top of it as it works currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've redesigned the feature a bit, and found that we can indeed make this compatible with PopupWindow. You now have the option to specify PopupWindow.anchorPolicy, which defaults to the current behavior, but can also be specified to work as you describe (using AnchorPolicy.AUTO).
| * | ||
| * @since 26 | ||
| */ | ||
| public enum ClampPolicy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call it RelocationPolicy?
|
I will test and review this. Especially test with the scenarios where I used the workaround described in the mailing list. |
|
|
||
| @Override | ||
| public void centerOnScreen() { | ||
| relocationRequest = null; // cancel previous relocation request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, with the new anchor system, a center on screen can easily be achieved as well? I wonder if we could also deprecate this method instead. But not sure if I understand it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the anchor is always a point on the window being shown, not on the screen. All anchor APIs require you to specify an explicit screen location, and the window is then moved so that the anchor coincides with that screen location. So to emulate centerOnScreen(), you'd have to manually get the screen bounds, and then calculate the center.
This enhancement allows
Stageto be placed on the screen similar to a popup window, where a user-specified positioning anchor defines a point on the stage that should coincide with a given location on the screen. For this purpose, the following new methods are added toStage:AnchorPoint
AnchorPointis a point that is either specified in absolute coordinates, or relative to the stage:For example, a stage that sits flush with the bottom-right corner of the screen can be shown as follows:
AnchorPolicy
AnchorPolicycontrols how the anchor may be adjusted when the preferred placement doesn't fit within the screen bounds:FIXED: always use the provided anchor; only adjust the resulting position to fit within the screen.FLIP_HORIZONTAL: if the preferred placement violates horizontal constraints, try a horizontally flipped anchor (e.g. top-left to top-right) before falling back to the original anchor.FLIP_VERTICAL: likewise for vertical constraints.AUTO: automatically choose the most suitable flip:if only horizontal constraints are violated, acts like
FLIP_HORIZONTAL;if only vertical constraints are violated, acts like
FLIP_VERTICAL;if both are violated, try a diagonally flipped anchor (both axes) and pick the placement that requires the least adjustment.
This is useful for popup-like behavior where you have a preferred "opening direction", but want the window to flip to the opposite side of the reference point when there isn’t enough space (e.g. "prefer below, but open above if below doesn’t fit").
PopupWindow support
The new
PopupWindow.anchorPolicyproperty adds the same "flip the anchor when it would go off-screen" capability for popups. This complements the existingPopupWindow.anchorLocation/PopupWindow.autoFixbehavior: applications can keep usingPopupWindow.anchorLocationto express the preferred anchor, whilePopupWindow.anchorPolicycontrols how the popup may flip that anchor as a fallback when the preferred placement wouldn't fit within screen bounds.Edge constraints
screenPaddingspecifies per-edge padding relative to the screen, expressed as anInsets:Enabled constraints effectively shrink the usable screen area by the given insets.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1986/head:pull/1986$ git checkout pull/1986Update a local copy of the PR:
$ git checkout pull/1986$ git pull https://git.openjdk.org/jfx.git pull/1986/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1986View PR using the GUI difftool:
$ git pr show -t 1986Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1986.diff