Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
Hi Kevin, Yes, this is actually a pre-review mail to know if I am on the right path. Exposing these properties from ContextMenu can confuse users because the control has overloaded show() method. The second show() method doesn't use any of these properties and instead use screenX and screenY to place the ContextMenu. Given the circumstances, using Helper class seems the best solution to me. If someone has a better alternate solution, I am all ears. -- Abhinay From: Kevin Rushforth Sent: Friday, February 7, 2020 10:14 PM To: Abhinay Agarwal ; openjfx-dev@openjdk.java.net Subject: Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side I presume this is a "pre" review before sending out an actual review (which the Skara tooling will do automatically when you generate a pull request)... My concern with this approach is that it leaks what should be an implementation detail into a visible property of the control (and really, using the properties map in this manner is a bit of a workaround). An alternative would be to use package scope methods and a Helper class, which is done in many other places. Worth noting is that if this is information that a custom skin would also need, then some sort of way to make this visible to the Skin will eventually be needed. -- Kevin On 2/7/2020 8:33 AM, Abhinay Agarwal wrote: > The issue is caused because of a fix which was pushed as a part of > https://bugs.openjdk.java.net/browse/JDK-8159044 > > The root cause of the issue is that ContextMenuSkin#performPopupShifts > doesn't consider the cases where shiftX / shiftY can be negative, just as in > case of the example provided by Robert in 8228363. Moreover, these shifts are > only required for Side.TOP and Side.LEFT. > > My fix tries to find a solution to this problem using properties map as side, > dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double > dy) are not exposed by the control and cannot be accessed by Skin. I am not > sure if it is the best way to do it. > > Changes: > https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001 > > Any feedback on the changes are appreciated. > > Best regards, > Abhinay
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Fri, 7 Feb 2020 19:51:07 GMT, Kevin Rushforth wrote: >>> The OCA needs to be validated and recorded. >> >> https://www.oracle.com/technetwork/community/oca-486395.html#w now has >> `Bernhard Wiedemann - OpenJDK - bmwiedemann` >> >>> As for the bug, it has been transferred to the JDK project as >>> [JDK-8238650](https://bugs.openjdk.java.net/browse/JDK-8238650). >> >> Thanks, somehow I had not got an email notification for that. >> Also worth noting that this PR only fixes one of the sources of >> non-determinism in openjfx. Do I have to open a separate bug for each of >> them? > >> Do I have to open a separate bug for each of them? > > Every fix (meaning each pull request) needs a unique bug ID. As an optional override, I am OK with the concept of having a way for the build to be reproducible. FWIW, I have scripts that will unpack the modular jar files and diff each class as well as doing the same for a src.zip, and it's pretty easy to tell if only VersionInfo (which is the class that records the time stamps) has changed. I note that in practice, this is useful for a certain class of builds (e.g., CI or nightly test builds), but each released build is necessarily going to be different because you want a unique time stamp and build number associated with it. I will review this (probably some time next week) and would like @johanvos to review as well. - PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Fri, 7 Feb 2020 19:32:39 GMT, Bernhard M. Wiedemann wrote: >> The OCA needs to be validated and recorded. As for the bug, it has been >> transferred to the JDK project as >> [JDK-8238650](https://bugs.openjdk.java.net/browse/JDK-8238650). > >> The OCA needs to be validated and recorded. > > https://www.oracle.com/technetwork/community/oca-486395.html#w now has > `Bernhard Wiedemann - OpenJDK - bmwiedemann` > >> As for the bug, it has been transferred to the JDK project as >> [JDK-8238650](https://bugs.openjdk.java.net/browse/JDK-8238650). > > Thanks, somehow I had not got an email notification for that. > Also worth noting that this PR only fixes one of the sources of > non-determinism in openjfx. Do I have to open a separate bug for each of them? > Do I have to open a separate bug for each of them? Every fix (meaning each pull request) needs a unique bug ID. - PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Fri, 7 Feb 2020 13:43:50 GMT, Kevin Rushforth wrote: >> So the contributor agreement is done. How can I find the bug? > > The OCA needs to be validated and recorded. As for the bug, it has been > transferred to the JDK project as > [JDK-8238650](https://bugs.openjdk.java.net/browse/JDK-8238650). > The OCA needs to be validated and recorded. https://www.oracle.com/technetwork/community/oca-486395.html#w now has `Bernhard Wiedemann - OpenJDK - bmwiedemann` > As for the bug, it has been transferred to the JDK project as > [JDK-8238650](https://bugs.openjdk.java.net/browse/JDK-8238650). Thanks, somehow I had not got an email notification for that. Also worth noting that this PR only fixes one of the sources of non-determinism in openjfx. Do I have to open a separate bug for each of them? - PR: https://git.openjdk.java.net/jfx/pull/99
RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
Allow to override buildDate with `SOURCE_DATE_EPOCH` in order to make builds reproducible. See https://reproducible-builds.org/ for why this is good and https://reproducible-builds.org/specs/source-date-epoch/ for the definition of this variable. This PR was done while working on reproducible builds for openSUSE. - Commits: - e7f71fb9: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH Changes: https://git.openjdk.java.net/jfx/pull/99/files Webrev: https://webrevs.openjdk.java.net/jfx/99/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8238650 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/99.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/99/head:pull/99 PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Wed, 29 Jan 2020 12:48:35 GMT, Kevin Rushforth wrote: >> /signed >> >> I filed one bug and got review ID : 9063488 > >> I filed one bug and got review ID : 9063488 > > OK. So the contributor agreement is done. How can I find the bug? - PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Wed, 29 Jan 2020 12:40:05 GMT, Bernhard M. Wiedemann wrote: >> In addition to a signed OCA, we will need a bug filed to track this. Please >> read the [CONTRIBUTING >> guidelines](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md). > > /signed > > I filed one bug and got review ID : 9063488 > I filed one bug and got review ID : 9063488 OK. - PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Wed, 29 Jan 2020 08:58:47 GMT, Bernhard M. Wiedemann wrote: > Allow to override buildDate with `SOURCE_DATE_EPOCH` > in order to make builds reproducible. > See https://reproducible-builds.org/ for why this is good > and https://reproducible-builds.org/specs/source-date-epoch/ > for the definition of this variable. > > This PR was done while working on reproducible builds for openSUSE. In addition to a signed OCA, we will need a bug filed to track this. Please read the [CONTRIBUTING guidelines](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md). - PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Fri, 7 Feb 2020 12:11:24 GMT, Bernhard M. Wiedemann wrote: >>> I filed one bug and got review ID : 9063488 >> >> OK. > > So the contributor agreement is done. How can I find the bug? The OCA needs to be validated and recorded. As for the bug, it has been transferred to the JDK project as [JDK-8238650](https://bugs.openjdk.java.net/browse/JDK-8238650). - PR: https://git.openjdk.java.net/jfx/pull/99
Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH
On Wed, 29 Jan 2020 12:33:00 GMT, Kevin Rushforth wrote: >> Allow to override buildDate with `SOURCE_DATE_EPOCH` >> in order to make builds reproducible. >> See https://reproducible-builds.org/ for why this is good >> and https://reproducible-builds.org/specs/source-date-epoch/ >> for the definition of this variable. >> >> This PR was done while working on reproducible builds for openSUSE. > > In addition to a signed OCA, we will need a bug filed to track this. Please > read the [CONTRIBUTING > guidelines](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md). /signed I filed one bug and got review ID : 9063488 - PR: https://git.openjdk.java.net/jfx/pull/99
Re: [Rev 01] RFR: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
On Fri, 7 Feb 2020 12:32:26 GMT, Ambarish Rapte wrote: >> modules/javafx.graphics/src/main/java/com/sun/pisces/JavaSurface.java line >> 46: >> >>> 45: initialize(dataType, width, height); >>> 46: addDisposerRecord(); >>> 47: } >> >> Should this be called from the superclass instead? It works as-is, but if >> there were ever another subclass added, it would have to be replicated there >> as well. > > `AbstractSurface.nativePtr` is needed to create the > `AbstractSurfaceDisposerRecord`. > It is created and set by the native method `initialize(dataType, width, > height);` invoked at line 45 in the `JavaSurface` constructor. > It is the reason `addDisposerRecord()` is needed here. Oh, I see. The native initialize method in the subclass is writing into a private field in the superclass. Can you add a comment to this effect, since it isn't obvious without reading the native code? - PR: https://git.openjdk.java.net/jfx/pull/66
Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
On Sat, 21 Dec 2019 15:36:14 GMT, Oliver Kopp wrote: > This is a WIP PR. Requesting for comments. > > I could not replicate the test given at > https://bugs.openjdk.java.net/browse/JDK-8176270 without TestFX. I > nevertheless let my try to replicate the @Test things in here. > > The fix is just a wild guess without really understanding the side effects of > `.addListener`. I left comments inline. This will need changes. Also, you will need to provide a test that fails without the fix and passes with the fix. modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java line 168: > 167: // Ensure that the last character to get is within the > bounds of the txt string > 168: if (end >= start + length) end = length-1; > 169: // In case the start is after the whole txt, nothing > valid is selected. Thus, return the default. First, I think the existing test is simply wrong: Why should the `start` value matter when checking whether the `end` value needs to be clamped -- it's an `end` point not a number of characters. I think the entire problem might be the fact that it is comparing against `start + length`. Second, the value to be clamped to was correct before your change. The `substring` method to which `end` is passed is documented as subtracting 1. I think the test should be something like: if (end > length) end = length; modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java line 170: > 169: // In case the start is after the whole txt, nothing > valid is selected. Thus, return the default. > 170: if (start >= length) return ""; > 171: return txt.substring(start, end); This change seems OK, and might be clearer and the existing code, but the existing code is correct, and produces the same effect. modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 2088: > 2087: Semaphore semaphore = new Semaphore(0); > 2088: Platform.runLater(semaphore::release); > 2089: semaphore.acquire(); Since controls tests run using the StubToolkit, `Platform.runLater` does not do what you expect. If you need to run a pulse to get some code to run that normally would run as part of pulse, you can call that method directly. See other controls tests for how this is done. - Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/73
Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
I presume this is a "pre" review before sending out an actual review (which the Skara tooling will do automatically when you generate a pull request)... My concern with this approach is that it leaks what should be an implementation detail into a visible property of the control (and really, using the properties map in this manner is a bit of a workaround). An alternative would be to use package scope methods and a Helper class, which is done in many other places. Worth noting is that if this is information that a custom skin would also need, then some sort of way to make this visible to the Skin will eventually be needed. -- Kevin On 2/7/2020 8:33 AM, Abhinay Agarwal wrote: The issue is caused because of a fix which was pushed as a part of https://bugs.openjdk.java.net/browse/JDK-8159044 The root cause of the issue is that ContextMenuSkin#performPopupShifts doesn't consider the cases where shiftX / shiftY can be negative, just as in case of the example provided by Robert in 8228363. Moreover, these shifts are only required for Side.TOP and Side.LEFT. My fix tries to find a solution to this problem using properties map as side, dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double dy) are not exposed by the control and cannot be accessed by Skin. I am not sure if it is the best way to do it. Changes: https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001 Any feedback on the changes are appreciated. Best regards, Abhinay
RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
The issue is caused because of a fix which was pushed as a part of https://bugs.openjdk.java.net/browse/JDK-8159044 The root cause of the issue is that ContextMenuSkin#performPopupShifts doesn't consider the cases where shiftX / shiftY can be negative, just as in case of the example provided by Robert in 8228363. Moreover, these shifts are only required for Side.TOP and Side.LEFT. My fix tries to find a solution to this problem using properties map as side, dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double dy) are not exposed by the control and cannot be accessed by Skin. I am not sure if it is the best way to do it. Changes: https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001 Any feedback on the changes are appreciated. Best regards, Abhinay
Re: [Integrated] RFR: 8236839: System menubar removed when other menubars are created or modified
Changeset: a74137a1 Author:Johan Kaving Committer: Kevin Rushforth Date: 2020-02-07 14:43:29 + URL: https://git.openjdk.java.net/jfx/commit/a74137a1 8236839: System menubar removed when other menubars are created or modified Reviewed-by: kcr, aghaisas ! modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java ! modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/MenuBarSkinTest.java
RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
This is a WIP PR. Requesting for comments. I could not replicate the test given at https://bugs.openjdk.java.net/browse/JDK-8176270 without TestFX. I nevertheless let my try to replicate the @Test things in here. The fix is just a wild guess without really understanding the side effects of `.addListener`. - Commits: - 492cb6ca: 8176270: Fix TextInputControl String boundaries Changes: https://git.openjdk.java.net/jfx/pull/73/files Webrev: https://webrevs.openjdk.java.net/jfx/73/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8176270 Stats: 108 lines in 2 files changed: 106 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/73.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/73/head:pull/73 PR: https://git.openjdk.java.net/jfx/pull/73
Re: [Rev 01] RFR: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
On Wed, 5 Feb 2020 22:42:19 GMT, Kevin Rushforth wrote: >> The pull request has been updated with a new target base due to a merge or a >> rebase. > > modules/javafx.graphics/src/main/java/com/sun/pisces/JavaSurface.java line 46: > >> 45: initialize(dataType, width, height); >> 46: addDisposerRecord(); >> 47: } > > Should this be called from the superclass instead? It works as-is, but if > there were ever another subclass added, it would have to be replicated there > as well. `AbstractSurface.nativePtr` is needed to create the `AbstractSurfaceDisposerRecord`. It is created and set by the native method `initialize(dataType, width, height);` invoked at line 45 in the `JavaSurface` constructor. It is the reason `addDisposerRecord()` is needed here. - PR: https://git.openjdk.java.net/jfx/pull/66
Re: [Rev 01] RFR: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
On Wed, 5 Feb 2020 22:48:45 GMT, Kevin Rushforth wrote:
>> The pull request has been updated with a new target base due to a merge or a
>> rebase.
>
> modules/javafx.graphics/src/main/native-prism-sw/JAbstractSurface.c line 189:
>
>> 188: static void
>> 189: disposeNativeImpl(JNIEnv* env, jobject objectHandle) {
>> 190: AbstractSurface* surface;
>
> Since you removed `disposeNativeImpl`, you can also remove the static
> declaration on [line
> 40](https://github.com/openjdk/jfx/blob/0591e41f9106e93ae957cb9d80cf2e9e2c6fd805/modules/javafx.graphics/src/main/native-prism-sw/JAbstractSurface.c#L40).
I shall do this in next commit.
-
PR: https://git.openjdk.java.net/jfx/pull/66
Re: [Rev 01] RFR: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
On Wed, 5 Feb 2020 22:43:13 GMT, Kevin Rushforth wrote: >> The pull request has been updated with a new target base due to a merge or a >> rebase. > > modules/javafx.graphics/src/main/java/com/sun/pisces/AbstractSurface.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Best to revert this or make it 2020. I will make it 2020 in all files. - PR: https://git.openjdk.java.net/jfx/pull/66
Re: RFR: 8227619: Potential memory leak in javafx.scene.control.ListView
On Wed, 5 Feb 2020 23:29:50 GMT, Kevin Rushforth wrote: >> `ListView` does not get GCed because `SelectedItemsReadOnlyObservableList` >> adds a `ListChangeListener` to the (`ObservableList`) items of `ListView`. >> >> Adding a `WeakListChangeListener` instead of `ListChangeListener` fixes the >> issue. >> >> Added a unit test and verified that existing tests do not fail due to this >> change. > > Presuming that using a weak reference causes no other issues, the fix looks > fine and will resolve the leak. How confident are you that there are no cases > where the listener might be prematurely garbage collected? > > > How confident are you that there are no cases where the listener might be > prematurely garbage collected? The listener is added by `ListViewBitSetSelectionModel`. The selection model is set only once for a `ListView` and we do not unset it. So the change looks very safe to me. - PR: https://git.openjdk.java.net/jfx/pull/84
Re: [Rev 02] RFR: 8236839: System menubar removed when other menubars are created or modified
On Fri, 7 Feb 2020 10:54:44 GMT, Johan Kaving wrote: >> This pull request fixes the sceneProperty listener in `MenuBarSkin` so that >> we leave the >> current system menubar alone when other menubars are changed. >> >> It also adds a test case that reproduces the problem before the fix. > > Previous commits in this pull request have been removed, probably due to a > force push. The incremental views will show differences compared to the > previous content of the PR. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/86
