Re: RFR: 8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true
On Thu, 1 Dec 2022 17:03:03 GMT, Karthik P K wrote:
> Cause: When slider is dragged for first time after tooltip appears,
> setOnMousePressed event was not invoked, hence dragStart was null in the
> subsequently invoked event handler (setOnMouseDragged).
>
> Fix: Initialized dragStart in initialize method.
>
> Test: Added system test to validate the fix.
>
> Note: Initializing dragStart in layoutChildren method as suggested in the bug
> was causing side effects. Hence initialized dragStart in initialize method.
Providing few quick comments about the test.
tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
line 78:
> 76: }
> 77: }
> 78:
`main()` method is not required, can be removed.
tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
line 84:
> 82: }
> 83:
> 84: private void dragSliderAfterTooltipDisplayed(int dragDistance,
> boolean xIncr) throws Throwable {
`boolean xIncr` is not required for the test. It can be removed along with it's
use on line 101.
tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
line 89:
> 87: Util.runAndWait(() -> {
> 88: robot.mouseMove((int)(scene.getWindow().getX() + scene.getX()
> + SCENE_WIDTH/2),
> 89: (int)(scene.getWindow().getY() + scene.getY() +
> SCENE_HEIGHT/2));
The x and y position calculation should consider the sliders layout as well.
I think it should be as:
robot.mouseMove((int)(scene.getWindow().getX() + scene.getX() +
slider.getLayoutX() + slider.getLayoutBounds().getWidth()/2),
(int)(scene.getWindow().getY() + scene.getY() +
slider.getLayoutY() + slider.getLayoutBounds().getHeight()/2));
and similar change on line 102.
Line 105 would get removed when removing `xIncr`
tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
line 92:
> 90: });
> 91:
> 92: Thread.sleep(3000); // Wait for tooltip to be displayed
This `sleep` can be removed, The sleep of 1000 ms on line 85 should be
sufficient.
I ran by removing this sleep on Mac, Test ran well.
Please check on other platforms.
-
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.org/jfx/pull/965
RFR: 8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true
Cause: When slider is dragged for first time after tooltip appears, setOnMousePressed event was not invoked, hence dragStart was null in the subsequently invoked event handler (setOnMouseDragged). Fix: Initialized dragStart in initialize method. Test: Added system test to validate the fix. Note: Initializing dragStart in layoutChildren method as suggested in the bug was causing side effects. Hence initialized dragStart in initialize method. - Commit messages: - Correct file permissions - Correct line endings - Merge branch 'master' into slider_npe_fix - SliderSkin NPE fix Changes: https://git.openjdk.org/jfx/pull/965/files Webrev: https://webrevs.openjdk.org/?repo=jfx=965=00 Issue: https://bugs.openjdk.org/browse/JDK-8190411 Stats: 152 lines in 2 files changed: 152 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/965.diff Fetch: git fetch https://git.openjdk.org/jfx pull/965/head:pull/965 PR: https://git.openjdk.org/jfx/pull/965
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]
On Thu, 1 Dec 2022 17:38:01 GMT, John Hendrikx wrote:
>> This PR adds a new (lazy*) property on `Node` which provides a boolean which
>> indicates whether or not the `Node` is currently part of a `Scene`, which in
>> turn is part of a currently showing `Window`.
>>
>> It also adds a new fluent binding method on `ObservableValue` dubbed `when`
>> (open for discussion, originally I had `conditionOn` here).
>>
>> Both of these together means it becomes much easier to break strong
>> references that prevent garbage collection between a long lived property and
>> one that should be shorter lived. A good example is when a `Label` is bound
>> to a long lived property:
>>
>>
>> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty));
>>
>> The above basically ties the life cycle of the label to the long lived
>> property **only** when the label is currently showing. When it is not
>> showing, the label can be eligible for GC as the listener on
>> `longLivedProperty` is removed when the condition provided by
>> `label::isShowingProperty` is `false`. A big advantage is that these
>> listeners stop observing the long lived property **immediately** when the
>> label is no longer showing, in contrast to weak bindings which may keep
>> observing the long lived property (and updating the label, and triggering
>> its listeners in turn) until the next GC comes along.
>>
>> The issue in JBS also describes making the `Subscription` API public, but I
>> think that might best be a separate PR.
>>
>> Note that this PR contains a bugfix in `ObjectBinding` for which there is
>> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because
>> the tests for the newly added method would fail otherwise; once it has been
>> integrated to jfx19 and then to master, I can take the fix out.
>>
>> (*) Lazy means here that the property won't be creating any listeners unless
>> observed itself, to avoid problems creating too many listeners on
>> Scene/Window.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adjust Node
>
> - Fixed javadoc
> - Added comment for code that avoid eager instantiation
> - Changed `isShown` to use property if it is available
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1413:
> 1411: * Indicates whether or not this {@code Node} is shown. A node is
> considered shown if it's
> 1412: * part of a {@code Scene} that is part of a {@code Window} whose
> 1413: * {@link Window#showingProperty showing property} is {@code true}.
> The {@link Node#visibleProperty visibility}
I think you should choose one of the following:
`...whose {@link Window#showingProperty} is...`
`...whose {@link Window#showingProperty showing} property is...`
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1420:
> 1418: *
> 1419: * This property can also be used to start animations when the node
> is shown, and to stop them
> 1420: * when it is no longer shown.
This sentence sounds like `shownProperty()` is quite deeply connected to
animations, which it isn't. Maybe you can clarify with "For example, ..."?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1437:
> 1435: }
> 1436:
> 1437: public final ReadOnlyBooleanProperty shownProperty() {
This property probably should have been named `showing` to align it with
`Window.showing`, but the "good name" is already taken by `ComboBox` and other
controls. Just out of interest, have you considered alternatives to `shown`?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1443:
> 1441: .flatMap(Window::showingProperty); // note: the null
> case is handled by the delegate with an orElse(false)
> 1442:
> 1443: shown = new ReadOnlyBooleanDelegate(Node.this, "shown", ov);
`shownProperty()` might be used quite a lot, have you considered using a
specialized implementation that doesn't need several intermediate observable
values?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1453:
> 1451: private final ObservableValue delegate;
> 1452: private final Object bean;
> 1453: private final String name;
You could omit the `bean` and `name` fields by making this a non-static inner
class and returning `Node.this` and `"shown"` directly from `getBean()` and
`getName()` respectively. In that case, you could also make this class an
anonymous implementation of `ReadOnlyBooleanProperty`. This pattern is used
quite extensively in JavaFX, since it can potentially save a few bytes of
memory.
-
PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v30]
On Tue, 22 Nov 2022 01:40:02 GMT, Thiago Milczarek Sayao wrote: >> This cleans size and positioning code, reducing special cases, code >> complexity and size. >> >> Changes: >> >> - cached extents: 28, 1, 1, 1 are old defaults - modern gnome uses different >> sizes. It does not assume any size because it varies - it does cache because >> it's unlikely to vary on the same system - but if it does occur, it will >> only waste a resize event. >> - window geometry, min/max size are centralized in >> `update_window_constraints`; >> - Frame extents (the window decoration size used for "total window size"): >> - frame extents are received in `process_property_notify`; >> - removed quirks in java code; >> - When received, call `set_bounds` again to adjust the size (to account >> decorations later received); >> - Removed `activate_window` because it's the same as focusing the window. >> `gtk_window_present` will deiconify and focus it. >> - `ensure_window_size` was a quirk - removed; >> - `requested_bounds` removed - not used anymore; >> - `window_configure` incorporated in `set_bounds` with `gtk_window_move` and >> `gtk_window_resize`; >> - `process_net_wm_property` is a work-around for Unity only (added a check >> if Unity - but it can probably be removed at some point) >> - `restack` split in `to_front()` and `to_back()` to conform to managed code; >> - Set `gtk_window_set_focus_on_map` to FALSE because if TRUE the Window >> Manager changes the window ordering in the "focus stealing" mechanism - this >> makes possible to remove the quirk on `request_focus()`; >> - Note: `geometry_get_*` and `geometry_set_*` moved location but unchanged. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Fix View position Those iconify tests always fails for me, but it works testing manually. About the focus, `gtk_window_present` might have changed between releases, I will investigate. - PR: https://git.openjdk.org/jfx/pull/915
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]
On Fri, 2 Dec 2022 00:34:35 GMT, Kevin Rushforth wrote: > The API and docs for `ObservableValue::when` look like they are in good > shape, so I'll review them next week. > > My only overall comment is that this PR should be limited to the new API, > implementation, and tests in `javafx.base`. Although it's helpful to review > them together, the changes to `Node` to add a new `shown` property is an > interesting use case that should stand on its own merits and should be done > as a follow-on Enhancement in a separate PR. Okay, that would mean removing the examples from `when` that use the `shownProperty`, and I suppose adding them back in later. The ticket was specifically worded to make it easier to deterministically manage listeners, which at least for UI controls, the `shown` property is an important part of. If you think its realistic, I can file a new ticket, split it off so hopefully both can be part of the same release? - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]
On Thu, 1 Dec 2022 17:38:01 GMT, John Hendrikx wrote: >> This PR adds a new (lazy*) property on `Node` which provides a boolean which >> indicates whether or not the `Node` is currently part of a `Scene`, which in >> turn is part of a currently showing `Window`. >> >> It also adds a new fluent binding method on `ObservableValue` dubbed `when` >> (open for discussion, originally I had `conditionOn` here). >> >> Both of these together means it becomes much easier to break strong >> references that prevent garbage collection between a long lived property and >> one that should be shorter lived. A good example is when a `Label` is bound >> to a long lived property: >> >> >> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); >> >> The above basically ties the life cycle of the label to the long lived >> property **only** when the label is currently showing. When it is not >> showing, the label can be eligible for GC as the listener on >> `longLivedProperty` is removed when the condition provided by >> `label::isShowingProperty` is `false`. A big advantage is that these >> listeners stop observing the long lived property **immediately** when the >> label is no longer showing, in contrast to weak bindings which may keep >> observing the long lived property (and updating the label, and triggering >> its listeners in turn) until the next GC comes along. >> >> The issue in JBS also describes making the `Subscription` API public, but I >> think that might best be a separate PR. >> >> Note that this PR contains a bugfix in `ObjectBinding` for which there is >> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because >> the tests for the newly added method would fail otherwise; once it has been >> integrated to jfx19 and then to master, I can take the fix out. >> >> (*) Lazy means here that the property won't be creating any listeners unless >> observed itself, to avoid problems creating too many listeners on >> Scene/Window. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust Node > > - Fixed javadoc > - Added comment for code that avoid eager instantiation > - Changed `isShown` to use property if it is available The API and docs for `ObservableValue::when` look like they are in good shape, so I'll review them next week. My only overall comment is that this PR should be limited to the new API, implementation, and tests in `javafx.base`. Although it's helpful to review them together, the changes to `Node` to add a new `shown` property is an interesting use case that should stand on its own merits and should be done as a follow-on Enhancement in a separate PR. - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v19]
On Tue, 11 Oct 2022 18:50:54 GMT, Andy Goryachev wrote:
>>> You are right, @kleopatra - this executeOnceWhenPropertyIsNonNull() is
>>> installing a listener. perhaps we should add a similar functionality to
>>> LambdaMultiplePropertyChangeListenerHandler and actually install a
>>> WeakListener instead.
>>
>> - yes, we need weakListeners (to wait for a not-null value to add the event
>> handler) _and_ remove both the weakListener and the event handlers in dispose
>> - adding functionality to the LambdaHandler sounds like a good idea, but
>> needs some thinking and should be done in a separate issue which also adds
>> delegate methods to expose the new functionality in SkinBase for use in sub
>> classes
>>
>> For this issue, you might simply inline the utility method and register the
>> listeners using skinbase api (didn't try, it's your issue :), see
>> TableRowSkin for a similar pattern
>>
>> This conditional adding of handlers/listeners is needed if we have to
>> support path properties (also f.i. when listening to selectedXX in any of
>> the skins with selectionModels as properties). Which basically should be
>> supported by base - but probably without any chance to ever get them ;)
>
> ListenerHelper - replacement for LambdaMultiplePropertyChangeListenerHandler .
> Please take a look at https://github.com/openjdk/jfx/pull/908
> Sure, but I think this should be done with a clear vision of where we're
> going, and not just as a collection of ad-hoc helper methods. There are even
> more problems that a new API should at least consider: Some skins change
> their behavior depending on the configuration of their associated control at
> the time when the skin is installed. For example, `MenuButtonSkinBase` adds a
> `MOUSE_PRESSED` handler only if the control didn't specify one for its
> onMousePressed property:
>
> ```
> if (control.getOnMousePressed() == null) {
> control.addEventHandler(MouseEvent.MOUSE_PRESSED, ...
> ```
>
> But what happens if the `onMousePressed` property is set after the skin is
> installed? Rather than a one-time check, cases like these should probably be
> a dynamic expression:
>
> ```
> when control.onMousePressedProperty().isNotNull() and skin.isInstalled()
> control.addEventHandler
> otherwise
> control.removeEventHandler
> ```
I think you meant `isNull()` instead of `isNotNull()`:
when control.onMousePressedProperty().isNull() and skin.isInstalled()
control.addEventHandler
otherwise
control.removeEventHandler
So... I was curious, and was checking to see if this can be done with fluent
bindings these days, and I can get quite close. First assume we have a
convenient property in all skins (or everything that is `SkinBase`) that
reflects whether or not the skin is currently installed or active:
BooleanProperty installed = new SimpleBooleanProperty();
You can then create a condition:
ObservableValue noMouseAndInstalled = installed
.flatMap(b -> b ? control.onMousePressedProperty() : installed)
.map(x -> false) // ---+ use case for isNotEmpty
.orElse(true);// --/
Or (if `isNotEmpty` already exists):
ObservableValue noMouseAndInstalled = installed
.flatMap(b -> b ? control.onMousePressedProperty() : installed)
.isNotEmpty();
It's then a simple matter of adding a listener to this result to either add or
remove the event handler:
installed
.flatMap(b -> b ? control.onMousePressedProperty() : installed)
.isNotEmpty();
.addListener((obs, old, active) -> {
if(active) control.addEventHandler(...);
else control.removeEventHandler(...);
});
One of the nice things here is that when `installed` is `false`, the listener
on `control.onMousePressedProperty()` is automatically removed. This is
because the `flatMap` will switch to listening to `installed`. This should
mean that it will automatically be eligible for GC when `installed` becomes
`false`.
The second nice thing is that you can do this in the constructor already, no
need to do this in the `install` callback. If you do it in the callback, then
make sure to do it before `installed` is set to `true` as otherwise the change
listener won't fire for the first change (another reason to have value
listeners...)
If this becomes a common idiom, we could introduce a short circuiting `and`
construct so the `flatMap` can be expressed easier (the short circuiting part
will ensure the onMousePressedProperty won't be observed if installed is
`false`):
installed
.and(control.onMousePressedProperty().isNotEmpty())
.addListener( ... );
-
PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v19]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294589: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/906/files - new: https://git.openjdk.org/jfx/pull/906/files/cbcc17e7..693667a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=906=18 - incr: https://webrevs.openjdk.org/?repo=jfx=906=17-18 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Thu, 1 Dec 2022 22:45:20 GMT, John Hendrikx wrote: >> just to be sure, which weak listener are you referring to? > > I didn't mean a weak listener, but a weak reference. This line creates one: > > sceneListenerHelper = new ListenerHelper(MenuBarSkin.this); > > I think a plain `new ListenerHelper()` would work correct here. If you don't > think so, I think it may be good to add a comment why the weak reference is > needed to make the clean up work correctly. I have to agree with you - it is not needed in this case. The reason I put it there was "what if Stage.hide() gets called and the whole thing disappears, perhaps we want to make sure that all listeners that were registered via `sceneListenerHelper` get disconnected. The thing is, all these listeners are registered against the scene belonging to that stage, so we are ok. - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v18]
On Thu, 1 Dec 2022 19:55:33 GMT, Andy Goryachev wrote: >> Fixed memory leak by using weak listeners and making sure to undo everything >> that has been done to MenuBar/Skin in dispose(). >> >> This PR depends on a new internal class ListenerHelper, a replacement for >> LambdaMultiplePropertyChangeListenerHandler. >> See https://github.com/openjdk/jfx/pull/908 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > 8294589: review comments @aghaisas : would you please take a look? - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Thu, 1 Dec 2022 19:47:52 GMT, Andy Goryachev wrote: >>> but instead the whole component or Pane might be removed from the scene and >>> discarded. >> >> If that happens, weak reference won't make a difference there either. >> Removing an entire branch from the Scene and not referring to it anymore >> will do correct clean-up even without a call to dispose and without the use >> of weak references. >> >> The only use case I see is that we still don't trust the Skin lifecycle to >> be adhered to and "just in case" are using weak references in case somehow >> dispose is not called. I would much prefer to see an actual reason to use >> it, and then adding a comment as to why this weak reference is needed so >> that in 6 months time we're not scratching our heads as to why a weak >> reference is needed here. > > just to be sure, which weak listener are you referring to? I didn't mean a weak listener, but a weak reference. This line creates one: sceneListenerHelper = new ListenerHelper(MenuBarSkin.this); I think a plain `new ListenerHelper()` would work correct here. If you don't think so, I think it may be good to add a comment why the weak reference is needed to make the clean up work correctly. - PR: https://git.openjdk.org/jfx/pull/906
Re: Setting graphics of a Labeled does not show the Label correctly
Are we convinced that a node can't be both a graphic and a clip, or something else? The docs for clip specify that the node is not a child in the scenegraph. On Thu, Dec 1, 2022 at 11:41 PM John Hendrikx wrote: > Adding another field doesn't seem ideal, would it be possible to > generalize the clipParent field to function for both (the ownedBy or owner > field that I suggested earlier)? > > --John > On 01/12/2022 20:26, Nir Lisker wrote: > > Michael's idea could solve the problem if it's about more than just > traversing, it needs to set rules for allowing a node to serve only 1 > logical role (or 1 role type, like clip and graphic?) at the same time. In > any case, these rules need to be decided upon before starting to work on > anything. I can do a quick fix for now that can be reverted later > if needed. From what I gather, I will need to add a graphicsParent field > like clipParent does. > > On Thu, Dec 1, 2022 at 8:47 PM Nir Lisker wrote: > >> By the way, these issues are caused by this inconsistent behavior (they >> are probably duplicates): >> >> https://bugs.openjdk.org/browse/JDK-8209017 >> https://bugs.openjdk.org/browse/JDK-8190331 >> >> The graphic of the checkbox of a CheckBoxTreeItem is not set correctly on >> the new CheckBox that is provided with the cell when virtual flow switches >> it. It might happen with other controls that use virtual flow. >> >> On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth < >> kevin.rushfo...@oracle.com> wrote: >> >>> This seems related, but somewhat tangential. A Control's "graphic" isn't >>> a child node, just like a Shape's "clip" isn't a child node. >>> >>> Creating a separate "document graph" (or "logical graph") sounds like an >>> interesting idea, but it would bring its own set of challenges. And it >>> wouldn't directly solve this case anyway. >>> >>> -- Kevin >>> >>> >>> On 12/1/2022 9:42 AM, Michael Strauß wrote: >>> > There's a larger picture here: from a user perspective, there's a >>> > difference between the scene graph and the "document graph". >>> > The document graph is what users actually work with, for example by >>> > setting the `Labeled.graphic` property. In some cases, document nodes >>> > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to >>> > mind). >>> > The document graph is later inflated into a scene graph of unknown >>> > structure (because skins are mostly black boxes with regards to their >>> > internal structure). >>> > >>> > I've proposed an enhancement that would make the document graph a >>> > first-class citizen: >>> > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html >>> > >>> > With this in place, we could simply disallow the same node appearing >>> > multiple times in the document graph, which would not only solve the >>> > problem for `Labeled`, but for all controls with a similar problem. >>> > >>> > >>> > On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx >>> wrote: >>> >> The mechanism does seem like it is a bit poorly designed, as it is >>> easy to create inconsistencies. >>> >> >>> >> Luckily it seems that you can't remove a graphic yourself from a >>> Control (getChildren is protected). >>> >> >>> >> I don't think there is an easy solution though... >>> >> >>> >> I think that once you set a graphic on a Labeled, you need to somehow >>> mark it as "in use". Normally you could just check parent != null for >>> this, but it is trickier than that. The Skin ultimately determines if it >>> adds the graphic as child, which may be delayed or may even be disabled >>> (content display property is set to showing TEXT only). >>> >> >>> >> Perhaps Skins should always add the graphic and just hide it (visible >>> false, managed false), but that's going to be hard to enforce. >>> >> >>> >> Marking the graphic as "in use" could be done with: >>> >> >>> >> - a property in `getProperties` >>> >> - a new "owner" or "ownedBy" property >>> >> - allowing "parent" to be set without adding it to children (probably >>> going to mess up stuff) >>> >> >>> >> --John >>> >>>
Re: Setting graphics of a Labeled does not show the Label correctly
Adding another field doesn't seem ideal, would it be possible to generalize the clipParent field to function for both (the ownedBy or owner field that I suggested earlier)? --John On 01/12/2022 20:26, Nir Lisker wrote: Michael's idea could solve the problem if it's about more than just traversing, it needs to set rules for allowing a node to serve only 1 logical role (or 1 role type, like clip and graphic?) at the same time. In any case, these rules need to be decided upon before starting to work on anything. I can do a quick fix for now that can be reverted later if needed. From what I gather, I will need to add a graphicsParent field like clipParent does. On Thu, Dec 1, 2022 at 8:47 PM Nir Lisker wrote: By the way, these issues are caused by this inconsistent behavior (they are probably duplicates): https://bugs.openjdk.org/browse/JDK-8209017 https://bugs.openjdk.org/browse/JDK-8190331 The graphic of the checkbox of a CheckBoxTreeItem is not set correctly on the new CheckBox that is provided with the cell when virtual flow switches it. It might happen with other controls that use virtual flow. On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth wrote: This seems related, but somewhat tangential. A Control's "graphic" isn't a child node, just like a Shape's "clip" isn't a child node. Creating a separate "document graph" (or "logical graph") sounds like an interesting idea, but it would bring its own set of challenges. And it wouldn't directly solve this case anyway. -- Kevin On 12/1/2022 9:42 AM, Michael Strauß wrote: > There's a larger picture here: from a user perspective, there's a > difference between the scene graph and the "document graph". > The document graph is what users actually work with, for example by > setting the `Labeled.graphic` property. In some cases, document nodes > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to > mind). > The document graph is later inflated into a scene graph of unknown > structure (because skins are mostly black boxes with regards to their > internal structure). > > I've proposed an enhancement that would make the document graph a > first-class citizen: > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html > > With this in place, we could simply disallow the same node appearing > multiple times in the document graph, which would not only solve the > problem for `Labeled`, but for all controls with a similar problem. > > > On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx wrote: >> The mechanism does seem like it is a bit poorly designed, as it is easy to create inconsistencies. >> >> Luckily it seems that you can't remove a graphic yourself from a Control (getChildren is protected). >> >> I don't think there is an easy solution though... >> >> I think that once you set a graphic on a Labeled, you need to somehow mark it as "in use". Normally you could just check parent != null for this, but it is trickier than that. The Skin ultimately determines if it adds the graphic as child, which may be delayed or may even be disabled (content display property is set to showing TEXT only). >> >> Perhaps Skins should always add the graphic and just hide it (visible false, managed false), but that's going to be hard to enforce. >> >> Marking the graphic as "in use" could be done with: >> >> - a property in `getProperties` >> - a new "owner" or "ownedBy" property >> - allowing "parent" to be set without adding it to children (probably going to mess up stuff) >> >> --John
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Thu, 1 Dec 2022 17:53:32 GMT, John Hendrikx wrote: >> my version does not create extra object(s). > > Hm, I thought that the number of objects created is the same, either a > `ChangeListener` is created and then wrapped in `ChLi` or a `Consumer` is > created and wrapped in `ChLi`. yes, you are right (I was thinking of some earlier version of LH). thanks! - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Thu, 1 Dec 2022 17:48:59 GMT, John Hendrikx wrote: >> You are right: some weak listeners remain, I did not want to re-write the >> whole thing for a fear of introducing regression and to keep the changes to >> a minimum. >> >> The second `ListenerHelper` (line 293) gets disconnected in dispose(), or >> when the skin is collected (since the skin may not be explicitly >> uninstalled, but instead the whole component or `Pane` might be removed from >> the scene and discarded. > >> but instead the whole component or Pane might be removed from the scene and >> discarded. > > If that happens, weak reference won't make a difference there either. > Removing an entire branch from the Scene and not referring to it anymore will > do correct clean-up even without a call to dispose and without the use of > weak references. > > The only use case I see is that we still don't trust the Skin lifecycle to be > adhered to and "just in case" are using weak references in case somehow > dispose is not called. I would much prefer to see an actual reason to use it, > and then adding a comment as to why this weak reference is needed so that in > 6 months time we're not scratching our heads as to why a weak reference is > needed here. just to be sure, which weak listener are you referring to? - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v18]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294589: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/906/files - new: https://git.openjdk.org/jfx/pull/906/files/b51db815..cbcc17e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=906=17 - incr: https://webrevs.openjdk.org/?repo=jfx=906=16-17 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: Setting graphics of a Labeled does not show the Label correctly
Michael's idea could solve the problem if it's about more than just traversing, it needs to set rules for allowing a node to serve only 1 logical role (or 1 role type, like clip and graphic?) at the same time. In any case, these rules need to be decided upon before starting to work on anything. I can do a quick fix for now that can be reverted later if needed. From what I gather, I will need to add a graphicsParent field like clipParent does. On Thu, Dec 1, 2022 at 8:47 PM Nir Lisker wrote: > By the way, these issues are caused by this inconsistent behavior (they > are probably duplicates): > > https://bugs.openjdk.org/browse/JDK-8209017 > https://bugs.openjdk.org/browse/JDK-8190331 > > The graphic of the checkbox of a CheckBoxTreeItem is not set correctly on > the new CheckBox that is provided with the cell when virtual flow switches > it. It might happen with other controls that use virtual flow. > > On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth > wrote: > >> This seems related, but somewhat tangential. A Control's "graphic" isn't >> a child node, just like a Shape's "clip" isn't a child node. >> >> Creating a separate "document graph" (or "logical graph") sounds like an >> interesting idea, but it would bring its own set of challenges. And it >> wouldn't directly solve this case anyway. >> >> -- Kevin >> >> >> On 12/1/2022 9:42 AM, Michael Strauß wrote: >> > There's a larger picture here: from a user perspective, there's a >> > difference between the scene graph and the "document graph". >> > The document graph is what users actually work with, for example by >> > setting the `Labeled.graphic` property. In some cases, document nodes >> > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to >> > mind). >> > The document graph is later inflated into a scene graph of unknown >> > structure (because skins are mostly black boxes with regards to their >> > internal structure). >> > >> > I've proposed an enhancement that would make the document graph a >> > first-class citizen: >> > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html >> > >> > With this in place, we could simply disallow the same node appearing >> > multiple times in the document graph, which would not only solve the >> > problem for `Labeled`, but for all controls with a similar problem. >> > >> > >> > On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx >> wrote: >> >> The mechanism does seem like it is a bit poorly designed, as it is >> easy to create inconsistencies. >> >> >> >> Luckily it seems that you can't remove a graphic yourself from a >> Control (getChildren is protected). >> >> >> >> I don't think there is an easy solution though... >> >> >> >> I think that once you set a graphic on a Labeled, you need to somehow >> mark it as "in use". Normally you could just check parent != null for >> this, but it is trickier than that. The Skin ultimately determines if it >> adds the graphic as child, which may be delayed or may even be disabled >> (content display property is set to showing TEXT only). >> >> >> >> Perhaps Skins should always add the graphic and just hide it (visible >> false, managed false), but that's going to be hard to enforce. >> >> >> >> Marking the graphic as "in use" could be done with: >> >> >> >> - a property in `getProperties` >> >> - a new "owner" or "ownedBy" property >> >> - allowing "parent" to be set without adding it to children (probably >> going to mess up stuff) >> >> >> >> --John >> >>
Re: Setting graphics of a Labeled does not show the Label correctly
Yes, I always felt something was off about how graphics and clips worked and that in some cases these are actually inaccessible children (you can still look them up) -- it feels like they should allow sharing (clips especially) but things then break. On 01/12/2022 18:42, Michael Strauß wrote: There's a larger picture here: from a user perspective, there's a difference between the scene graph and the "document graph". The document graph is what users actually work with, for example by setting the `Labeled.graphic` property. In some cases, document nodes don't correspond to scene nodes at all (`MenuItem` or `Tab` come to mind). The document graph is later inflated into a scene graph of unknown structure (because skins are mostly black boxes with regards to their internal structure). I've proposed an enhancement that would make the document graph a first-class citizen: https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html With this in place, we could simply disallow the same node appearing multiple times in the document graph, which would not only solve the problem for `Labeled`, but for all controls with a similar problem. And also when Nodes are shared between different types of properties: Label label; Button button; label.setGraphic(button); label.setClip(button); Even with the logic in `setClip` (using `clipParent`) this will still go undetected. --John On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx wrote: The mechanism does seem like it is a bit poorly designed, as it is easy to create inconsistencies. Luckily it seems that you can't remove a graphic yourself from a Control (getChildren is protected). I don't think there is an easy solution though... I think that once you set a graphic on a Labeled, you need to somehow mark it as "in use". Normally you could just check parent != null for this, but it is trickier than that. The Skin ultimately determines if it adds the graphic as child, which may be delayed or may even be disabled (content display property is set to showing TEXT only). Perhaps Skins should always add the graphic and just hide it (visible false, managed false), but that's going to be hard to enforce. Marking the graphic as "in use" could be done with: - a property in `getProperties` - a new "owner" or "ownedBy" property - allowing "parent" to be set without adding it to children (probably going to mess up stuff) --John
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]
On Thu, 1 Dec 2022 17:38:01 GMT, John Hendrikx wrote: >> This PR adds a new (lazy*) property on `Node` which provides a boolean which >> indicates whether or not the `Node` is currently part of a `Scene`, which in >> turn is part of a currently showing `Window`. >> >> It also adds a new fluent binding method on `ObservableValue` dubbed `when` >> (open for discussion, originally I had `conditionOn` here). >> >> Both of these together means it becomes much easier to break strong >> references that prevent garbage collection between a long lived property and >> one that should be shorter lived. A good example is when a `Label` is bound >> to a long lived property: >> >> >> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); >> >> The above basically ties the life cycle of the label to the long lived >> property **only** when the label is currently showing. When it is not >> showing, the label can be eligible for GC as the listener on >> `longLivedProperty` is removed when the condition provided by >> `label::isShowingProperty` is `false`. A big advantage is that these >> listeners stop observing the long lived property **immediately** when the >> label is no longer showing, in contrast to weak bindings which may keep >> observing the long lived property (and updating the label, and triggering >> its listeners in turn) until the next GC comes along. >> >> The issue in JBS also describes making the `Subscription` API public, but I >> think that might best be a separate PR. >> >> Note that this PR contains a bugfix in `ObjectBinding` for which there is >> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because >> the tests for the newly added method would fail otherwise; once it has been >> integrated to jfx19 and then to master, I can take the fix out. >> >> (*) Lazy means here that the property won't be creating any listeners unless >> observed itself, to avoid problems creating too many listeners on >> Scene/Window. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust Node > > - Fixed javadoc > - Added comment for code that avoid eager instantiation > - Changed `isShown` to use property if it is available looking good! - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/830
Re: Setting graphics of a Labeled does not show the Label correctly
By the way, these issues are caused by this inconsistent behavior (they are probably duplicates): https://bugs.openjdk.org/browse/JDK-8209017 https://bugs.openjdk.org/browse/JDK-8190331 The graphic of the checkbox of a CheckBoxTreeItem is not set correctly on the new CheckBox that is provided with the cell when virtual flow switches it. It might happen with other controls that use virtual flow. On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth wrote: > This seems related, but somewhat tangential. A Control's "graphic" isn't > a child node, just like a Shape's "clip" isn't a child node. > > Creating a separate "document graph" (or "logical graph") sounds like an > interesting idea, but it would bring its own set of challenges. And it > wouldn't directly solve this case anyway. > > -- Kevin > > > On 12/1/2022 9:42 AM, Michael Strauß wrote: > > There's a larger picture here: from a user perspective, there's a > > difference between the scene graph and the "document graph". > > The document graph is what users actually work with, for example by > > setting the `Labeled.graphic` property. In some cases, document nodes > > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to > > mind). > > The document graph is later inflated into a scene graph of unknown > > structure (because skins are mostly black boxes with regards to their > > internal structure). > > > > I've proposed an enhancement that would make the document graph a > > first-class citizen: > > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html > > > > With this in place, we could simply disallow the same node appearing > > multiple times in the document graph, which would not only solve the > > problem for `Labeled`, but for all controls with a similar problem. > > > > > > On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx > wrote: > >> The mechanism does seem like it is a bit poorly designed, as it is easy > to create inconsistencies. > >> > >> Luckily it seems that you can't remove a graphic yourself from a > Control (getChildren is protected). > >> > >> I don't think there is an easy solution though... > >> > >> I think that once you set a graphic on a Labeled, you need to somehow > mark it as "in use". Normally you could just check parent != null for > this, but it is trickier than that. The Skin ultimately determines if it > adds the graphic as child, which may be delayed or may even be disabled > (content display property is set to showing TEXT only). > >> > >> Perhaps Skins should always add the graphic and just hide it (visible > false, managed false), but that's going to be hard to enforce. > >> > >> Marking the graphic as "in use" could be done with: > >> > >> - a property in `getProperties` > >> - a new "owner" or "ownedBy" property > >> - allowing "parent" to be set without adding it to children (probably > going to mess up stuff) > >> > >> --John > >
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 23:28:02 GMT, Andy Goryachev wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
>> line 374:
>>
>>> 372:
>>> 373: // When the parent window looses focus - menu
>>> selection should be cleared
>>> 374:
>>> sceneListenerHelper.addChangeListener(scene.windowProperty(), true, (sr,
>>> oldw, w) -> {
>>
>> Suggestion:
>>
>>
>> sceneListenerHelper.addChangeListener(scene.windowProperty(), true, w -> {
>
> my version does not create extra object(s).
Hm, I thought that the number of objects created is the same, either a
`ChangeListener` is created and then wrapped in `ChLi` or a `Consumer` is
created and wrapped in `ChLi`.
-
PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]
On Thu, 1 Dec 2022 17:38:01 GMT, John Hendrikx wrote: >> This PR adds a new (lazy*) property on `Node` which provides a boolean which >> indicates whether or not the `Node` is currently part of a `Scene`, which in >> turn is part of a currently showing `Window`. >> >> It also adds a new fluent binding method on `ObservableValue` dubbed `when` >> (open for discussion, originally I had `conditionOn` here). >> >> Both of these together means it becomes much easier to break strong >> references that prevent garbage collection between a long lived property and >> one that should be shorter lived. A good example is when a `Label` is bound >> to a long lived property: >> >> >> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); >> >> The above basically ties the life cycle of the label to the long lived >> property **only** when the label is currently showing. When it is not >> showing, the label can be eligible for GC as the listener on >> `longLivedProperty` is removed when the condition provided by >> `label::isShowingProperty` is `false`. A big advantage is that these >> listeners stop observing the long lived property **immediately** when the >> label is no longer showing, in contrast to weak bindings which may keep >> observing the long lived property (and updating the label, and triggering >> its listeners in turn) until the next GC comes along. >> >> The issue in JBS also describes making the `Subscription` API public, but I >> think that might best be a separate PR. >> >> Note that this PR contains a bugfix in `ObjectBinding` for which there is >> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because >> the tests for the newly added method would fail otherwise; once it has been >> integrated to jfx19 and then to master, I can take the fix out. >> >> (*) Lazy means here that the property won't be creating any listeners unless >> observed itself, to avoid problems creating too many listeners on >> Scene/Window. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust Node > > - Fixed javadoc > - Added comment for code that avoid eager instantiation > - Changed `isShown` to use property if it is available Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 23:25:16 GMT, Andy Goryachev wrote: > but instead the whole component or Pane might be removed from the scene and > discarded. If that happens, weak reference won't make a difference there either. Removing an entire branch from the Scene and not referring to it anymore will do correct clean-up even without a call to dispose and without the use of weak references. The only use case I see is that we still don't trust the Skin lifecycle to be adhered to and "just in case" are using weak references in case somehow dispose is not called. I would much prefer to see an actual reason to use it, and then adding a comment as to why this weak reference is needed so that in 6 months time we're not scratching our heads as to why a weak reference is needed here. - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v8]
On Mon, 7 Nov 2022 20:29:16 GMT, Andy Goryachev wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix javadoc error > > I am not a native speaker, but to me `whenever` sounds event better than > `conditionOn`. I might be wrong, but it does suggest that the binding > becomes active *again* when the condition turns `true`. @andy-goryachev-oracle @mstr2 would one of you be willing to review this one as well? It should be close to its final state now. - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v16]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 67 commits: - Merge remote-tracking branch 'origin/master' into 8293119.constrained - 8293119: step1 - 8293119: more integers - 8293119: more integers - 8293119: use integers for rounded values - 8293119: tester - 8293119: descriptions - 8293119: all columns - Merge remote-tracking branch 'origin/master' into 8293119.constrained - 8293119: cleanup - ... and 57 more: https://git.openjdk.org/jfx/compare/4a19fe71...b6e2ae59 - Changes: https://git.openjdk.org/jfx/pull/897/files Webrev: https://webrevs.openjdk.org/?repo=jfx=897=15 Stats: 2260 lines in 14 files changed: 1999 ins; 250 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Thu, 1 Dec 2022 10:35:58 GMT, Nir Lisker wrote:
>> Yeah, I actually copied this pattern from an older method that has since
>> been removed (`isWindowShowing`) in an effort to avoid initializing the
>> property if all you're doing is calling its getter. I personally don't mind
>> either way, but as it seems `Node` goes through every effort to delay
>> initialization, I followed that pattern. It does duplicate the logic of the
>> property which uses `flatMap`s to achieve the same.
>
> I would suggest adding a comment saying that this is done to avoid
> initialization.
>
> I'm not sure that it's that critical for performance, to be honest.
I think the pattern is good to do, the `shownProperty` will only be used for a
small number of nodes, and not initializing it when not needed is a common
pattern in `Node` itself already. Here for example for `opacity`:
private DoubleProperty opacity;
public final void setOpacity(double value) {
opacityProperty().set(value);
}
public final double getOpacity() {
return opacity == null ? 1 : opacity.get();
}
But it is far from the only one. `disabled` does it, `pickOnBounds`,
`blendMode`, etc.
None of these contain a comment as to why, but I'll add one for `shown`.
-
PR: https://git.openjdk.org/jfx/pull/830
Re: Setting graphics of a Labeled does not show the Label correctly
This seems related, but somewhat tangential. A Control's "graphic" isn't a child node, just like a Shape's "clip" isn't a child node. Creating a separate "document graph" (or "logical graph") sounds like an interesting idea, but it would bring its own set of challenges. And it wouldn't directly solve this case anyway. -- Kevin On 12/1/2022 9:42 AM, Michael Strauß wrote: There's a larger picture here: from a user perspective, there's a difference between the scene graph and the "document graph". The document graph is what users actually work with, for example by setting the `Labeled.graphic` property. In some cases, document nodes don't correspond to scene nodes at all (`MenuItem` or `Tab` come to mind). The document graph is later inflated into a scene graph of unknown structure (because skins are mostly black boxes with regards to their internal structure). I've proposed an enhancement that would make the document graph a first-class citizen: https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html With this in place, we could simply disallow the same node appearing multiple times in the document graph, which would not only solve the problem for `Labeled`, but for all controls with a similar problem. On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx wrote: The mechanism does seem like it is a bit poorly designed, as it is easy to create inconsistencies. Luckily it seems that you can't remove a graphic yourself from a Control (getChildren is protected). I don't think there is an easy solution though... I think that once you set a graphic on a Labeled, you need to somehow mark it as "in use". Normally you could just check parent != null for this, but it is trickier than that. The Skin ultimately determines if it adds the graphic as child, which may be delayed or may even be disabled (content display property is set to showing TEXT only). Perhaps Skins should always add the graphic and just hide it (visible false, managed false), but that's going to be hard to enforce. Marking the graphic as "in use" could be done with: - a property in `getProperties` - a new "owner" or "ownedBy" property - allowing "parent" to be set without adding it to children (probably going to mess up stuff) --John
Re: Setting graphics of a Labeled does not show the Label correctly
There's a larger picture here: from a user perspective, there's a difference between the scene graph and the "document graph". The document graph is what users actually work with, for example by setting the `Labeled.graphic` property. In some cases, document nodes don't correspond to scene nodes at all (`MenuItem` or `Tab` come to mind). The document graph is later inflated into a scene graph of unknown structure (because skins are mostly black boxes with regards to their internal structure). I've proposed an enhancement that would make the document graph a first-class citizen: https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html With this in place, we could simply disallow the same node appearing multiple times in the document graph, which would not only solve the problem for `Labeled`, but for all controls with a similar problem. On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx wrote: > > The mechanism does seem like it is a bit poorly designed, as it is easy to > create inconsistencies. > > Luckily it seems that you can't remove a graphic yourself from a Control > (getChildren is protected). > > I don't think there is an easy solution though... > > I think that once you set a graphic on a Labeled, you need to somehow mark it > as "in use". Normally you could just check parent != null for this, but it > is trickier than that. The Skin ultimately determines if it adds the graphic > as child, which may be delayed or may even be disabled (content display > property is set to showing TEXT only). > > Perhaps Skins should always add the graphic and just hide it (visible false, > managed false), but that's going to be hard to enforce. > > Marking the graphic as "in use" could be done with: > > - a property in `getProperties` > - a new "owner" or "ownedBy" property > - allowing "parent" to be set without adding it to children (probably going > to mess up stuff) > > --John
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]
> This PR adds a new (lazy*) property on `Node` which provides a boolean which > indicates whether or not the `Node` is currently part of a `Scene`, which in > turn is part of a currently showing `Window`. > > It also adds a new fluent binding method on `ObservableValue` dubbed `when` > (open for discussion, originally I had `conditionOn` here). > > Both of these together means it becomes much easier to break strong > references that prevent garbage collection between a long lived property and > one that should be shorter lived. A good example is when a `Label` is bound > to a long lived property: > > > label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); > > The above basically ties the life cycle of the label to the long lived > property **only** when the label is currently showing. When it is not > showing, the label can be eligible for GC as the listener on > `longLivedProperty` is removed when the condition provided by > `label::isShowingProperty` is `false`. A big advantage is that these > listeners stop observing the long lived property **immediately** when the > label is no longer showing, in contrast to weak bindings which may keep > observing the long lived property (and updating the label, and triggering its > listeners in turn) until the next GC comes along. > > The issue in JBS also describes making the `Subscription` API public, but I > think that might best be a separate PR. > > Note that this PR contains a bugfix in `ObjectBinding` for which there is > another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because > the tests for the newly added method would fail otherwise; once it has been > integrated to jfx19 and then to master, I can take the fix out. > > (*) Lazy means here that the property won't be creating any listeners unless > observed itself, to avoid problems creating too many listeners on > Scene/Window. John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Adjust Node - Fixed javadoc - Added comment for code that avoid eager instantiation - Changed `isShown` to use property if it is available - Changes: - all: https://git.openjdk.org/jfx/pull/830/files - new: https://git.openjdk.org/jfx/pull/830/files/46f206aa..5c22fdd0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=830=08 - incr: https://webrevs.openjdk.org/?repo=jfx=830=07-08 Stats: 12 lines in 1 file changed: 6 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/830.diff Fetch: git fetch https://git.openjdk.org/jfx pull/830/head:pull/830 PR: https://git.openjdk.org/jfx/pull/830
Re: Setting graphics of a Labeled does not show the Label correctly
I'm not sure wrapping it in a runLater would make much difference, and
if it does, it would be fragile.
I think that the best approach might be to disallow using the same Node
as the "graphic" of two controls, in the same way that we disallow
setting a Node as a clip of two different nodes. This would involve a
CSR, since it's both a spec and a behavior change. The implementation
would need to check whether the Node is in use, and if so, throw an
exception (after first unbinding if bound).
-- Kevin
On 12/1/2022 9:20 AM, Andy Goryachev wrote:
Does wrapping the action code in runLater() help?
b1.setOnAction((ev) -> {
Platform.runLater(() -> {
if (b1.getParent() == cb1) {
...
-andy
*From: *openjfx-dev on behalf of John
Hendrikx
*Date: *Thursday, 2022/12/01 at 09:14
*To: *Nir Lisker
*Cc: *openjfx-dev@openjdk.org
*Subject: *Re: Setting graphics of a Labeled does not show the Label
correctly
The mechanism does seem like it is a bit poorly designed, as it is
easy to create inconsistencies.
Luckily it seems that you can't remove a graphic yourself from a
Control (getChildren is protected).
I don't think there is an easy solution though...
I think that once you set a graphic on a Labeled, you need to somehow
mark it as "in use". Normally you could just check parent != null for
this, but it is trickier than that. The Skin ultimately determines if
it adds the graphic as child, which may be delayed or may even be
disabled (content display property is set to showing TEXT only).
Perhaps Skins should always add the graphic and just hide it (visible
false, managed false), but that's going to be hard to enforce.
Marking the graphic as "in use" could be done with:
- a property in `getProperties`
- a new "owner" or "ownedBy" property
- allowing "parent" to be set without adding it to children (probably
going to mess up stuff)
--John
On 01/12/2022 15:21, Nir Lisker wrote:
Technically it doesn't appear elsewhere in the scenegraph, it is
the child of only one label. It's set as the graphics property of
2 labels though. The mismatch is that being set as a graphics
property isn't a 1-to-1 relationship with being a child of the label.
Something has to be fixed along this chain of inconsistency, I
just wasn't sure where. It seems like the IAE that you mentioned
should be thrown, but isn't. I can write a PR for that. One thing
I'm not sure about is: in a situation where the graphic belongs to
a label that is detached from a scene, and that graphic is set to
a label that *is* part of a scene, should an IAE be thrown as well.
Even if the Labeled is part of the Scene, the graphic may not be
(depending on Skin used and on Content Display property for the
default skin).
By the way, changing to throwing an IAE is going to cause some new
exceptions. There is a possibility that some VirtualFlow things
will break because that mechanism recycles nodes and re-attaches
their properties. Then again, it might just mean that it was done
wrong.
On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx
wrote:
Sorry, I meant on the first click already.
--John
On 01/12/2022 14:46, John Hendrikx wrote:
Setting the same Node for multiple graphics is not
allowed. Your program should IMHO throw
"IllegalArgumentException" on the 2nd click.
* An optional icon for the Labeled. This can be
positioned relative to the
* text by using {@link #setContentDisplay}. The node
specified for this
* variable cannot appear elsewhere in the scene
graph, otherwise
* the {@code IllegalArgumentException} is thrown.
See the class
* description of {@link Node} for more detail.
--John
On 01/12/2022 13:38, Nir Lisker wrote:
That's my point. Currently, a node can serve as the
graphics property for multiple Labels, but will appear
only in 1 because it can only have a single parent in
the scenegraph. While this is technically fine, it
causes issues like the one I showed above. I'm not
sure if a node is supposed to be able to serve as
multiple graphics (and displayed only in the last
Label it was set to?), or removed from other Labels'
graphic properties just like is done for children in
the scenegraph. Personally, I find it confusing that
label.getGraphics() will return a node that isn't
shown in that label.
On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
wrote:
Internally the graphics is just a child node, and
nodes can't be part of
the scene graph twice (this is
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v17]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 49 commits: - Merge remote-tracking branch 'origin/master' into 8294589.menubarskin.leak - 8294589: review comments - 8294589: cleanup - Merge remote-tracking branch 'origin/master' into 8294589.menubarskin.leak - 8294589: testing github merge-conflict label behavior - 8294589: cleanup - Merge remote-tracking branch 'origin/master' into 8294589.menubarskin.leak - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak - 8294809: generics - 8294589: owner - ... and 39 more: https://git.openjdk.org/jfx/compare/4a19fe71...b51db815 - Changes: https://git.openjdk.org/jfx/pull/906/files Webrev: https://webrevs.openjdk.org/?repo=jfx=906=16 Stats: 470 lines in 2 files changed: 234 ins; 205 del; 31 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: Setting graphics of a Labeled does not show the Label correctly
Does wrapping the action code in runLater() help?
b1.setOnAction((ev) -> {
Platform.runLater(() -> {
if (b1.getParent() == cb1) {
...
-andy
From: openjfx-dev on behalf of John Hendrikx
Date: Thursday, 2022/12/01 at 09:14
To: Nir Lisker
Cc: openjfx-dev@openjdk.org
Subject: Re: Setting graphics of a Labeled does not show the Label correctly
The mechanism does seem like it is a bit poorly designed, as it is easy to
create inconsistencies.
Luckily it seems that you can't remove a graphic yourself from a Control
(getChildren is protected).
I don't think there is an easy solution though...
I think that once you set a graphic on a Labeled, you need to somehow mark it
as "in use". Normally you could just check parent != null for this, but it is
trickier than that. The Skin ultimately determines if it adds the graphic as
child, which may be delayed or may even be disabled (content display property
is set to showing TEXT only).
Perhaps Skins should always add the graphic and just hide it (visible false,
managed false), but that's going to be hard to enforce.
Marking the graphic as "in use" could be done with:
- a property in `getProperties`
- a new "owner" or "ownedBy" property
- allowing "parent" to be set without adding it to children (probably going to
mess up stuff)
--John
On 01/12/2022 15:21, Nir Lisker wrote:
Technically it doesn't appear elsewhere in the scenegraph, it is the child of
only one label. It's set as the graphics property of 2 labels though. The
mismatch is that being set as a graphics property isn't a 1-to-1 relationship
with being a child of the label.
Something has to be fixed along this chain of inconsistency, I just wasn't sure
where. It seems like the IAE that you mentioned should be thrown, but isn't. I
can write a PR for that. One thing I'm not sure about is: in a situation where
the graphic belongs to a label that is detached from a scene, and that graphic
is set to a label that *is* part of a scene, should an IAE be thrown as well.
Even if the Labeled is part of the Scene, the graphic may not be (depending on
Skin used and on Content Display property for the default skin).
By the way, changing to throwing an IAE is going to cause some new exceptions.
There is a possibility that some VirtualFlow things will break because that
mechanism recycles nodes and re-attaches their properties. Then again, it might
just mean that it was done wrong.
On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx
mailto:john.hendr...@gmail.com>> wrote:
Sorry, I meant on the first click already.
--John
On 01/12/2022 14:46, John Hendrikx wrote:
Setting the same Node for multiple graphics is not allowed. Your program
should IMHO throw "IllegalArgumentException" on the 2nd click.
* An optional icon for the Labeled. This can be positioned relative to the
* text by using {@link #setContentDisplay}. The node specified for this
* variable cannot appear elsewhere in the scene graph, otherwise
* the {@code IllegalArgumentException} is thrown. See the class
* description of {@link Node} for more detail.
--John
On 01/12/2022 13:38, Nir Lisker wrote:
That's my point. Currently, a node can serve as the graphics property for
multiple Labels, but will appear only in 1 because it can only have a single
parent in the scenegraph. While this is technically fine, it causes issues like
the one I showed above. I'm not sure if a node is supposed to be able to serve
as multiple graphics (and displayed only in the last Label it was set to?), or
removed from other Labels' graphic properties just like is done for children in
the scenegraph. Personally, I find it confusing that label.getGraphics() will
return a node that isn't shown in that label.
On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
mailto:john.hendr...@gmail.com>> wrote:
Internally the graphics is just a child node, and nodes can't be part of
the scene graph twice (this is done in LabeledSkinBase).
It showing up only once is probably because it is getting removed from
the other labels.
I think things are probably getting out of sync, where the graphics
property may think it still has a certain node as its graphics, but it
is no longer a child of the label.
--John
On 01/12/2022 11:23, Nir Lisker wrote:
> Hi,
>
> Given the following code
>
> var cb1 = new Label("1");
> var cb2 = new Label ("2");
> var b1 = new Button("A");
> cb1.setGraphic(b1);
> b1.setOnAction(e -> {
> if (b1.getParent() == cb1) {
> // cb1.setGraphic(null);
> cb2.setGraphic(b1);
> } else {
> // cb2.setGraphic(null);
> cb1.setGraphic(b1);
> }
> });
>
> Pressing the first button will move it (the graphic) to the second
> label, however, pressing it again will not move it back to the first
> label. It's required to set the graphics to null prior to moving them
> as in the commented
Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v3]
> as determined by SkinMemoryLeakTest (remove line 165) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.BUTTON_BAR; > > > caused by: > - adding and not removing listeners > - adding and not removing children Nodes Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits: - 8295506: cleanup - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin - 8294809: generics - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin - 8294809: is alive - Revert "8294809: removed weak listeners support" This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. - 8295506: button bar skin - ... and 15 more: https://git.openjdk.org/jfx/compare/4a19fe71...efc60c0c - Changes: https://git.openjdk.org/jfx/pull/921/files Webrev: https://webrevs.openjdk.org/?repo=jfx=921=02 Stats: 30 lines in 2 files changed: 17 ins; 9 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/921.diff Fetch: git fetch https://git.openjdk.org/jfx pull/921/head:pull/921 PR: https://git.openjdk.org/jfx/pull/921
Re: Setting graphics of a Labeled does not show the Label correctly
The mechanism does seem like it is a bit poorly designed, as it is easy
to create inconsistencies.
Luckily it seems that you can't remove a graphic yourself from a Control
(getChildren is protected).
I don't think there is an easy solution though...
I think that once you set a graphic on a Labeled, you need to somehow
mark it as "in use". Normally you could just check parent != null for
this, but it is trickier than that. The Skin ultimately determines if
it adds the graphic as child, which may be delayed or may even be
disabled (content display property is set to showing TEXT only).
Perhaps Skins should always add the graphic and just hide it (visible
false, managed false), but that's going to be hard to enforce.
Marking the graphic as "in use" could be done with:
- a property in `getProperties`
- a new "owner" or "ownedBy" property
- allowing "parent" to be set without adding it to children (probably
going to mess up stuff)
--John
On 01/12/2022 15:21, Nir Lisker wrote:
Technically it doesn't appear elsewhere in the scenegraph, it is the
child of only one label. It's set as the graphics property of 2 labels
though. The mismatch is that being set as a graphics property isn't a
1-to-1 relationship with being a child of the label.
Something has to be fixed along this chain of inconsistency, I just
wasn't sure where. It seems like the IAE that you mentioned should be
thrown, but isn't. I can write a PR for that. One thing I'm not sure
about is: in a situation where the graphic belongs to a label that is
detached from a scene, and that graphic is set to a label that *is*
part of a scene, should an IAE be thrown as well.
Even if the Labeled is part of the Scene, the graphic may not be
(depending on Skin used and on Content Display property for the default
skin).
By the way, changing to throwing an IAE is going to cause some new
exceptions. There is a possibility that some VirtualFlow things will
break because that mechanism recycles nodes and re-attaches their
properties. Then again, it might just mean that it was done wrong.
On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx
wrote:
Sorry, I meant on the first click already.
--John
On 01/12/2022 14:46, John Hendrikx wrote:
Setting the same Node for multiple graphics is not allowed. Your
program should IMHO throw "IllegalArgumentException" on the 2nd
click.
* An optional icon for the Labeled. This can be positioned
relative to the
* text by using {@link #setContentDisplay}. The node
specified for this
* variable cannot appear elsewhere in the scene graph, otherwise
* the {@code IllegalArgumentException} is thrown. See the class
* description of {@link Node} for more detail.
--John
On 01/12/2022 13:38, Nir Lisker wrote:
That's my point. Currently, a node can serve as the graphics
property for multiple Labels, but will appear only in 1 because
it can only have a single parent in the scenegraph. While this
is technically fine, it causes issues like the one I showed
above. I'm not sure if a node is supposed to be able to serve as
multiple graphics (and displayed only in the last Label it was
set to?), or removed from other Labels' graphic properties just
like is done for children in the scenegraph. Personally, I find
it confusing that label.getGraphics() will return a node that
isn't shown in that label.
On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
wrote:
Internally the graphics is just a child node, and nodes
can't be part of
the scene graph twice (this is done in LabeledSkinBase).
It showing up only once is probably because it is getting
removed from
the other labels.
I think things are probably getting out of sync, where the
graphics
property may think it still has a certain node as its
graphics, but it
is no longer a child of the label.
--John
On 01/12/2022 11:23, Nir Lisker wrote:
> Hi,
>
> Given the following code
>
> var cb1 = new Label("1");
> var cb2 = new Label ("2");
> var b1 = new Button("A");
> cb1.setGraphic(b1);
> b1.setOnAction(e -> {
> if (b1.getParent() == cb1) {
> // cb1.setGraphic(null);
> cb2.setGraphic(b1);
> } else {
> // cb2.setGraphic(null);
> cb1.setGraphic(b1);
> }
> });
>
> Pressing the first button will move it (the graphic) to
the second
> label, however, pressing it again will not move it back to
the first
> label. It's required to set the graphics to null prior to
moving them
> as in the commented
Re: RFR: 8295500: AccordionSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 16:29:58 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 164) and a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.ACCORDION; >> >> >> caused by: >> - adding and not removing listeners > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 27 commits: > > - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin > - 8295500: cleanup > - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin > - Merge branch '8294809.listener.helper' into 8295500.accordion.skin > - 8294809: generics > - Merge branch '8294809.listener.helper' into 8295500.accordion.skin > - 8294809: is alive > - Merge branch '8294809.listener.helper' into 8295500.accordion.skin > - Revert "8294809: removed weak listeners support" > >This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. > - 8295500: cleanup > - ... and 17 more: https://git.openjdk.org/jfx/compare/0a785ae0...218c6ea9 @aghaisas : would you please review this? - PR: https://git.openjdk.org/jfx/pull/920
Re: RFR: 8295500: AccordionSkin: memory leak when changing skin [v3]
> as determined by SkinMemoryLeakTest (remove line 164) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.ACCORDION; > > > caused by: > - adding and not removing listeners Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits: - 8295500: cleanup - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin - 8295500: cleanup - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin - Merge branch '8294809.listener.helper' into 8295500.accordion.skin - 8294809: generics - Merge branch '8294809.listener.helper' into 8295500.accordion.skin - 8294809: is alive - Merge branch '8294809.listener.helper' into 8295500.accordion.skin - ... and 19 more: https://git.openjdk.org/jfx/compare/4a19fe71...8da3b64f - Changes: https://git.openjdk.org/jfx/pull/920/files Webrev: https://webrevs.openjdk.org/?repo=jfx=920=02 Stats: 37 lines in 2 files changed: 16 ins; 13 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/920.diff Fetch: git fetch https://git.openjdk.org/jfx pull/920/head:pull/920 PR: https://git.openjdk.org/jfx/pull/920
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 23:13:42 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java >> line 227: >> >>> 225: ComboBox.class, >>> 226: DatePicker.class, >>> 227: //MenuBar.class, >> >> minor: commented out code > > intentionally, to avoid merge conflicts. edit: unfortunately, this trick did help - getting merge conflicts. will be removing the entries. - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v2]
On Thu, 1 Dec 2022 11:11:31 GMT, Ajit Ghaisas wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 23 commits: >> >> - Merge remote-tracking branch 'origin/master' into >>8295506.button.bar.skin >> - Merge remote-tracking branch 'origin/master' into >>8295506.button.bar.skin >> - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin >> - 8294809: generics >> - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin >> - 8294809: is alive >> - Revert "8294809: removed weak listeners support" >> >>This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. >> - 8295506: button bar skin >> - 8294809: removed weak listeners support >> - 8294809: use weak reference correctly this time >> - ... and 13 more: https://git.openjdk.org/jfx/compare/0a785ae0...759ecaf4 > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonBarSkin.java > line 139: > >> 137: } >> 138: >> 139: updateButtonListeners(getSkinnable().getButtons(), false); > > Can we use `ListenerHelper` in `updateButtonListeners` method? > We may need to use `ListernerHelper` of `ButtonSkin`. good question. I did not want to make drastic structural changes for risk of introducing regression. Right now, this skin uses the same pattern when dealing with multiple child items - it adds/removes listeners in the constructor, in the dispose() method, and upon any changes to the observable list. Perhaps not the most optimal solution, but it works and causes no trouble. I'd rather keep it as is. - PR: https://git.openjdk.org/jfx/pull/921
Integrated: 8295426: MenuButtonSkin: memory leak when changing skin
On Mon, 17 Oct 2022 22:42:42 GMT, Andy Goryachev wrote: > as determined by SkinMemoryLeakTest (remove line 170) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Also applies to SplitMenuButton, since they share the same base class > MenuButtonSkinBase. > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or > SPLIT_MENU_BUTTON > > In addition, there seems to be another failure scenario when simply replacing > the skin - no menu is shown upon a click. To reproduce, launch LeakTest and > click once on the [Replace Skin] button. Second click restores the function. > > caused by: > - adding and not removing EventHandlers > - setting and not clearing onAction handlers > - incorrect logic in setting mousePressed/mouse/Released handlers This pull request has now been integrated. Changeset: 4a19fe71 Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/4a19fe71f9151460dca97d4ca9962fd630404ee8 Stats: 175 lines in 4 files changed: 109 ins; 32 del; 34 mod 8295426: MenuButtonSkin: memory leak when changing skin Reviewed-by: aghaisas - PR: https://git.openjdk.org/jfx/pull/919
Re: RFR: 8276170: Create Sources when publishing to Maven
On Fri, 29 Oct 2021 12:36:13 GMT, eduardsdv wrote: > Create sources.jars and attach they to the publish task, so that they can be > uploaded to the (e.g. maven) repository automatically. In this case, there was no agreement that we should add this capability. As @johanvos said in [this comment](https://bugs.openjdk.org/browse/JDK-8276170?focusedCommentId=14455624=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14455624) in the JBS bug report, it might have been better for the maven publishing task to not be in the build at all. - PR: https://git.openjdk.org/jfx/pull/657
Re: RFR: 8276170: Create Sources when publishing to Maven
On Fri, 29 Oct 2021 12:36:13 GMT, eduardsdv wrote: > Create sources.jars and attach they to the publish task, so that they can be > uploaded to the (e.g. maven) repository automatically. It requires only 1 reviewer with the tole of Reviewer. One of you can be the second reviewer. - PR: https://git.openjdk.org/jfx/pull/657
Re: RFR: 8276170: Create Sources when publishing to Maven
On Fri, 29 Oct 2021 12:36:13 GMT, eduardsdv wrote: > Create sources.jars and attach they to the publish task, so that they can be > uploaded to the (e.g. maven) repository automatically. I'm actually using it for my builds ... But sadly I'm not a reviewer. - PR: https://git.openjdk.org/jfx/pull/657
Re: Setting graphics of a Labeled does not show the Label correctly
Technically it doesn't appear elsewhere in the scenegraph, it is the child
of only one label. It's set as the graphics property of 2 labels though.
The mismatch is that being set as a graphics property isn't a 1-to-1
relationship with being a child of the label.
Something has to be fixed along this chain of inconsistency, I just wasn't
sure where. It seems like the IAE that you mentioned should be thrown, but
isn't. I can write a PR for that. One thing I'm not sure about is: in a
situation where the graphic belongs to a label that is detached from a
scene, and that graphic is set to a label that *is* part of a scene, should
an IAE be thrown as well.
By the way, changing to throwing an IAE is going to cause some new
exceptions. There is a possibility that some VirtualFlow things will break
because that mechanism recycles nodes and re-attaches their properties.
Then again, it might just mean that it was done wrong.
On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx
wrote:
> Sorry, I meant on the first click already.
>
> --John
> On 01/12/2022 14:46, John Hendrikx wrote:
>
> Setting the same Node for multiple graphics is not allowed. Your program
> should IMHO throw "IllegalArgumentException" on the 2nd click.
>
> * An optional icon for the Labeled. This can be positioned relative
> to the
> * text by using {@link #setContentDisplay}. The node specified for
> this
> * variable cannot appear elsewhere in the scene graph, otherwise
> * the {@code IllegalArgumentException} is thrown. See the class
> * description of {@link Node} for more detail.
>
> --John
> On 01/12/2022 13:38, Nir Lisker wrote:
>
> That's my point. Currently, a node can serve as the graphics property for
> multiple Labels, but will appear only in 1 because it can only have a
> single parent in the scenegraph. While this is technically fine, it causes
> issues like the one I showed above. I'm not sure if a node is supposed to
> be able to serve as multiple graphics (and displayed only in the last Label
> it was set to?), or removed from other Labels' graphic properties just like
> is done for children in the scenegraph. Personally, I find it confusing
> that label.getGraphics() will return a node that isn't shown in that label.
>
> On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
> wrote:
>
>> Internally the graphics is just a child node, and nodes can't be part of
>> the scene graph twice (this is done in LabeledSkinBase).
>>
>> It showing up only once is probably because it is getting removed from
>> the other labels.
>>
>> I think things are probably getting out of sync, where the graphics
>> property may think it still has a certain node as its graphics, but it
>> is no longer a child of the label.
>>
>> --John
>>
>> On 01/12/2022 11:23, Nir Lisker wrote:
>> > Hi,
>> >
>> > Given the following code
>> >
>> > var cb1 = new Label("1");
>> > var cb2 = new Label ("2");
>> > var b1 = new Button("A");
>> > cb1.setGraphic(b1);
>> > b1.setOnAction(e -> {
>> > if (b1.getParent() == cb1) {
>> > // cb1.setGraphic(null);
>> > cb2.setGraphic(b1);
>> > } else {
>> > // cb2.setGraphic(null);
>> > cb1.setGraphic(b1);
>> > }
>> > });
>> >
>> > Pressing the first button will move it (the graphic) to the second
>> > label, however, pressing it again will not move it back to the first
>> > label. It's required to set the graphics to null prior to moving them
>> > as in the commented lines. This looks like a bug to me. I would think
>> > that when a graphic is moved, it will appear in its new label
>> > immediately, like moving a child. Apparently a single node can be the
>> > graphics for multiple Labeled nodes, but it will appear only in 1. Is
>> > this intentional?
>> >
>> > - Nir
>>
>
Re: Setting graphics of a Labeled does not show the Label correctly
Sorry, I meant on the first click already.
--John
On 01/12/2022 14:46, John Hendrikx wrote:
Setting the same Node for multiple graphics is not allowed. Your
program should IMHO throw "IllegalArgumentException" on the 2nd click.
* An optional icon for the Labeled. This can be positioned
relative to the
* text by using {@link #setContentDisplay}. The node specified
for this
* variable cannot appear elsewhere in the scene graph, otherwise
* the {@code IllegalArgumentException} is thrown. See the class
* description of {@link Node} for more detail.
--John
On 01/12/2022 13:38, Nir Lisker wrote:
That's my point. Currently, a node can serve as the graphics property
for multiple Labels, but will appear only in 1 because it can only
have a single parent in the scenegraph. While this is technically
fine, it causes issues like the one I showed above. I'm not sure if a
node is supposed to be able to serve as multiple graphics (and
displayed only in the last Label it was set to?), or removed from
other Labels' graphic properties just like is done for children in
the scenegraph. Personally, I find it confusing that
label.getGraphics() will return a node that isn't shown in that label.
On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
wrote:
Internally the graphics is just a child node, and nodes can't be
part of
the scene graph twice (this is done in LabeledSkinBase).
It showing up only once is probably because it is getting removed
from
the other labels.
I think things are probably getting out of sync, where the graphics
property may think it still has a certain node as its graphics,
but it
is no longer a child of the label.
--John
On 01/12/2022 11:23, Nir Lisker wrote:
> Hi,
>
> Given the following code
>
> var cb1 = new Label("1");
> var cb2 = new Label ("2");
> var b1 = new Button("A");
> cb1.setGraphic(b1);
> b1.setOnAction(e -> {
> if (b1.getParent() == cb1) {
> // cb1.setGraphic(null);
> cb2.setGraphic(b1);
> } else {
> // cb2.setGraphic(null);
> cb1.setGraphic(b1);
> }
> });
>
> Pressing the first button will move it (the graphic) to the second
> label, however, pressing it again will not move it back to the
first
> label. It's required to set the graphics to null prior to
moving them
> as in the commented lines. This looks like a bug to me. I would
think
> that when a graphic is moved, it will appear in its new label
> immediately, like moving a child. Apparently a single node can
be the
> graphics for multiple Labeled nodes, but it will appear only in
1. Is
> this intentional?
>
> - Nir
Re: Setting graphics of a Labeled does not show the Label correctly
Setting the same Node for multiple graphics is not allowed. Your
program should IMHO throw "IllegalArgumentException" on the 2nd click.
* An optional icon for the Labeled. This can be positioned
relative to the
* text by using {@link #setContentDisplay}. The node specified
for this
* variable cannot appear elsewhere in the scene graph, otherwise
* the {@code IllegalArgumentException} is thrown. See the class
* description of {@link Node} for more detail.
--John
On 01/12/2022 13:38, Nir Lisker wrote:
That's my point. Currently, a node can serve as the graphics property
for multiple Labels, but will appear only in 1 because it can only
have a single parent in the scenegraph. While this is technically
fine, it causes issues like the one I showed above. I'm not sure if a
node is supposed to be able to serve as multiple graphics (and
displayed only in the last Label it was set to?), or removed from
other Labels' graphic properties just like is done for children in the
scenegraph. Personally, I find it confusing that label.getGraphics()
will return a node that isn't shown in that label.
On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
wrote:
Internally the graphics is just a child node, and nodes can't be
part of
the scene graph twice (this is done in LabeledSkinBase).
It showing up only once is probably because it is getting removed
from
the other labels.
I think things are probably getting out of sync, where the graphics
property may think it still has a certain node as its graphics,
but it
is no longer a child of the label.
--John
On 01/12/2022 11:23, Nir Lisker wrote:
> Hi,
>
> Given the following code
>
> var cb1 = new Label("1");
> var cb2 = new Label ("2");
> var b1 = new Button("A");
> cb1.setGraphic(b1);
> b1.setOnAction(e -> {
> if (b1.getParent() == cb1) {
> // cb1.setGraphic(null);
> cb2.setGraphic(b1);
> } else {
> // cb2.setGraphic(null);
> cb1.setGraphic(b1);
> }
> });
>
> Pressing the first button will move it (the graphic) to the second
> label, however, pressing it again will not move it back to the
first
> label. It's required to set the graphics to null prior to moving
them
> as in the commented lines. This looks like a bug to me. I would
think
> that when a graphic is moved, it will appear in its new label
> immediately, like moving a child. Apparently a single node can
be the
> graphics for multiple Labeled nodes, but it will appear only in
1. Is
> this intentional?
>
> - Nir
Re: Setting graphics of a Labeled does not show the Label correctly
That's my point. Currently, a node can serve as the graphics property for
multiple Labels, but will appear only in 1 because it can only have a
single parent in the scenegraph. While this is technically fine, it causes
issues like the one I showed above. I'm not sure if a node is supposed to
be able to serve as multiple graphics (and displayed only in the last Label
it was set to?), or removed from other Labels' graphic properties just like
is done for children in the scenegraph. Personally, I find it confusing
that label.getGraphics() will return a node that isn't shown in that label.
On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx
wrote:
> Internally the graphics is just a child node, and nodes can't be part of
> the scene graph twice (this is done in LabeledSkinBase).
>
> It showing up only once is probably because it is getting removed from
> the other labels.
>
> I think things are probably getting out of sync, where the graphics
> property may think it still has a certain node as its graphics, but it
> is no longer a child of the label.
>
> --John
>
> On 01/12/2022 11:23, Nir Lisker wrote:
> > Hi,
> >
> > Given the following code
> >
> > var cb1 = new Label("1");
> > var cb2 = new Label ("2");
> > var b1 = new Button("A");
> > cb1.setGraphic(b1);
> > b1.setOnAction(e -> {
> > if (b1.getParent() == cb1) {
> > // cb1.setGraphic(null);
> > cb2.setGraphic(b1);
> > } else {
> > // cb2.setGraphic(null);
> > cb1.setGraphic(b1);
> > }
> > });
> >
> > Pressing the first button will move it (the graphic) to the second
> > label, however, pressing it again will not move it back to the first
> > label. It's required to set the graphics to null prior to moving them
> > as in the commented lines. This looks like a bug to me. I would think
> > that when a graphic is moved, it will appear in its new label
> > immediately, like moving a child. Apparently a single node can be the
> > graphics for multiple Labeled nodes, but it will appear only in 1. Is
> > this intentional?
> >
> > - Nir
>
Re: Setting graphics of a Labeled does not show the Label correctly
Internally the graphics is just a child node, and nodes can't be part of
the scene graph twice (this is done in LabeledSkinBase).
It showing up only once is probably because it is getting removed from
the other labels.
I think things are probably getting out of sync, where the graphics
property may think it still has a certain node as its graphics, but it
is no longer a child of the label.
--John
On 01/12/2022 11:23, Nir Lisker wrote:
Hi,
Given the following code
var cb1 = new Label("1");
var cb2 = new Label ("2");
var b1 = new Button("A");
cb1.setGraphic(b1);
b1.setOnAction(e -> {
if (b1.getParent() == cb1) {
// cb1.setGraphic(null);
cb2.setGraphic(b1);
} else {
// cb2.setGraphic(null);
cb1.setGraphic(b1);
}
});
Pressing the first button will move it (the graphic) to the second
label, however, pressing it again will not move it back to the first
label. It's required to set the graphics to null prior to moving them
as in the commented lines. This looks like a bug to me. I would think
that when a graphic is moved, it will appear in its new label
immediately, like moving a child. Apparently a single node can be the
graphics for multiple Labeled nodes, but it will appear only in 1. Is
this intentional?
- Nir
Re: RFR: 8295426: MenuButtonSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 17:00:55 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 170) and a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Also applies to SplitMenuButton, since they share the same base class >> MenuButtonSkinBase. >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or >> SPLIT_MENU_BUTTON >> >> In addition, there seems to be another failure scenario when simply >> replacing the skin - no menu is shown upon a click. To reproduce, launch >> LeakTest and click once on the [Replace Skin] button. Second click restores >> the function. >> >> caused by: >> - adding and not removing EventHandlers >> - setting and not clearing onAction handlers >> - incorrect logic in setting mousePressed/mouse/Released handlers > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 37 commits: > > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - 8295426: listener helper update > - Merge remote-tracking branch 'origin/8294809.listener.helper' into > 8295426.menu.button.skin > - 8294809: review comments > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - 8294809: whitespace > - 8294809: no public api > - 8294809: map change listener > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - ... and 27 more: https://git.openjdk.org/jfx/compare/0a785ae0...800d3f1e Fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/919
Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 17:10:50 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 165) and a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.BUTTON_BAR; >> >> >> caused by: >> - adding and not removing listeners >> - adding and not removing children Nodes > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 23 commits: > > - Merge remote-tracking branch 'origin/master' into >8295506.button.bar.skin > - Merge remote-tracking branch 'origin/master' into >8295506.button.bar.skin > - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin > - 8294809: generics > - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin > - 8294809: is alive > - Revert "8294809: removed weak listeners support" > >This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. > - 8295506: button bar skin > - 8294809: removed weak listeners support > - 8294809: use weak reference correctly this time > - ... and 13 more: https://git.openjdk.org/jfx/compare/0a785ae0...759ecaf4 modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonBarSkin.java line 139: > 137: } > 138: > 139: updateButtonListeners(getSkinnable().getButtons(), false); Can we use `ListenerHelper` in `updateButtonListeners` method? We may need to use `ListernerHelper` of `ButtonSkin`. - PR: https://git.openjdk.org/jfx/pull/921
Re: RFR: 8296621: Stage steals focus on scene change [v2]
On Sun, 13 Nov 2022 01:02:36 GMT, Thiago Milczarek Sayao wrote: >> Simple fix to not requestFocus() on scene change. >> >> The attached bug sample shows that the TextField focused on the scene >> remains focused when the scene comes back. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Add test Sanity testing looks good, on Mac and Windows. No Robot test failed. Shall try some combinations and update. - PR: https://git.openjdk.org/jfx/pull/940
Re: RFR: JDK-8295755 : Update SQLite to 3.39.4
On Thu, 17 Nov 2022 06:16:34 GMT, Hima Bindu Meda wrote: > Updated sqlite to v3.39.4 > Verified build on windows, linux and mac. > Sanity testing looks fine. Sanity testing looks good. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/953
Re: RFR: 8276170: Create Sources when publishing to Maven
On Fri, 29 Oct 2021 12:36:13 GMT, eduardsdv wrote: > Create sources.jars and attach they to the publish task, so that they can be > uploaded to the (e.g. maven) repository automatically. It's a pitty that this review was never completed. Very demotivating. - PR: https://git.openjdk.org/jfx/pull/657
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v8]
On Wed, 26 Oct 2022 07:56:52 GMT, John Hendrikx wrote:
>> This PR adds a new (lazy*) property on `Node` which provides a boolean which
>> indicates whether or not the `Node` is currently part of a `Scene`, which in
>> turn is part of a currently showing `Window`.
>>
>> It also adds a new fluent binding method on `ObservableValue` dubbed `when`
>> (open for discussion, originally I had `conditionOn` here).
>>
>> Both of these together means it becomes much easier to break strong
>> references that prevent garbage collection between a long lived property and
>> one that should be shorter lived. A good example is when a `Label` is bound
>> to a long lived property:
>>
>>
>> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty));
>>
>> The above basically ties the life cycle of the label to the long lived
>> property **only** when the label is currently showing. When it is not
>> showing, the label can be eligible for GC as the listener on
>> `longLivedProperty` is removed when the condition provided by
>> `label::isShowingProperty` is `false`. A big advantage is that these
>> listeners stop observing the long lived property **immediately** when the
>> label is no longer showing, in contrast to weak bindings which may keep
>> observing the long lived property (and updating the label, and triggering
>> its listeners in turn) until the next GC comes along.
>>
>> The issue in JBS also describes making the `Subscription` API public, but I
>> think that might best be a separate PR.
>>
>> Note that this PR contains a bugfix in `ObjectBinding` for which there is
>> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because
>> the tests for the newly added method would fail otherwise; once it has been
>> integrated to jfx19 and then to master, I can take the fix out.
>>
>> (*) Lazy means here that the property won't be creating any listeners unless
>> observed itself, to avoid problems creating too many listeners on
>> Scene/Window.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix javadoc error
Looks good. Left a couple of minor comments.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1410:
> 1408: /**
> 1409: * Indicates whether or not this {@code Node} is shown. A node is
> considered shown if it's
> 1410: * part of a {@code Scene} which is part of a {@code Window} whose
which -> that
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1420:
> 1418: * when it is no longer shown.
> 1419: *
> 1420: * @see ObservableValue#when(ObservableValue)
You don't need the `@see` since you linked it in the docs.
-
PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8295324: JavaFX: Blank pages when printing [v4]
On Thu, 1 Dec 2022 10:12:26 GMT, eduardsdv wrote: >> 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 three additional > commits since the last revision: > > - Revert "8295324: Apply patch with junit from the issue" > >This reverts commit c76b8207242a7af82f5515e49760158fa40da2a9. > - Revert "8295324: Fix race condition in junit test" > >This reverts commit fdec73d8f4ff21908bf99d191b76ffeed42bfb36. > - Revert "8295324: Adjust the J2DPrinterJobTest" > >This reverts commit 0723d2ebcd2c41d40005dbb1652c4ec96cfe4f8c. Ok. I have reverted all changes regarding refactoring and testing. - PR: https://git.openjdk.org/jfx/pull/916
Re: RFR: 8297554: Remove Scene.KeyHandler
On Thu, 24 Nov 2022 06:49:02 GMT, Michael Strauß wrote: > The `Scene.KeyHandler` class doesn't seem to have a clear purpose, mixing > focus handling with event propagation. Since #852, > `KeyHandler.setFocusVisible` is also called from mouse and touch event > handlers, which makes the purpose of the class even less pronounced. > > Moving the focus-related functionality next to the other focus functions in > the `Scene` class makes it easier to work with the code in the future. > > With the focus-related functions gone, `KeyHandler` only contains a single, > small method that is called from `Scene.processKeyEvent`. For simplicity, > this code can be rolled into `Scene.processKeyEvent` and the now-empty > `KeyHandler` class can be removed. It looks in a quick look, shall review in detail in couple days. - PR: https://git.openjdk.org/jfx/pull/962
Withdrawn: 8276170: Create Sources when publishing to Maven
On Fri, 29 Oct 2021 12:36:13 GMT, eduardsdv wrote: > Create sources.jars and attach they to the publish task, so that they can be > uploaded to the (e.g. maven) repository automatically. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jfx/pull/657
Re: RFR: 8276170: Create Sources when publishing to Maven
On Fri, 29 Oct 2021 12:36:13 GMT, eduardsdv wrote: > Create sources.jars and attach they to the publish task, so that they can be > uploaded to the (e.g. maven) repository automatically. It seems that no one needs it, so I am closing this PR. - PR: https://git.openjdk.org/jfx/pull/657
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Wed, 26 Oct 2022 06:52:17 GMT, John Hendrikx wrote: >> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1426: >> >>> 1424: if (s == null) return false; >>> 1425: Window w = s.getWindow(); >>> 1426: return w != null && w.isShowing(); >> >> Are you avoiding calling `get` on the property to avoid its initialization? > > Yeah, I actually copied this pattern from an older method that has since been > removed (`isWindowShowing`) in an effort to avoid initializing the property > if all you're doing is calling its getter. I personally don't mind either > way, but as it seems `Node` goes through every effort to delay > initialization, I followed that pattern. It does duplicate the logic of the > property which uses `flatMap`s to achieve the same. I would suggest adding a comment saying that this is done to avoid initialization. I'm not sure that it's that critical for performance, to be honest. - PR: https://git.openjdk.org/jfx/pull/830
Setting graphics of a Labeled does not show the Label correctly
Hi,
Given the following code
var cb1 = new Label("1");
var cb2 = new Label ("2");
var b1 = new Button("A");
cb1.setGraphic(b1);
b1.setOnAction(e -> {
if (b1.getParent() == cb1) {
// cb1.setGraphic(null);
cb2.setGraphic(b1);
} else {
// cb2.setGraphic(null);
cb1.setGraphic(b1);
}
});
Pressing the first button will move it (the graphic) to the second label,
however, pressing it again will not move it back to the first label. It's
required to set the graphics to null prior to moving them as in the
commented lines. This looks like a bug to me. I would think that when a
graphic is moved, it will appear in its new label immediately, like moving
a child. Apparently a single node can be the graphics for multiple Labeled
nodes, but it will appear only in 1. Is this intentional?
- Nir
Re: RFR: 8295324: JavaFX: Blank pages when printing [v4]
> 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 three additional commits since the last revision: - Revert "8295324: Apply patch with junit from the issue" This reverts commit c76b8207242a7af82f5515e49760158fa40da2a9. - Revert "8295324: Fix race condition in junit test" This reverts commit fdec73d8f4ff21908bf99d191b76ffeed42bfb36. - Revert "8295324: Adjust the J2DPrinterJobTest" This reverts commit 0723d2ebcd2c41d40005dbb1652c4ec96cfe4f8c. - Changes: - all: https://git.openjdk.org/jfx/pull/916/files - new: https://git.openjdk.org/jfx/pull/916/files/acd4825b..127cb03b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=916=03 - incr: https://webrevs.openjdk.org/?repo=jfx=916=02-03 Stats: 488 lines in 6 files changed: 6 ins; 471 del; 11 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
Re: Signing shared libraries and questions about loading
A solution via dependency management would be great, indeed. But since the problem at hand needs to be fixed, for now we are rolling with the "duck tape" build. I have follow-up question on that: We want to add a guard to our CI, checking that the JavaFX version in the POM and the JavaFx version in the jmods are always the same. Extracting the javafx version from the pom is easy, but for the jmods there is no apparent way. After looking into the JMODS, i found the file javafx.base.jmod/lib/javafx.properties which contains the JFX version. Also looking at the JDK bug tracker, this file is mentioned in several tickets. Can I rely on this file or is it "for internal use" only and can change anytime? Best wishes, Armin Am 21/11/2022 um 22:59 schrieb Scott Palmer: This is why I suggested long ago that there should be .jmod zips in Maven central or similar. We need a dependency management system that works with JMODs for exactly this reason. Zips in the current public repos would work. Scott On Nov 21, 2022, at 11:08 AM, Armin Schrenk wrote: Oh, thanks, didn't knew that. I tried the JMOD files provided by Gloun company with a local build. Works! But how can this be integrated into an automatic/CI build? Using a more or less arbitrary url pointing to a third-party website downloading a zip file of unknown structure would result in a "ducktape build" and is not very resilient against any changes. Furthermore, some build systems (i.e., ubuntu-ppa) do not allow external downloads. Does automatic build examples exist which are jlinking JFX to a custom JRE? Best wishes, Armin Am 16/11/2022 um 15:39 schrieb Kevin Rushforth: Leaving aside the question of signed libraries, if you are using jpackage / jlink to create a custom Java runtime with the JavaFX modules, then you should use the JMODs bundles and not the artifacts from Maven central. The maven modules are a handy convenience for developers, but not recommend for creating packaged applications. -- Kevin On 11/16/2022 6:29 AM, Armin Schrenk wrote: Hello, for our application, a customer reported that the shared libraries (in this case DLLs) used by JFX are unsigned and thus were blocked from loading, which blocks the app from starting. The culprit for blocking is a new security feature for Windows 11, Smart App Control (https://support.microsoft.com/en-gb/topic/what-is-smart-app-control-285ea03d-fa88-4d56-882e-6698afdb7003). Since this feature seems to be a future part of Windows, i suggest to sign the shared libs before the maven release. In our case, the archive javafx-graphics-*-win.jar contains the DLLs. Apart from this feature request, we want to fix the issue on our side. To do that, I investigated into the sharedLib loading of JFX. SharedLib Loading is in JFX is done with the NativeLibLoader (https://github.com/openjdk/jfx/blob/19+11/modules/javafx.graphics/src/main/java/com/sun/glass/utils/NativeLibLoader.java). It does the following to load the native lib: 1. Load the DLLs from a certain path (see below) 2. On Failure, load the DLLs from a resource (aka the jar) by extracting them to a cache directory and load them from there 3. On Failure, do other stuff not of interest Our app is modular (or as much as possible), thus, the DLLs were always loaded from the resource. But this extract-and-cache approach is unsatisfying from our point of view. The app uses a custom JRE via jlink and is packaged with jpackage, and we would like to place the sharedLibs at build time somewhere in the app directory. So I had to figure out the where to place the DLLs, or more specifically, what path is checked in Step 1 of shared libLoading. Reading the inline comment starting at line 117, it should be the same dir as the jar is placed. Unfortunately, this is not the case and i had to dig more through the code to find out. Our app has the following structure: |- JarsAndMods | - Mods | - modul1.jar | - modul2.jar | - ... | - javafx-graphics-XXX-win.jar | - ... | - nonModular1.jar | - nonModular2.jar | - ... |- executable According to the comment, the path to place all DLLs should be /JarsAndMods/Mods. But verbose logging showed and later proofed by the code, it has to be /JarsAndMods/bin. My questions are: Why does JFX require only for Windows the `../bin` directory? Additionally, this inline comment has a FIXME that Step 1 of SharedLibLoading should be removed in the future. Is there already an ETA? And lastly, I would love to see some Documentation for this. I can write it and create the PR, but where should it be placed? Best wishes, Armin
