Re: RFR: 8206430: Use consistent pattern for startup in FX system tests
On Tue, 15 Nov 2022 18:00:59 GMT, Andy Goryachev wrote:
> 1. Introduced the following utility methods:
> - Util.launch
> - Util.shutdown
> - Util.waitForLatch
> 2. Fixed the out-of order calls to Stage.hide() and Platform.exit() in many
> tests' shutdowns.
> 3. Replaced local waitForLatch copies with Util.waitForLatch
I left a couple quick comments on the new utility methods in `test.util.Util`.
Have you run a full test (including robot) on all three platforms?
> Fixed the out-of order calls to Stage.hide() and Platform.exit() in many
> tests' shutdowns.
I am not aware of any out of order calls. Perhaps you are referring to the
following pattern:
Platform.runLater(() -> stage.hide());
Platform.exit();
That is actually valid, since `Platform.exit()` can be called on any thread.
The FX runtime doesn't exit until all pending runnables are run. Doing the exit
on the FX application thread is fine, too.
tests/system/src/test/java/test/util/Util.java line 317:
> 315: * @param args - command line arguments
> 316: */
> 317: public static void launch (
This looks like a useful utility. I think it would be helpful to either remove
the timeout parameter entirely, or else have a variant of this that doesn't
take the timeout value, and use a default timeout value. Most tests use 10 or
15 seconds so standardizing on 15 seems reasonable.
Also, many of the tests use `Platform.startup` since they don't need to launch
an application. Have you looked at providing a similar utility method for those
cases?
tests/system/src/test/java/test/util/Util.java line 338:
> 336: public static void shutdown(Stage... stages) {
> 337: // why isn't runAndWait() exposed as a public API?
> 338: PlatformImpl.runAndWait(() -> {
Not providing a public `runAndWait` was a deliberate choice. In any case, tests
or utilities that need to wait should use the `runAndWait` method in this Util
class in preference to `PlatformImpl`. For one thing it will throw exceptions
back to the caller where as the `PlatformImpl` method will not.
I think `Platform.runLater` should be sufficient here, but as long as the tests
are still stable when using `runAndWait`, I don't have a strong objection.
-
PR: https://git.openjdk.org/jfx/pull/950
Re: RFR: 8206430: Use consistent pattern for startup in FX system tests
On Tue, 15 Nov 2022 18:00:59 GMT, Andy Goryachev wrote: > 1. Introduced the following utility methods: > - Util.launch > - Util.shutdown > - Util.waitForLatch > 2. Fixed the out-of order calls to Stage.hide() and Platform.exit() in many > tests' shutdowns. > 3. Replaced local waitForLatch copies with Util.waitForLatch This is a large enough change that it really could use two reviewers. - PR: https://git.openjdk.org/jfx/pull/950
RFR: 8206430: Use consistent pattern for startup in FX system tests
1. Introduced the following utility methods: - Util.launch - Util.shutdown - Util.waitForLatch 2. Fixed the out-of order calls to Stage.hide() and Platform.exit() in many tests' shutdowns. 3. Replaced local waitForLatch copies with Util.waitForLatch - Commit messages: - 8206430: wait for latch - 8206430: exit test - 8206430: wait for latch - 8206430: t..w - 8206430: r..s - 8206430: h..p - 8206430: util.shutdown - 8206430: c..f - 8206430: util.launch Changes: https://git.openjdk.org/jfx/pull/950/files Webrev: https://webrevs.openjdk.org/?repo=jfx=950=00 Issue: https://bugs.openjdk.org/browse/JDK-8206430 Stats: 2255 lines in 91 files changed: 603 ins; 1094 del; 558 mod Patch: https://git.openjdk.org/jfx/pull/950.diff Fetch: git fetch https://git.openjdk.org/jfx pull/950/head:pull/950 PR: https://git.openjdk.org/jfx/pull/950
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 This will need to be carefully tested on all platforms, since `WindowStage::requestFocus` will no longer ever be called when a Stage is first created and shown. If it isn't doing anything useful (which seems the case on Linux), then in practice, this might not matter, but that's where the testing comes in. - PR: https://git.openjdk.org/jfx/pull/940
Re: Discussion: Naming API method
The new operation returns a new ObservableValue that is only
meaningfully "exists" when the condition holds.
If the condition doesn't hold, the effect is as if the operation
wasn't invoked at all, i.e. it doesn't meaningfully exist.
With this in mind, here's another option:
label.textProperty().bind(longLivedProperty.withScope(container::isShownProperty));
On Mon, Nov 14, 2022 at 3:53 PM John Hendrikx wrote:
>
> Hi,
>
> I'm working on https://github.com/openjdk/jfx/pull/830 where I asked for
> some opinions on the naming of a new method I'd like to introduce in
> ObservableValue.
>
> I wrote a (perhaps too large) comment about the possible names and
> rationales: https://github.com/openjdk/jfx/pull/830#issuecomment-1304846220
>
> I'd like to ask what others think what would be a good name for this new
> method (Observable#when in the PR) in order to move the PR forward, as I
> think it offers a very compelling feature to JavaFX (moving from weak
> reference to deterministic behavior when it comes to listener
> management). My opinion has always been that using weak listeners for
> listener management is a crutch that relies far too much on the internal
> workings of the JVM and Garbage Collector which offer no guarantees as
> to the timely clean up of these references and the listeners related to
> them.
>
> Leading contenders are (but not limited to these, if you have a better
> name):
>
> 1) conditionOn
>
> 2) updateWhen
>
> 3) when
>
> 4) whenever
>
> Usage in code is nearly always going to be something like these constructs:
>
>// keeps text property in sync with longLivedProperty when label
> is shown:
> label.textProperty().bind(longLivedProperty.**when**(label::isShownProperty));
>
>// keeps text property in sync with longLivedProperty when
> container is shown:
> label.textProperty().bind(longLivedProperty.**when**(container::isShownProperty));
>
>
> It can also be used to make a listener only actively listen when a
> condition is met (the listener is added/removed immediately when the
> condition changes, facilitating GC):
>
>// listen to changes of longLivedProperty when container is shown:
>longLivedProperty.when(container::isShownProperty)
> .addListener((obs, old, current) -> { ... change listener
> ... });
>
> Or it can be used to disable updates temporarily (or permanently):
>
> BooleanProperty allowUpdates = new SimpleBooleanProperty(true)
>
> // keeps text property in sync when updates are allowed:
> name.textProperty().bind(model.title.when(allowUpdates));
> detail.textProperty().bind(model.subtitle.when(allowUpdates));
> asyncImageProperty.imageHandleProperty().bind(model.imageHandle.when(allowUpdates));
>
> This last example can be useful in Skin#dispose, but has uses outside of
> skins as well, for example when you want to prevent updates until things
> have settled down.
>
> Thanks for reading!
>
> --John
>
>
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v11]
> Introduction > > There is a number of places where various listeners (strong as well as weak) > are added, to be later disconnected in one go. For example, Skin > implementations use dispose() method to clean up the listeners installed in > the corresponding Control (sometimes using > LambdaMultiplePropertyChangeListenerHandler class). > > This pattern is also used by app developers, but there is no public utility > class to do that, so every one must invent their own, see for instance > https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java > > Proposal > > It might be beneficial to create a class (ListenerHelper, feel free to > suggest a better name) which: > > - provides convenient methods like addChangeListener, > addInvalidationListener, addWeakChangeListener, etc. > - keeps track of the listeners and the corresponding ObservableValues > - provides a single disconnect() method to remove all the listeners in one go. > - optionally, it should be possible to add a lambda (Runnable) to a group of > properties > - optionally, there should be a boolean flag to fire the lambda immediately > - strongly suggest implementing an IDisconnectable functional interface with > a single disconnect() method > > Make it Public Later > > Initially, I think we could place the new class in package > com.sun.javafx.scene.control; to be made publicly accessible later. > > Where It Will Be Useful > > [JDK-8294589](https://bugs.openjdk.org/browse/JDK-8294589) "MenuBarSkin: > memory leak when changing skin" > and other skins, as a replacement for > LambdaMultiplePropertyChangeListenerHandler. > > https://github.com/openjdk/jfx/pull/908#:~:text=19%20hours%20ago-,8295175%3A%20SplitPaneSkinSkin%3A%20memory%20leak%20when%20changing%20skin%20%23911,-Draft > > https://github.com/openjdk/jfx/pull/914 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294809: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/908/files - new: https://git.openjdk.org/jfx/pull/908/files/2a295c24..8cef69c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=908=10 - incr: https://webrevs.openjdk.org/?repo=jfx=908=09-10 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/908.diff Fetch: git fetch https://git.openjdk.org/jfx pull/908/head:pull/908 PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v9]
On Tue, 15 Nov 2022 06:35:05 GMT, Ajit Ghaisas wrote: >> Exactly. In its current form, `ListenerHelper` is a utility helper class for >> Skins, and should be reviewed with that in mind. As discussed in previous >> comments, there are several things that will need to change when/if this is >> proposed as a more general utility. We can defer that discussion, since this >> is entirely an internal helper class for now. >> >> As a next step, after this is integrated and before any discussion of making >> this a public utility, it can be used as a replacement for >> `LambdaMultiplePropertyChangeListenerHandler` -- see >> [JDK-8296076](https://bugs.openjdk.org/browse/JDK-8296076). > > Although it is an internal (non-public) utility class, once introduced, it > will be tempting to use it from non-Skin code as well. > If the current scope is limited to provide "a utility helper class for Skins" > then how about doing one of two things- > 1) Mention the current intention and possible future modifications as a block > comment above the `ListenerHelper` class > 2) Keep only common methods in`ListenerHelper` class and move `Skin` specific > code to a class derived from `ListenerHelper`. Skins should start using this > derived class. added comment for #1 option. - PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v9]
On Tue, 15 Nov 2022 06:17:21 GMT, Ajit Ghaisas wrote: >> Right, there is no way the accessor can be null, since it is initialized at >> class load time in the static initializer, We do this in many other places >> (e.g., Node.java). > > The `accessor` cannot be null **only if** it is used from *Skins. > In the current form, as we provide a no-argument public constructor, it is > possible to create an object of `ListenerHelper` without calling setAccessor. > Many unit tests added as part of this PR create such `ListenerHelper` objects. The `accessor` is used **only** if used from *Skins, meaning to get it from a Skin, one needs to invoke `get(SkinBase)` method, and it's magically not a null there! It is perfectly fine to create an instance of ListenerHelper using the constructor. The `accessor` will be null, of course, but it's ok because it will never be used until one calls `get(SkinBase)` ... and see above. - PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8221708 Update Eclipse project files [v4]
> > - the screenshots are outdated, using much older version of Eclipse, the > UI has been changed a bit. The latest Eclipse won't work with java19 > without a separate plugin, but i am sure they will fix it soon. > Yes, I will need to delete all the projects and re-import in order to create the new screenshot. The separate plugin support is explained under "Configure Eclipse to use the latest JDK". Each Eclipse release requires a plugin for the simultaneously released Java version, It is "fixed" in the next version, but then you need another plugin for the new release. - there is an option to 'ignore projects already in the workspace' and I > think it must be turned on (I was getting a warning). > If you are importing for the first time, no project will already be in the workspace, so it doesn't matter. If you are re-importing it's up to you if you want to ignore them or not, I would say. - we probably should mention that the gradle build should be run after > initial checkout, including *sdk* and *apps*. This will load and build > the dependencies (lucene). > That is explained in the main page - https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX, or at least should be, as it isn't an Eclipse-specific step. There is a mention of this in the Using Eclipse section. On Tue, Nov 15, 2022 at 6:54 PM Andy Goryachev wrote: > Nir, thank you so much! > > > > A few minor corrections might be needed. > > > > in Import the Eclipse Projects: > > - the screenshots are outdated, using much older version of Eclipse, the > UI has been changed a bit. The latest Eclipse won't work with java19 > without a separate plugin, but i am sure they will fix it soon. > > - there is an option to 'ignore projects already in the workspace' and I > think it must be turned on (I was getting a warning). > > - we probably should mention that the gradle build should be run after > initial checkout, including *sdk* and *apps*. This will load and build > the dependencies (lucene). > > > > Perhaps we should also add a section dedicated to configuring error > levels, as the default configuration turns off important warnings and > enables too many unimportant ones. > > > > Thank you again > > -andy > > > > > > > > *From: *openjfx-dev on behalf of Nir > Lisker > *Date: *Monday, 2022/11/14 at 18:09 > *To: *openjfx-dev@openjdk.org > *Subject: *Re: RFR: 8221708 Update Eclipse project files [v4] > > On Sun, 13 Nov 2022 02:27:55 GMT, John Hendrikx > wrote: > > >> See https://bugs.openjdk.org/browse/JDK-8221708 > > > > John Hendrikx has updated the pull request incrementally with one > additional commit since the last revision: > > > > Fix controls classpath > > I updated > https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse > . > > - > > PR: https://git.openjdk.org/jfx/pull/930 >
Re: RFR: 8221708 Update Eclipse project files [v4]
Nir, thank you so much! A few minor corrections might be needed. in Import the Eclipse Projects: - the screenshots are outdated, using much older version of Eclipse, the UI has been changed a bit. The latest Eclipse won't work with java19 without a separate plugin, but i am sure they will fix it soon. - there is an option to 'ignore projects already in the workspace' and I think it must be turned on (I was getting a warning). - we probably should mention that the gradle build should be run after initial checkout, including sdk and apps. This will load and build the dependencies (lucene). Perhaps we should also add a section dedicated to configuring error levels, as the default configuration turns off important warnings and enables too many unimportant ones. Thank you again -andy From: openjfx-dev on behalf of Nir Lisker Date: Monday, 2022/11/14 at 18:09 To: openjfx-dev@openjdk.org Subject: Re: RFR: 8221708 Update Eclipse project files [v4] On Sun, 13 Nov 2022 02:27:55 GMT, John Hendrikx wrote: >> See https://bugs.openjdk.org/browse/JDK-8221708 > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix controls classpath I updated https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse. - PR: https://git.openjdk.org/jfx/pull/930
RFR: 8297042: gradle -PBUILD_SDK_FOR_TEST=false fails with gradle 7.6
This PR fixes our `build.gradle` script so it can work with gradle 7.6. When doing a test build with gradle 7.6 RC3: $ gradle sdk $ gradle --info -PBUILD_SDK_FOR_TEST=false test We get the following error: FAILURE: Build failed with an exception. * Where: Build file 'C:\Users\kcr\javafx\jfx-kcr\jfx\rt\build.gradle' line: 615 * What went wrong: A problem occurred evaluating root project 'rt'. > No signature of method: > org.gradle.execution.taskgraph.DefaultTaskExecutionGraph.useFilter() is > applicable for argument types: > (build_32ube911nql8mvr8torfp363j$_run_closure1) values: > [build_32ube911nql8mvr8torfp363j$_run_closure1@1679a7fe] As noted in JBS, setting the gradle `-PBUILD_SDK_FOR_TEST=false` flag can be used when running tests to avoid a complete build of the sdk. The fix is to remove the use of `gradle.taskGraph.useFilter`, which is not public API, but is an implementation detail that happened to "work" up until recently, but stopped working some time between gradle 7.3 and gradle 7.6. I say "work" in quotes, because there is effectively no difference in what gets run because of the dependency on the shims. The following two GHA test runs show the problem when running with gradle 7.6 RC3 and the fix: [test-build-gradle-7.6](https://github.com/kevinrushforth/jfx/actions/runs/3470969280) : test build run using gradle 7.6 RC3 without the fix from this PR; it fails as expected [test-build-gradle-7.6+8297042](https://github.com/kevinrushforth/jfx/actions/runs/3470983142) : test build run using gradle 7.6 RC3 without the fix from this PR; it passes on all platforms Additionally, the GHA test run for _this_ PR shows that it works fine with gradle 7.3. - Commit messages: - 8297042: gradle -PBUILD_SDK_FOR_TEST=false fails with gradle 7.6 Changes: https://git.openjdk.org/jfx/pull/949/files Webrev: https://webrevs.openjdk.org/?repo=jfx=949=00 Issue: https://bugs.openjdk.org/browse/JDK-8297042 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/949.diff Fetch: git fetch https://git.openjdk.org/jfx pull/949/head:pull/949 PR: https://git.openjdk.org/jfx/pull/949
Withdrawn: 8296618: Skip failing test SwingNodeDnDMemoryLeakTest until JDK-8285503 is fixed
On Wed, 9 Nov 2022 10:08:28 GMT, pandaapo wrote: > Fixes [JDK-8296618](https://bugs.openjdk.org/browse/JDK-8296618) This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jfx/pull/942
Re: RFR: 8296618: Skip failing test SwingNodeDnDMemoryLeakTest until JDK-8285503 is fixed
On Wed, 9 Nov 2022 10:08:28 GMT, pandaapo wrote: > Fixes [JDK-8296618](https://bugs.openjdk.org/browse/JDK-8296618) Now that [JDK-8285503](https://bugs.openjdk.org/browse/JDK-8285503) has been fixed, [JDK-8296618](https://bugs.openjdk.org/browse/JDK-8296618) has been closed as "not an issue". I am therefore closing this PR. - PR: https://git.openjdk.org/jfx/pull/942
Re: Discussion: Naming API method
While rereading the proposed docs for the new method, the following
stood out to me:
"The returned ObservableValue only observes this value when condition
holds true."
So I agree that "updateWhen" isn't really accurate, but I don't care for
"onCondition" either.
I think "whenever" would be OK, but what about "observesWhen" (or maybe
"observedWhen") as a name?
-- Kevin
On 11/14/2022 9:47 AM, Andy Goryachev wrote:
I'd pick "whenever" or "onCondition".
"updateWhen" sounds like an update, which I think it is not. "when"
seems too vague.
Disclaimer: English is not my native language.
-andy
*From: *openjfx-dev on behalf of Kevin
Rushforth
*Date: *Monday, 2022/11/14 at 09:40
*To: *openjfx-dev@openjdk.org
*Subject: *Re: Discussion: Naming API method
I also think this will be a useful feature to get into JavaFX.
As for the name of the method, the only one of them I don't like is
"conditionOn". That name doesn't suggest (to me anyway) what its purpose
is. I think any of the ones with "when" in the name would work. I have a
slight preference for "updateWhen", but could be talked into one of the
others.
-- Kevin
On 11/14/2022 6:52 AM, John Hendrikx wrote:
> Hi,
>
> I'm working on https://github.com/openjdk/jfx/pull/830 where I asked
> for some opinions on the naming of a new method I'd like to introduce
> in ObservableValue.
>
> I wrote a (perhaps too large) comment about the possible names and
> rationales:
> https://github.com/openjdk/jfx/pull/830#issuecomment-1304846220
>
> I'd like to ask what others think what would be a good name for this
> new method (Observable#when in the PR) in order to move the PR
> forward, as I think it offers a very compelling feature to JavaFX
> (moving from weak reference to deterministic behavior when it comes to
> listener management). My opinion has always been that using weak
> listeners for listener management is a crutch that relies far too much
> on the internal workings of the JVM and Garbage Collector which offer
> no guarantees as to the timely clean up of these references and the
> listeners related to them.
>
> Leading contenders are (but not limited to these, if you have a better
> name):
>
> 1) conditionOn
>
> 2) updateWhen
>
> 3) when
>
> 4) whenever
>
> Usage in code is nearly always going to be something like these
> constructs:
>
> // keeps text property in sync with longLivedProperty when label
> is shown:
>
label.textProperty().bind(longLivedProperty.**when**(label::isShownProperty));
>
>
> // keeps text property in sync with longLivedProperty when
> container is shown:
>
label.textProperty().bind(longLivedProperty.**when**(container::isShownProperty));
>
>
> It can also be used to make a listener only actively listen when a
> condition is met (the listener is added/removed immediately when the
> condition changes, facilitating GC):
>
> // listen to changes of longLivedProperty when container is shown:
> longLivedProperty.when(container::isShownProperty)
> .addListener((obs, old, current) -> { ... change listener
> ... });
>
> Or it can be used to disable updates temporarily (or permanently):
>
> BooleanProperty allowUpdates = new SimpleBooleanProperty(true)
>
> // keeps text property in sync when updates are allowed:
> name.textProperty().bind(model.title.when(allowUpdates));
> detail.textProperty().bind(model.subtitle.when(allowUpdates));
>
asyncImageProperty.imageHandleProperty().bind(model.imageHandle.when(allowUpdates));
>
>
> This last example can be useful in Skin#dispose, but has uses outside
> of skins as well, for example when you want to prevent updates until
> things have settled down.
>
> Thanks for reading!
>
> --John
>
>
Re: RFR: 8221708 Update Eclipse project files [v4]
On Tue, 15 Nov 2022 02:05:33 GMT, Nir Lisker wrote: > I updated > https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse. Looks good, nicely explained :) - PR: https://git.openjdk.org/jfx/pull/930
