Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Mon, 17 Oct 2022 16:11:32 GMT, Andy Goryachev wrote:
>> What I meant is you can add it in Node.getProperties() and not to add a
>> pointer to every Node everywhere. If it's used, the actual properties will
>> be created in a few containers.
>
> private static final Object KEY = new Object();
>
> public final ReadOnlyBooleanProperty shownProperty() {
> Object x = getProperties().get(KEY);
> if (x instanceof ReadOnlyBooleanProperty p) {
> return p;
> }
> ReadOnlyBooleanProperty p = ... // create
> getProperties().put(KEY, p);
> return p;
> }
Thanks, I understand, what I find odd is that this would be the first property
in `Node` to be stored this way. If this was so important, then why isn't this
done for other properties? There seem to be sufficient candidates for this
approach to "slim" down `Node` (infrequently used properties like `Subscene`,
`id`, `style`, `blendMode`).
There's even a `MiscProperties` objects which is described as "Misc Seldom Used
Properties" that is created only when needed with these properties:
private LazyBoundsProperty boundsInParent;
private LazyBoundsProperty boundsInLocal;
private BooleanProperty cache;
private ObjectProperty cacheHint;
private ObjectProperty clip;
private ObjectProperty cursor;
private ObjectProperty depthTest;
private BooleanProperty disable;
private ObjectProperty effect;
private ObjectProperty inputMethodRequests;
private BooleanProperty mouseTransparent;
private DoubleProperty viewOrder;
Let's see what others think before I change this. It does sound like a
reasonable approach though.
-
PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]
On Mon, 17 Oct 2022 17:07:40 GMT, Nir Lisker wrote:
>> It would be suitable to align with our existing doc comment in other
>> classes, for example as here,
>>
>> 1.
>> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Point2D.java#L374
>> 2.
>> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java#L103
>> 3.
>> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/BoundingBox.java#L169
>>
>>
>> I would recommend to use as in Point2D,
>>
>>
>> /**
>> * Indicates whether some other object is "equal to" this one.
>> *
>> * @param obj the reference object with which to compare
>> * @return true if this Point3D is the same as the obj argument; false
>> otherwise
>> */
>
> I think that we should explain what makes 2 objects equal. Even if we don't
> explicitly name the methods used for comparison, we could say "2 points are
> equals if their coordinates are equal".
>
> By the way, I have https://bugs.openjdk.org/browse/JDK-8226930 assigned to go
> over some dubious equals/hashcode implementations, in case you want to
> delegate the task.
Agreed, mentioning equality criteria sounds good to me too. How does this look ?
/**
* Indicates whether some other object is "equal to" this one.
* Two instances of Point3D are equal if the return values of their
* {@code getX}, {@code getY}, and {@code getZ} methods are equal.
*
* @param obj the reference object with which to compare
* @return true if this Point3D is the same as the obj argument; false
otherwise
*/
-
PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: 8294722: FX: Update copyright year in docs, readme files to 2023
On Mon, 17 Oct 2022 09:36:55 GMT, Ambarish Rapte wrote: > Update Copyright year in these 3 doc files to 2023. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.org/jfx/pull/917
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]
On Mon, 17 Oct 2022 15:24:48 GMT, Ambarish Rapte wrote: >> ...correction, found in toString() as well. > > It would be suitable to align with our existing doc comment in other classes, > for example as here, > > 1. > https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Point2D.java#L374 > 2. > https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java#L103 > 3. > https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/BoundingBox.java#L169 > > > I would recommend to use as in Point2D, > > > /** > * Indicates whether some other object is "equal to" this one. > * > * @param obj the reference object with which to compare > * @return true if this Point3D is the same as the obj argument; false > otherwise > */ I think that we should explain what makes 2 objects equal. Even if we don't explicitly name the methods used for comparison, we could say "2 points are equals if their coordinates are equal". By the way, I have https://bugs.openjdk.org/browse/JDK-8226930 assigned to go over some dubious equals/hashcode implementations, in case you want to delegate the task. - PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]
On Fri, 14 Oct 2022 15:08:24 GMT, Douglas Held wrote: >> Not strictly enforced, but adding a blank line does aid readability (of the >> source code...the generated docs don't care). > > If I were to add empty lines before JavaDoc tags such as @param etc, then for > consistency I think I should do it for the whole class, where they are not > present. > > Note, on line 32: **// PENDING_DOC_REVIEW of this whole class** > Maybe we should first decide whether we are scrubbing all doc for the whole > class, or making a minimal change to the obviously incorrect information. My > feeling is now tending toward the latter. > > It could very well be that I misunderstood the comment by nlisker. We just tend to separate the words section from the tags section with an empty line in the source code. You don't have to do it for the whole class (unless you want to), I just suggest to do it here since you're making a change anyway. - PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Fri, 14 Oct 2022 23:01:48 GMT, Andy Goryachev wrote:
>> I agree with your first point, but I think limiting any changes to `Node`
>> because it would add another field is not a good enough argument. It would
>> depend on its merits.
>>
>> The second point, there is much more to the `shownProperty` then you might
>> think at first glance. In combination with `when`, it offers a standard way
>> to manage life cycles for controls. It is a very natural fit to tie the
>> lifecycle of bindings and listeners to whether a control is actually
>> currently shown. You may think that a Listener helper class would be just
>> as flexible, but it isn't. The helper is not available everywhere (unless
>> you propose to integrate it into Node), which means when constructing
>> complicated UI's which may have smaller groups of controls that are created
>> by other classes, methods or factories, that you may have to pass along an
>> instance of your helper to all these to make sure they register their
>> bindings and listeners with it (or perhaps to link multiple Listener helper
>> classes together in some kind of hierarchy, that matches the control
>> groupings that might have separate life cycles).
>>
>> The `shownProperty` does not have this problem. Since you can tie the
>> lifecycle to your own small piece of the UI (ie, a factory that creates a
>> `BorderPane` with several controls, can tie all these to the lifecycle of
>> the `BorderPane`). When this `BorderPane` is just a piece of UI used as
>> part of a larger whole, its lifecycle still works as expected -- if it is
>> not shown, its listeners disappear. If the whole window is destroyed, then
>> the same happens. If however the Pane next to it is destroyed, only their
>> listeners disappear. With a helper system, you'd need to coordinate this
>> somehow, perhaps having that `BorderPane` return its helper, so the large UI
>> can tie it to its helper, so when the main helper is called to unsubscribe,
>> that all its child helpers are also unsubscribed...
>>
>> Another advantage is that a listener does not have to start working right
>> away. When setting up a new pane of controls, many subscriptions may need
>> to be made. When using `when(control::shownProperty)` these listeners don't
>> immediately become active (potentially while still constructing and
>> initializing your control). Only when the control is fully constructed, and
>> added to an active Scene and Window do all the listeners become active.
>> This can be important when listening to external properties that change
>> often and may even change during construction.
>>
>> In ReactFX it the `shownProperty` was deemed important enough to go one step
>> further and even create a separate method on what would be `ObservableValue`
>> called `conditionOnShowing`.
>>
>> In short, `when` and the `shownProperty` is basically used every time you
>> create a binding or add a listener between two properties that have a
>> different lifecycle, like between a model and a control. This completely
>> removes the responsibility of having to track these bindings or listeners
>> separately; it becomes automatic. Subscribing all at once when it becomes
>> first shown becomes automatic (and you immediately get the first value),
>> unsubscribing when it goes invisible is automatic, and resubscribing if a
>> control becomes shown again is completely transparent.
>>
>> In my projects, I've completely eliminated memory leaks without a single
>> call to `removeListener` or `unbind` anywhere.
>> There are no explicit weak listeners (and not just because they're
>> incredibly cumbersome to use), nor is there anywhere where subscriptions or
>> bindings are being tracked (aside from the `when` construct). I'm very
>> picky about memory leaks, so I have a Scene scanner, that maintains weak
>> references to all Nodes at all times, and warns me if any are not getting
>> GC'd after not being part of a Scene for a while. In 99% of the cases, I
>> forgot a `when`, where in plain JavaFX you'd be forced to use a weak
>> listener.
>>
>> The simple rule to follow is, whenever you bind or listen to or on a
>> property, if they have different lifecycles, then use a `when`, otherwise
>> bind or listen directly.
>>
>> Some examples of how it is used:
>>
>> Below is a very common pattern when binding any property that has a
>> different lifecycle than its target, this is one of many:
>>
>> longLivedModel.selectedItem
>> .when(this::shownProperty) // this = BorderPane, and the code is
>> part of its constructor)
>> .addListener((obs, old, current) -> {
>> // code that starts a transition from the previous item to the
>> next item
>> }
>> );
>>
>> Below some binding examples where several properties are bound to long lived
>> versions:
>>
>> ObservableValue shown = pane.shownProperty();
>>
>>
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]
On Fri, 14 Oct 2022 15:15:27 GMT, Douglas Held wrote:
>> Looking at the JavaDoc in the rest of the class, the {@code Point3D} tags
>> are not used outside of these two methods hashCode() and equals(). I'd
>> propose actually that we remove them, for consistency with the rest of the
>> JavaDoc in this class.
>
> ...correction, found in toString() as well.
It would be suitable to align with our existing doc comment in other classes,
for example as here,
1.
https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Point2D.java#L374
2.
https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java#L103
3.
https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/BoundingBox.java#L169
I would recommend to use as in Point2D,
/**
* Indicates whether some other object is "equal to" this one.
*
* @param obj the reference object with which to compare
* @return true if this Point3D is the same as the obj argument; false
otherwise
*/
-
PR: https://git.openjdk.org/jfx/pull/913
RFR: 8294722: FX: Update copyright year in docs, readme files to 2023
Update Copyright year in these 3 doc files to 2023. - Commit messages: - copyright update in docs Changes: https://git.openjdk.org/jfx/pull/917/files Webrev: https://webrevs.openjdk.org/?repo=jfx=917=00 Issue: https://bugs.openjdk.org/browse/JDK-8294722 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/917.diff Fetch: git fetch https://git.openjdk.org/jfx pull/917/head:pull/917 PR: https://git.openjdk.org/jfx/pull/917
Re: RFR: 8295324: JavaFX: Blank pages when printing [v2]
> This fixes a race condition between application and 'Print Job Thread' > threads when printing. > > The race condition occurs when application thread calls `endJob()`, which in > effect sets the `jobDone` flag to true, > and when the 'Print Job Thread' thread was in the `synchronized` block in > `waitForNextPage()` at that time. > The 'Print Job Thread' thread checks `jobDone` flag after exiting the > `synchronized` block and, if it is true, skips the last page. > > In this fix, not only the `jobDone` is checked, but also that there is no > other page to be printed. > It was also needed to introduce a new flag 'jobCanceled', to skip the page if > the printing was canceled by 'cancelJob()'. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8295324: Fix race condition in junit test - Changes: - all: https://git.openjdk.org/jfx/pull/916/files - new: https://git.openjdk.org/jfx/pull/916/files/fa47d169..fdec73d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=916=01 - incr: https://webrevs.openjdk.org/?repo=jfx=916=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/916.diff Fetch: git fetch https://git.openjdk.org/jfx pull/916/head:pull/916 PR: https://git.openjdk.org/jfx/pull/916
