Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v6]
On Thu, 11 Jan 2024 00:20:19 GMT, John Hendrikx wrote: >> No, the explicit goal of this construction is to set active to false (in >> case it exists) so that existing listeners can be released; followed by >> creating a new active property that is by default set to true until >> `setMenus` is called again (e.g. after unfocus/focus events). >> I noticed however that it is not as simple as that, and the current PR >> introduces regression, as the `SystemMenuBarTest.testMemoryLeak` test is now >> failing. Listeners are not only registered when `setMenus` is called, but >> also when menu(Item)s are added to menus. I'm not sure if a single `active` >> property can address this, as we don't want to remove all listeners for all >> menuitems in case a particular menuitem is added to a particular menu. >> @hjohn does that make sense? > > @johanvos I don't think what I wrote above (option 1) will work -- there will > still be a reference from the `active` property that keeps the menu item > alive; a reference is indeed removed when `hasParent` becomes `false`, but > there is another (same reason we had to recreate the active property)... I > will think on this some more over the weekend. I think you already solved the issue. - PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1512255445
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v10]
On Mon, 4 Mar 2024 19:26:37 GMT, Jose Pereda wrote: >> Johan Vos has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 11 additional commits >> since the last revision: >> >> - Merge branch 'master' into 8319779-systemmenu >> - Add additional test for IOOBE detection. >>This test comes from JDK-8323787 >> - Revert some of the conditional bindings. >>Clear menu construction when an menuitem that is a menu needs to be >> removed >>Add a test for the latter >> - Merge remote-tracking branch 'upstream/master' into 8319779-systemmenu >> - Cleanup test >> - Add shim class so that we can access the references to >> com.sun.glass.ui.Menu instances. >>Add a test to make sure those references are gone. >> - Revert WeakInvalidationListeners and use new listener resource management >> approach. >> - Fix more memoryleaks due to listeners never being unregistered. >> - These changes are related to JBS-8318841 so we want to have that code in >>as well. >> >>Merge branch 'master' into 8319779-systemmenu >> - process reviewers comments >> - ... and 1 more: https://git.openjdk.org/jfx/compare/f65f11d2...ec7308df > > modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java > line 226: > >> 224: mb.textProperty().when(active).addListener(valueModel -> >> glassMenu.setTitle(parseText(mb))); >> 225: mb.disableProperty().when(active).addListener(valueModel -> >> glassMenu.setEnabled(!mb.isDisable())); >> 226: >> mb.mnemonicParsingProperty().when(active).addListener(valueModel -> >> glassMenu.setTitle(parseText(mb))); > > Since you are not calling `get()` from the `when` resulting observable (to > allow for invalidation), these need to use a `ChangeListener` instead of an > `InvalidationListener`, otherwise, while `active` doesn't change, any change > in those menuBase properties will trigger just one notification. > > Alternative, just replace `addListener` with `subscribe` (as it internally > uses a `ChangeListener`). > > @hjohn does this make sense to you? This could be a problem normally, but I think in this case you won't be able to get this to produce incorrect results. `parseText` is accessing the property (and it calls `isMnemonicParsing`), but only if the text is not empty. It's sort of "safe" as an empty text can't contain a mnemonic anyway, and you'd need to change the text at a minimum first to see the effect of turning mnemonic parsing on/off. Changing the text would trigger the `textProperty` listener, which will call `parseText` and read the mnemonic property. A change listener would be more obviously correct, but it is not strictly needed here. (not all `subscribe`s use change listener, the one that takes a `Runnable` uses invalidation) - PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1512252882
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
On Sat, 24 Feb 2024 17:38:11 GMT, Marius Hanl wrote: >> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all >> other implementations of the cell. >> >> This also means that the `TreeTableRow` always updates the item, although it >> should not, resulting in a performance loss as a `TreeTableRow` will always >> call `updateItem(..)`. >> >> It looks like that this was forgotten in >> [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593). >> >> Checking the whole history, it looks like the following was happening: >> 1. There was a check if the item is the same in all cell implementations >> (with `.equals(..)`) >> 2. Check was removed as it caused bugs >> 3. Check was readded, but instead we first check the index (same index) and >> then if we also have the same item (this time with `.isItemChanged(..)`, to >> allow developers to subclass it) >> 4. Forgotten for `TreeTableRow` inbetween this chaos. >> >> Added many tests for the case where an item should be changed and where it >> should not. >> Improved existing tests as well. Using `stageLoader`, as before the tests >> only created a stage when doing the assert. >> >> NOTE: The update logic is now the same as for all other 5 cell >> implementations. Especially `TreeCell` (since it is for a tree structure as >> well). > > Marius Hanl has updated the pull request incrementally with two additional > commits since the last revision: > > - JDK-8325402: remove labeled if > - JDK-8325402: Unit test improvements LGTM, tested in Mac 14.3.1. - Marked as reviewed by kpk (Committer). PR Review: https://git.openjdk.org/jfx/pull/1360#pullrequestreview-1916024806
Integrated: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation
On Tue, 9 Jan 2024 07:31:51 GMT, Karthik P K wrote: > In the `getHitInfo()` method of PrismTextLayout, RTL node orientation > conditions were not considered, hence hit test values such as character index > and insertion index values were incorrect. > > Added checks for RTL orientation of nodes and fixed the issue in > `getHitInfo()` to calculate correct hit test values. > > Added system tests to validate the changes. This pull request has now been integrated. Changeset: 66d96818 Author:Karthik P K URL: https://git.openjdk.org/jfx/commit/66d96818413f8ce5518cc20cff848eacd1a2d56c Stats: 1166 lines in 10 files changed: 1068 ins; 66 del; 32 mod 8319844: Text/TextFlow.hitTest() is incorrect in RTL orientation Co-authored-by: John Hendrikx Reviewed-by: angorya, jhendrikx - PR: https://git.openjdk.org/jfx/pull/1323
Re: RFR: 8270996: javadoc: missing comments in serialized classes [v3]
On Thu, 29 Feb 2024 23:56:06 GMT, Andy Goryachev wrote:
>> Adding `@SuppressWarnings("doclint:missing")` to the lines in Serializable
>> classes that generated javadoc's "missing comments" warning.
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
JFXPanel change ok to me..
-
Marked as reviewed by psadhukhan (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1386#pullrequestreview-1915786485
Re: RFR: 8270996: javadoc: missing comments in serialized classes [v3]
On Fri, 1 Mar 2024 17:11:00 GMT, Kevin Rushforth wrote:
>> FYI: after merging this and #1384 together all I see in the build log are 4
>> native warnings (on macOS):
>>
>>
>> /Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.graphics/src/main/native-prism-es2/macosx/MacOSXWindowSystemInterface.m:93:21:
>> warning: 'lockFocusIfCanDraw' is deprecated: first deprecated in macOS
>> 10.14 - To draw, subclass NSView and implement -drawRect:; AppKit's
>> automatic deferred display mechanism will call -drawRect: as necessary to
>> display the view. [-Wdeprecated-declarations]
>> if ([nsView lockFocusIfCanDraw] == NO) {
>> ^
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:190:1:
>> note: 'lockFocusIfCanDraw' has been explicitly marked deprecated here
>> - (BOOL)lockFocusIfCanDraw API_DEPRECATED("To draw, subclass NSView and
>> implement -drawRect:; AppKit's automatic deferred display mechanism will
>> call -drawRect: as necessary to display the view.", macos(10.0,10.14));
>> ^
>> /Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.graphics/src/main/native-prism-es2/macosx/MacOSXWindowSystemInterface.m:98:25:
>> warning: 'unlockFocus' is deprecated: first deprecated in macOS 10.14 - To
>> draw, subclass NSView and implement -drawRect:; AppKit's automatic deferred
>> display mechanism will call -drawRect: as necessary to display the view.
>> [-Wdeprecated-declarations]
>> [nsView unlockFocus];
>> ^
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:189:1:
>> note: 'unlockFocus' has been explicitly marked deprecated here
>> - (void)unlockFocus API_DEPRECATED("To draw, subclass NSView and implement
>> -drawRect:; AppKit's automatic deferred display mechanism will call
>> -drawRect: as necessary to display the view.", macos(10.0,10.14));
>> ^
>> /Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.graphics/src/main/native-prism-es2/macosx/MacOSXWindowSystemInterface.m:120:24:
>> warning: 'setView:' is deprecated: first deprecated in macOS 10.14 - Use
>> NSOpenGLView to provide OpenGL content in a Cocoa app.
>> [-Wdeprecated-declarations]
>> [nsContext setView : nsView];
>>^
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSOpenGL.h:193:36:
>> note: property 'view' is dec...
>
> That's because we don't enable javac lint warnings by default. I am getting
> ready to file a few JBS issues related to this. Stay tuned.
>
> In the mean time, if you want to see how many warnings we do have, try
> running with `gradle -PLINT=all`. Or rather, you'll see how many modules hit
> the 100 warnings limit. :)
So, that
@SuppressWarnings("serial") // Same-version serialization only
would be addressed as javac lint warning JBS?
Just thinking out aloud, I dont see this @SuppressWarnings("doclint:missing")
in our java.desktop classes? what's the difference there?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1386#discussion_r1512059413
RFR: 8092102: Labeled: truncated property
Adds Labeled.truncated property which indicates when the text is visually truncated (and the ellipsis string is inserted) in order to fit the available width. The new property reacts to changes in the following properties: - ellipsisString - font - text - width - wrapText For some reason, line 859 generates a javadoc "co comment" warning, despite the javadoc comment present at the property declaration in line 832. I don't think it's worth creating a headful test (headless won't work) due to relative simplicity of the code. **Alternative** The desired functionality can be just as easily achieved on an application level, by adding a similar property to a subclass. What is the benefit of adding this functionality to the core? - Commit messages: - 8092102 Labeled: truncated property Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1389&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8092102 Stats: 80 lines in 1 file changed: 59 ins; 17 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v10]
On Tue, 27 Feb 2024 09:30:14 GMT, Johan Vos wrote: >> A listener was added but never removed. >> This patch removes the listener when the menu it links to is cleared. Fix >> for https://bugs.openjdk.org/browse/JDK-8319779 > > Johan Vos has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 11 additional commits since > the last revision: > > - Merge branch 'master' into 8319779-systemmenu > - Add additional test for IOOBE detection. >This test comes from JDK-8323787 > - Revert some of the conditional bindings. >Clear menu construction when an menuitem that is a menu needs to be removed >Add a test for the latter > - Merge remote-tracking branch 'upstream/master' into 8319779-systemmenu > - Cleanup test > - Add shim class so that we can access the references to > com.sun.glass.ui.Menu instances. >Add a test to make sure those references are gone. > - Revert WeakInvalidationListeners and use new listener resource management > approach. > - Fix more memoryleaks due to listeners never being unregistered. > - These changes are related to JBS-8318841 so we want to have that code in >as well. > >Merge branch 'master' into 8319779-systemmenu > - process reviewers comments > - ... and 1 more: https://git.openjdk.org/jfx/compare/a68c0a68...ec7308df modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java line 226: > 224: mb.textProperty().when(active).addListener(valueModel -> > glassMenu.setTitle(parseText(mb))); > 225: mb.disableProperty().when(active).addListener(valueModel -> > glassMenu.setEnabled(!mb.isDisable())); > 226: mb.mnemonicParsingProperty().when(active).addListener(valueModel > -> glassMenu.setTitle(parseText(mb))); Since you are not calling `get()` from the `when` resulting observable (to allow for invalidation), these need to use a `ChangeListener` instead of an `InvalidationListener`, otherwise, while `active` doesn't change, any change in those menuBase properties will trigger just one notification. Alternative, just replace `addListener` with `subscribe` (as it internally uses a `ChangeListener`). @hjohn does this make sense to you? - PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1511687632
Re: RFR: JDK-8285893: Hiding dialog and showing new one causes dialog to be frozen [v5]
> This PR fixes the dialog freeze problem once and for all. > > This one is a bit tricky to understand, here is how it works: > This bug happens on every platform, although the implementation of nested > event loops differs on every platform. > E.g. on Linux we use `gtk_main` and `gtk_main_quit`, on Windows and Mac we > have an own implementation of a nested event loop (while loop), controlled by > a boolean flag. > > Funny enough, the reason why this bug happens is always the same: Timing. > > 1. When we hide a dialog, `_leaveNestedEventLoop` is called. > 2. This will call native code to get out of the nested event loop, e.g. on > Windows we try to break out of the while loop with a boolean flag, on Linux > we call `gtk_main_quit`. > 3. Now, if we immediately open a new dialog, we enter a new nested event loop > via `_enterNestedEventLoop`, as a consequence we do not break out of the > while loop on Windows (the flag is set back again, the while loop is still > running), and we do not return from `gtk_main` on Linux. > 4. And this will result in the Java code never returning and calling > `notifyLeftNestedEventLoop`, which we need to recover the UI. > > So it is actually not trivial to fix this problem, and we can not really do > anything on the Java side. We may can try to wait until one more frame has > run so that things will hopefully be right, but that sounds rather hacky. > > I therefore analyzed, if we even need to return from `_enterNestedEventLoop`. > Turns out, we don't need to. > There is a return value which we actually do not use (it does not have any > meaning to us, other that it is used inside an assert statement). > Without the need of a return value, we also do not need to care when > `_enterNestedEventLoop` is returning - instead we cleanup and call > `notifyLeftNestedEventLoop` in `_leaveNestedEventLoop`, after the native code > was called. > > Lets see if this is the right approach (for all platforms). > Testing appreciated. > # > - [x] Tested on Windows > - [x] Tested on Linux > - [x] Tested on Mac > - [ ] Tested on iOS (although the nested event loop code is the same as for > Mac) (I would appreciate if someone can do this as I have no access to an iOS > device) > - [ ] Adjust copyright > - [ ] Write Systemtest Marius Hanl has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: do not attempt to leave eventloop after leaving another eventloop - Changes: - all: https://git.openjdk.org/jfx/pull/1324/files - new: https://git.openjdk.org/jfx/pull/1324/files/508e2569..0bcda3a2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=03-04 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1324.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1324/head:pull/1324 PR: https://git.openjdk.org/jfx/pull/1324
Re: RFR: JDK-8285893: Hiding dialog and showing new one causes dialog to be frozen [v3]
On Fri, 1 Mar 2024 23:56:19 GMT, Martin Fox wrote: > The top of the call stack when the exception is thrown: Yeah, had the same issue and fixed it locally. Now pushed. Problem as stated above remains though - PR Comment: https://git.openjdk.org/jfx/pull/1324#issuecomment-1977265143
Re: RFR: JDK-8285893: Hiding dialog and showing new one causes dialog to be frozen [v4]
> This PR fixes the dialog freeze problem once and for all. > > This one is a bit tricky to understand, here is how it works: > This bug happens on every platform, although the implementation of nested > event loops differs on every platform. > E.g. on Linux we use `gtk_main` and `gtk_main_quit`, on Windows and Mac we > have an own implementation of a nested event loop (while loop), controlled by > a boolean flag. > > Funny enough, the reason why this bug happens is always the same: Timing. > > 1. When we hide a dialog, `_leaveNestedEventLoop` is called. > 2. This will call native code to get out of the nested event loop, e.g. on > Windows we try to break out of the while loop with a boolean flag, on Linux > we call `gtk_main_quit`. > 3. Now, if we immediately open a new dialog, we enter a new nested event loop > via `_enterNestedEventLoop`, as a consequence we do not break out of the > while loop on Windows (the flag is set back again, the while loop is still > running), and we do not return from `gtk_main` on Linux. > 4. And this will result in the Java code never returning and calling > `notifyLeftNestedEventLoop`, which we need to recover the UI. > > So it is actually not trivial to fix this problem, and we can not really do > anything on the Java side. We may can try to wait until one more frame has > run so that things will hopefully be right, but that sounds rather hacky. > > I therefore analyzed, if we even need to return from `_enterNestedEventLoop`. > Turns out, we don't need to. > There is a return value which we actually do not use (it does not have any > meaning to us, other that it is used inside an assert statement). > Without the need of a return value, we also do not need to care when > `_enterNestedEventLoop` is returning - instead we cleanup and call > `notifyLeftNestedEventLoop` in `_leaveNestedEventLoop`, after the native code > was called. > > Lets see if this is the right approach (for all platforms). > Testing appreciated. > # > - [x] Tested on Windows > - [x] Tested on Linux > - [x] Tested on Mac > - [ ] Tested on iOS (although the nested event loop code is the same as for > Mac) (I would appreciate if someone can do this as I have no access to an iOS > device) > - [ ] Adjust copyright > - [ ] Write Systemtest Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: do not attempt to leave eventloop after it is done - Changes: - all: https://git.openjdk.org/jfx/pull/1324/files - new: https://git.openjdk.org/jfx/pull/1324/files/a4cce087..508e2569 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=02-03 Stats: 20 lines in 1 file changed: 10 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1324.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1324/head:pull/1324 PR: https://git.openjdk.org/jfx/pull/1324
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
On Mon, 4 Mar 2024 17:13:01 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1044:
>>
>>> 1042: private int findFirstRunStart() {
>>> 1043: int start = Integer.MAX_VALUE;
>>> 1044: for (GlyphList r: getRuns()) {
>>
>> the old code had a 0 check for getRuns.length, presumably to avoid the
>> iterator creation.
>> the new code is probably fine, because most of the time we'll have the need
>> for the iterator anyway.
>
> `getRuns` returns an array, it won't create an `Iterator`.
you are right!
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511547919
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Mon, 4 Mar 2024 11:03:26 GMT, Karthik P K wrote: >> @andy-goryachev-oracle The coverage comes from EclEmma, an Eclipse plugin. >> Once installed, there is another way to run tests called `Coverage as...` >> just above `Run as...`. It's very useful to use it on a JUnit test to see >> if there are major branches you missed in testing that may require >> additional cases. > > Thank you @hjohn for providing the test. I have added this. > Since we do not use scene in this test, I think it is difficult to test > HitInfo in RTL mode. However these tests will be very useful in testing the > HitInfo. I think it's possible even in the JUnit test. You can call `setDirection` on `layout` to set mirroring. However, since we removed `isMirrored` check in `getHitInfo`, the code is no longer dependent on whether the text is mirrored or not, so extra tests won't catch anything new. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511512588
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
On Mon, 4 Mar 2024 11:01:09 GMT, Karthik P K wrote: >> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation >> conditions were not considered, hence hit test values such as character >> index and insertion index values were incorrect. >> >> Added checks for RTL orientation of nodes and fixed the issue in >> `getHitInfo()` to calculate correct hit test values. >> >> Added system tests to validate the changes. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Add unit test LGTM - Marked as reviewed by jhendrikx (Committer). PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1914883510
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
On Mon, 4 Mar 2024 16:15:02 GMT, Andy Goryachev wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add unit test
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1044:
>
>> 1042: private int findFirstRunStart() {
>> 1043: int start = Integer.MAX_VALUE;
>> 1044: for (GlyphList r: getRuns()) {
>
> the old code had a 0 check for getRuns.length, presumably to avoid the
> iterator creation.
> the new code is probably fine, because most of the time we'll have the need
> for the iterator anyway.
`getRuns` returns an array, it won't create an `Iterator`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511503440
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
On Mon, 4 Mar 2024 11:01:09 GMT, Karthik P K wrote: >> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation >> conditions were not considered, hence hit test values such as character >> index and insertion index values were incorrect. >> >> Added checks for RTL orientation of nodes and fixed the issue in >> `getHitInfo()` to calculate correct hit test values. >> >> Added system tests to validate the changes. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Add unit test I was gonna review, but I'm a contributor now... still allowed? - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1977079190
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
On Mon, 4 Mar 2024 16:12:12 GMT, Andy Goryachev wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add unit test
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
> line 108:
>
>> 106: if (obj == null)
>> 107: return false;
>> 108: if (getClass() != obj.getClass())
>
> any reason why this code uses getClass() != obj.getClass() ?
>
> perhaps a better choice might be the usual pattern
>
> if(x == this) {
> return true;
> } else if(x instanceof Hit h) {
> return charIndex == h.charIndex && ...
> }
> return false;
There are long discussions about this, and not really worth going into. They
both have their place. Generally, the `getClass` versions are easier to get
right without violating the `equals` contract. `instanceof` can be used, to
allow comparisons with (trivial) subtypes, but then you must make `equals`
final. Anything fancier violates the equals contract.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511498972
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
On Mon, 4 Mar 2024 11:01:09 GMT, Karthik P K wrote:
>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation
>> conditions were not considered, hence hit test values such as character
>> index and insertion index values were incorrect.
>>
>> Added checks for RTL orientation of nodes and fixed the issue in
>> `getHitInfo()` to calculate correct hit test values.
>>
>> Added system tests to validate the changes.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add unit test
a couple of very minor comments, I am ok with pushing this code as is.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
line 108:
> 106: if (obj == null)
> 107: return false;
> 108: if (getClass() != obj.getClass())
any reason why this code uses getClass() != obj.getClass() ?
perhaps a better choice might be the usual pattern
if(x == this) {
return true;
} else if(x instanceof Hit h) {
return charIndex == h.charIndex && ...
}
return false;
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1044:
> 1042: private int findFirstRunStart() {
> 1043: int start = Integer.MAX_VALUE;
> 1044: for (GlyphList r: getRuns()) {
the old code had a 0 check for getRuns.length, presumably to avoid the iterator
creation.
the new code is probably fine, because most of the time we'll have the need for
the iterator anyway.
-
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1914727360
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511416507
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511420549
Integrated: 8327177: macOS: wrong GlobalRef deleted in GlassMenu
On Mon, 4 Mar 2024 11:07:07 GMT, Johan Vos wrote: > Explicitly remove old global ref value from menu->jCallback. > This avoids the possibility of that value (which can be reused for other > refs) to be deleted twice. This pull request has now been integrated. Changeset: 8114559e Author:Johan Vos URL: https://git.openjdk.org/jfx/commit/8114559ed666fe272d238cdf1e531e5d8ec6 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8327177: macOS: wrong GlobalRef deleted in GlassMenu Reviewed-by: kcr - PR: https://git.openjdk.org/jfx/pull/1388
Re: RFR: 8327177: macOS: wrong GlobalRef deleted in GlassMenu
On Mon, 4 Mar 2024 11:07:07 GMT, Johan Vos wrote: > Explicitly remove old global ref value from menu->jCallback. > This avoids the possibility of that value (which can be reused for other > refs) to be deleted twice. Updated title looks good. Thanks. - PR Comment: https://git.openjdk.org/jfx/pull/1388#issuecomment-1976682668
Re: RFR: 8327177: Wrong GlobalRef deleted
On Mon, 4 Mar 2024 11:07:07 GMT, Johan Vos wrote: > Explicitly remove old global ref value from menu->jCallback. > This avoids the possibility of that value (which can be reused for other > refs) to be deleted twice. This is a safe fix that is clearly the right thing to do. I recommend changing the Bug title (and then the PR title to match) to add "macOS" and "GlassMenu" somewhere in the title to make it a little more informative, but I'll leave it up to you. This is a simple enough fix that a single reviewer is sufficient. Once integrated, I recommend backporting this to jfx22u for 22.0.1 (and possibly to earlier releases as you see fit). - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1388#pullrequestreview-1914377783
Re: RFR: 8327179: Update the 3D lighting application
On Sun, 3 Mar 2024 22:29:02 GMT, Nir Lisker wrote: > Update for the 3D lighting test tool as described in the JBS issue. Reviewer: @arapte @jayathirthrao you might also want to take a look? - PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-1976604084
RFR: 8327177: Wrong GlobalRef deleted
Explicitly remove old global ref value from menu->jCallback. This avoids the possibility of that value (which can be reused for other refs) to be deleted twice. - Commit messages: - Explicitly remove old global ref value from menu->jCallback. Changes: https://git.openjdk.org/jfx/pull/1388/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1388&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327177 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1388.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1388/head:pull/1388 PR: https://git.openjdk.org/jfx/pull/1388
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Thu, 29 Feb 2024 17:33:14 GMT, John Hendrikx wrote: >> @hjohn how do you get this coverage diagram? >> >> The BreakIterator is a part of the existing code, we should probably have >> this discussion outside of this PR. I agree, there might be a situation >> when the app wants to select a specific locale for the text component >> instead of relying on the default locale. > > @andy-goryachev-oracle The coverage comes from EclEmma, an Eclipse plugin. > Once installed, there is another way to run tests called `Coverage as...` > just above `Run as...`. It's very useful to use it on a JUnit test to see if > there are major branches you missed in testing that may require additional > cases. Thank you @hjohn for providing the test. I have added this. Since we do not use scene in this test, I think it is difficult to test HitInfo in RTL mode. However these tests will be very useful in testing the HitInfo. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1510982462
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v16]
On Thu, 29 Feb 2024 12:12:01 GMT, John Hendrikx wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1031:
>
>> 1029: if (runs.length != 0) {
>> 1030: textRunStart = findFirstRunStart(runs);
>> 1031: }
>
> Can you rewrite this to be:
>
> int textRunStart = findFirstRunStart();
>
> The `runs` parameter doesn't need to be passed (`findFirstRunStart` can
> access it just as easily), and the length check can be moved inside that
> method as well.
Updated `findFirstRunStart` method. I believe the length test is not really
necessary here because even if the Text node is empty, a run will be created
for it. So did not add length check.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1510984490
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v17]
> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation > conditions were not considered, hence hit test values such as character index > and insertion index values were incorrect. > > Added checks for RTL orientation of nodes and fixed the issue in > `getHitInfo()` to calculate correct hit test values. > > Added system tests to validate the changes. Karthik P K has updated the pull request incrementally with one additional commit since the last revision: Add unit test - Changes: - all: https://git.openjdk.org/jfx/pull/1323/files - new: https://git.openjdk.org/jfx/pull/1323/files/130587c9..7ef09bf5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1323&range=16 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1323&range=15-16 Stats: 228 lines in 5 files changed: 218 ins; 3 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1323.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1323/head:pull/1323 PR: https://git.openjdk.org/jfx/pull/1323
Re: Validation Support
I second what John and Michael say (provide more APIs in OpenJFX that can only realistically be implemented in OpenJFX). I believe the experience from Robert as the creator of ValidatorFX is extremely valuable to this. The key question that might help to see what we need here is: "What would have made it easier to create ValidatorFX (I hear the point of not using final everywhere)?" - Johan On Mon, Mar 4, 2024 at 10:03 AM Michael Strauß wrote: > I would not be in favor of adding any particular data validation > framework to JavaFX. Data validation comes in all kinds of different > shapes and sizes, which makes it a good fit for (opinionated) > third-party libraries. However, I fully agree with John that JavaFX > should provide more APIs that can only realistically be implemented in > FX. I've proposed a "significant interaction" API, which is crucial > for many data validation scenarios: > https://mail.openjdk.org/pipermail/openjfx-dev/2023-March/039327.html > > > On Fri, Mar 1, 2024 at 12:50 PM Dirk Lemmermann > wrote: > > > > Hi everyone, > > > > I updated the validation framework ValidatorFX today in our project to > the latest release and I really like it a lot. It is a small compact API > and works with any observable as opposed to the validation support provided > by ControlsFX. > > > > Using it made me wonder whether it would make sense to bundle it or > something like it directly with JavaFX. Developers often mention missing > validation support as a drawback of using JavaFX. Adding this would take > one item off from the list of arguments against using JavaFX. > > > > Many UI frameworks do have built-in validation support, e.g. Vaadin [0], > Angular, [1], or QT [2] > > > > What do you think? > > > > —Dirk > > > > [0] > https://vaadin.com/docs/latest/binding-data/components-binder-validation > > [1] https://angular.io/guide/form-validation > > [2] https://doc.qt.io/qt-6/qtquick-input-textinput.html > > >
Re: Validation Support
Am 04.03.24 um 10:01 schrieb Michael Strauß: I would not be in favor of adding any particular data validation framework to JavaFX. Data validation comes in all kinds of different shapes and sizes, which makes it a good fit for (opinionated) third-party libraries. I agree with that while at the same time trying to provide different options (opinions) with ValidatorFX. However, I fully agree with John that JavaFX should provide more APIs that can only realistically be implemented in FX. I've proposed a "significant interaction" API, which is crucial for many data validation scenarios: https://mail.openjdk.org/pipermail/openjfx-dev/2023-March/039327.html Shame on me, I totally missed that thread. It contains a lot of insight. Interestingly, ValidatorFX (since 0.5.0) supports these different modes at the moment: * immediate (which you call "Eager" in the thread above) * explicit (which you call "Submit") * immediate-after-submit: This leaves the user undisturbed until they first press the "submit" button. After that, the form changes into immediate mode, only then highlighting everything that has to be corrected. * explicit with immediate clearing: Validation happens on submit, but as soon as the user changes any of the validated properties, validation is reset so that the user has some "breathing space" when correcting their input An implementation of your "second" mode, which will redo validation on "significant user input" is missing at the moment. But I think this should be possible to do with sth. like a .checkedOn(ObservableValue userModifiedProperty) method. That will still leave the burden of defining the userModifiedProperty to the application but that is probably for the best: For a "required" check, getting and losing the focus is not a significant user input, while a field that requires either no input at all or a valid SSN may use the lost focus for that. To that end, I've just created https://github.com/effad/ValidatorFX/issues/37. --Robert
Re: Validation Support
I would not be in favor of adding any particular data validation framework to JavaFX. Data validation comes in all kinds of different shapes and sizes, which makes it a good fit for (opinionated) third-party libraries. However, I fully agree with John that JavaFX should provide more APIs that can only realistically be implemented in FX. I've proposed a "significant interaction" API, which is crucial for many data validation scenarios: https://mail.openjdk.org/pipermail/openjfx-dev/2023-March/039327.html On Fri, Mar 1, 2024 at 12:50 PM Dirk Lemmermann wrote: > > Hi everyone, > > I updated the validation framework ValidatorFX today in our project to the > latest release and I really like it a lot. It is a small compact API and > works with any observable as opposed to the validation support provided by > ControlsFX. > > Using it made me wonder whether it would make sense to bundle it or something > like it directly with JavaFX. Developers often mention missing validation > support as a drawback of using JavaFX. Adding this would take one item off > from the list of arguments against using JavaFX. > > Many UI frameworks do have built-in validation support, e.g. Vaadin [0], > Angular, [1], or QT [2] > > What do you think? > > —Dirk > > [0] https://vaadin.com/docs/latest/binding-data/components-binder-validation > [1] https://angular.io/guide/form-validation > [2] https://doc.qt.io/qt-6/qtquick-input-textinput.html >
