Re: RFR: 8303740 JavaFX - Leak in Logging, Logging remembers last exception
On Tue, 7 Mar 2023 11:59:20 GMT, Florian Kirmaier wrote: > When an exception is logged in JavaFX, then the exception is kept in a > reference. > This way, always the last logged exception is retained. > > This is a memory-leak. > This was done to write unit-tests to ensure certain error-cases are logged. > > A simple fix is, to add a flag, to enable/disable retaining the exception. I think it's cleaner, to just have nothing in production. The flag is a bit ugly - just aesthetically, but I don't think it's a problem. - PR: https://git.openjdk.org/jfx/pull/1053
Re: RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex() [v2]
On Tue, 7 Mar 2023 22:03:12 GMT, Phil Race wrote: >> This fixes an the AIOOBE when finding a line break point in RTL laid out >> glyphs. >> The comment in the bug report explains how we can end up trying to find an >> unachievable break point and yet there's no "stop" on the search when we've >> run out of glyphs so hence the exception. >> >> The fix uses a different method to choose a break point. >> >> A system test has been supplied which will fail on macOS (even with standard >> macOS fonts, not just the Noto Sans Arabic) unless the fix is applied. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8302797 works in monkey tester - wraps correctly, throws no exceptions. - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/1055
Re: RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()
On Tue, 7 Mar 2023 20:18:59 GMT, Andy Goryachev wrote:
>> This fixes an the AIOOBE when finding a line break point in RTL laid out
>> glyphs.
>> The comment in the bug report explains how we can end up trying to find an
>> unachievable break point and yet there's no "stop" on the search when we've
>> run out of glyphs so hence the exception.
>>
>> The fix uses a different method to choose a break point.
>>
>> A system test has been supplied which will fail on macOS (even with standard
>> macOS fonts, not just the Noto Sans Arabic) unless the fix is applied.
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java line
> 288:
>
>> 286: */
>> 287: /* Not need to check for compact as bidi disables the
>> simple case */
>> 288: for (int gi = glyphCount; gi <= 0; gi--) {
>
> should we also correct the comment on line 288?
oops. I think I see the problem.
And I had noticed the typo in the existing comment and can correct that ..
-
PR: https://git.openjdk.org/jfx/pull/1055
Re: RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex() [v2]
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
On Tue, 7 Mar 2023 21:29:20 GMT, Phil Race wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java line
>> 288:
>>
>>> 286: */
>>> 287: /* Not need to check for compact as bidi disables the
>>> simple case */
>>> 288: for (int gi = glyphCount; gi <= 0; gi--) {
>>
>> should we also correct the comment on line 288?
>
> oops. I think I see the problem.
> And I had noticed the typo in the existing comment and can correct that ..
Updated to look at the right segment !
-
PR: https://git.openjdk.org/jfx/pull/1055
Re: RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex() [v2]
> This fixes an the AIOOBE when finding a line break point in RTL laid out > glyphs. > The comment in the bug report explains how we can end up trying to find an > unachievable break point and yet there's no "stop" on the search when we've > run out of glyphs so hence the exception. > > The fix uses a different method to choose a break point. > > A system test has been supplied which will fail on macOS (even with standard > macOS fonts, not just the Noto Sans Arabic) unless the fix is applied. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8302797 - Changes: - all: https://git.openjdk.org/jfx/pull/1055/files - new: https://git.openjdk.org/jfx/pull/1055/files/e9925237..ba0f28dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1055&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1055&range=00-01 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1055.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1055/head:pull/1055 PR: https://git.openjdk.org/jfx/pull/1055
Re: RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()
On Tue, 7 Mar 2023 19:54:15 GMT, Phil Race wrote:
> This fixes an the AIOOBE when finding a line break point in RTL laid out
> glyphs.
> The comment in the bug report explains how we can end up trying to find an
> unachievable break point and yet there's no "stop" on the search when we've
> run out of glyphs so hence the exception.
>
> The fix uses a different method to choose a break point.
>
> A system test has been supplied which will fail on macOS (even with standard
> macOS fonts, not just the Noto Sans Arabic) unless the fix is applied.
Changes requested by angorya (Committer).
The fix introduces another problem, affecting the wrapping of RTL text. To see
the issue, use a TextFlow page in the monkey tester with a RTL text (or simply
place a TextFlow in a SplitPane and reduce the width of the TextFlow.
LTR script seems unaffected.
master branch - wrapping is correct:
the fix branch - wrapping leaves one character per line:
modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java line 288:
> 286: */
> 287: /* Not need to check for compact as bidi disables the simple
> case */
> 288: for (int gi = glyphCount; gi <= 0; gi--) {
should we also correct the comment on line 288?
-
PR: https://git.openjdk.org/jfx/pull/1055
RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()
This fixes an the AIOOBE when finding a line break point in RTL laid out glyphs. The comment in the bug report explains how we can end up trying to find an unachievable break point and yet there's no "stop" on the search when we've run out of glyphs so hence the exception. The fix uses a different method to choose a break point. A system test has been supplied which will fail on macOS (even with standard macOS fonts, not just the Noto Sans Arabic) unless the fix is applied. - Commit messages: - 8302797 Changes: https://git.openjdk.org/jfx/pull/1055/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1055&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8302797 Stats: 221 lines in 2 files changed: 214 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1055.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1055/head:pull/1055 PR: https://git.openjdk.org/jfx/pull/1055
Integrated: 8090123: Items are no longer visible when collection is changed
On Fri, 17 Feb 2023 12:33:31 GMT, Karthik P K wrote: > When a large number of items were scrolled in the `ChoiceBox`, the scrolled > offset was carried forward when the list is replaced with small number of > items. Hence the scroll up arrow was displayed with empty popup. > > Changed code to scroll to top before popup display when content height of > `ChoiceBox` is smaller than the scrolled offset. > > Added system test to validate the fix. This pull request has now been integrated. Changeset: 4178ad72 Author:Karthik P K Committer: Andy Goryachev URL: https://git.openjdk.org/jfx/commit/4178ad72c356ce8bd8fca95ee4dd22987a3b175a Stats: 226 lines in 3 files changed: 226 ins; 0 del; 0 mod 8090123: Items are no longer visible when collection is changed Reviewed-by: angorya, kcr - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8090123: Items are no longer visible when collection is changed [v6]
On Tue, 7 Mar 2023 14:24:20 GMT, Kevin Rushforth wrote: >> Noticed a minor behavior issue, on Mac with multiple monitors. The >> secondary monitor (scale=1) is positioned above the primary retina screen >> (scale=2). When showing a popup in the case of 100 elements, the down arrow >> at the bottom cannot be seen. >> >> In the attached screenshot, one can see the popup clipped at the secondary >> monitor bottom edge, the gray bar is the top of the primary screen: >> >>  >> >> Perhaps the wrong Screen is used in computation, or not accounting for >> getVisualBounds()? >> >> Since this control is designed to work with a few items, I'd suggest >> addressing this issue in a separate PR. Karthik, would you please create a >> new bug? > > @andy-goryachev-oracle can you re-review? @kevinrushforth / @andy-goryachev-oracle could you please sponsor the PR? - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8090123: Items are no longer visible when collection is changed [v9]
On Tue, 7 Mar 2023 14:09:38 GMT, Karthik P K wrote: >> When a large number of items were scrolled in the `ChoiceBox`, the scrolled >> offset was carried forward when the list is replaced with small number of >> items. Hence the scroll up arrow was displayed with empty popup. >> >> Changed code to scroll to top before popup display when content height of >> `ChoiceBox` is smaller than the scrolled offset. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments Marked as reviewed by angorya (Committer). This PR looks good. We can address [JDK-8303060](https://bugs.openjdk.org/browse/JDK-8303060) and [JDK-8303064](https://bugs.openjdk.org/browse/JDK-8303064) separately. - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8303740 JavaFX - Leak in Logging, Logging remembers last exception
On Tue, 7 Mar 2023 11:59:20 GMT, Florian Kirmaier wrote: > When an exception is logged in JavaFX, then the exception is kept in a > reference. > This way, always the last logged exception is retained. > > This is a memory-leak. > This was done to write unit-tests to ensure certain error-cases are logged. > > A simple fix is, to add a flag, to enable/disable retaining the exception. While the fix is probably OK, and would fix the leak, another possibility might be to store the name of the exception class (not the class itself) and check that. - PR: https://git.openjdk.org/jfx/pull/1053
Re: RFR: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0 [v2]
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- > Issue happened during setting a new Scene - updating a new View was done > while the Window reference it kept was null. This caused it to default > scaling values to 1.0f (or 100%) while processing a resize notification, > which for high DPI screens with scaling different than 100% caused UI issues. > > Resolved by splitting `_setView()` native call into two parts - first one > sets the view, then back in JVM side we set a correct Window reference, then > we trigger the notification. It has to be triggered from native side, because > Windows backend of Glass sends back new width/height pulled from WinAPI > `::GetClientRect()` call. > > In process of working on this issue I also found another scenario causing the > same problem - calling `Stage.setScene()` after `Stage.show()`. The patch > fixed that case as well. > > Added a system test which is supposed to check for above issues. I didn't > limit it to run only on platforms with UI scaling enabled because it also > serves as a good sanity check in case there are some other changes to code > that might move/scale the UI unwantingly. I tested this patch both on macOS > Ventura and Windows 11, with `d9c091f` all tests pass while without `d9c091f` > on Windows tests `testShowAndSetScene` and `testSecondSetScene` fail as > expected. Lukasz Kostyra has updated the pull request incrementally with one additional commit since the last revision: Move SetSceneScalingTest to robot test directory - Changes: - all: https://git.openjdk.org/jfx/pull/1054/files - new: https://git.openjdk.org/jfx/pull/1054/files/396d38e5..ad2ccb02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1054&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1054&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1054.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1054/head:pull/1054 PR: https://git.openjdk.org/jfx/pull/1054
Re: RFR: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
On Tue, 7 Mar 2023 14:39:29 GMT, Kevin Rushforth wrote: >> Issue happened during setting a new Scene - updating a new View was done >> while the Window reference it kept was null. This caused it to default >> scaling values to 1.0f (or 100%) while processing a resize notification, >> which for high DPI screens with scaling different than 100% caused UI issues. >> >> Resolved by splitting `_setView()` native call into two parts - first one >> sets the view, then back in JVM side we set a correct Window reference, then >> we trigger the notification. It has to be triggered from native side, >> because Windows backend of Glass sends back new width/height pulled from >> WinAPI `::GetClientRect()` call. >> >> In process of working on this issue I also found another scenario causing >> the same problem - calling `Stage.setScene()` after `Stage.show()`. The >> patch fixed that case as well. >> >> Added a system test which is supposed to check for above issues. I didn't >> limit it to run only on platforms with UI scaling enabled because it also >> serves as a good sanity check in case there are some other changes to code >> that might move/scale the UI unwantingly. I tested this patch both on macOS >> Ventura and Windows 11, with `d9c091f` all tests pass while without >> `d9c091f` on Windows tests `testShowAndSetScene` and `testSecondSetScene` >> fail as expected. > > I'll test and review it. One quick comment: since the newly-added system test > uses Robot, the test class will need to move under the `test.robot` hierarchy > so it will be correctly excluded unless `-PUSE_ROBOT=true`. @kevinrushforth my mistake, should be good now! - PR: https://git.openjdk.org/jfx/pull/1054
Integrated: JDK-8090647: Mnemonics : on windows we should cancel the underscore latch when an app loses focus.
Hi, It's been a while (I've been on vacation, etc) but thank you very much for fixing this issue on such short notice! Thanks again, kind regards, -- Pedro Duque Vieira - https://www.pixelduke.com
Re: RFR: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
On Tue, 7 Mar 2023 14:33:33 GMT, Lukasz Kostyra wrote: > Issue happened during setting a new Scene - updating a new View was done > while the Window reference it kept was null. This caused it to default > scaling values to 1.0f (or 100%) while processing a resize notification, > which for high DPI screens with scaling different than 100% caused UI issues. > > Resolved by splitting `_setView()` native call into two parts - first one > sets the view, then back in JVM side we set a correct Window reference, then > we trigger the notification. It has to be triggered from native side, because > Windows backend of Glass sends back new width/height pulled from WinAPI > `::GetClientRect()` call. > > In process of working on this issue I also found another scenario causing the > same problem - calling `Stage.setScene()` after `Stage.show()`. The patch > fixed that case as well. > > Added a system test which is supposed to check for above issues. I didn't > limit it to run only on platforms with UI scaling enabled because it also > serves as a good sanity check in case there are some other changes to code > that might move/scale the UI unwantingly. I tested this patch both on macOS > Ventura and Windows 11, with `d9c091f` all tests pass while without `d9c091f` > on Windows tests `testShowAndSetScene` and `testSecondSetScene` fail as > expected. I'll test and review it. One quick comment: since the newly-added system test uses Robot, the test class will need to move under the `test.robot` hierarchy so it will be correctly excluded unless `-PUSE_ROBOT=true`. - PR: https://git.openjdk.org/jfx/pull/1054
RFR: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
Issue happened during setting a new Scene - updating a new View was done while the Window reference it kept was null. This caused it to default scaling values to 1.0f (or 100%) while processing a resize notification, which for high DPI screens with scaling different than 100% caused UI issues. Resolved by splitting `_setView()` native call into two parts - first one sets the view, then back in JVM side we set a correct Window reference, then we trigger the notification. It has to be triggered from native side, because Windows backend of Glass sends back new width/height pulled from WinAPI `::GetClientRect()` call. In process of working on this issue I also found another scenario causing the same problem - calling `Stage.setScene()` after `Stage.show()`. The patch fixed that case as well. Added a system test which is supposed to check for above issues. I didn't limit it to run only on platforms with UI scaling enabled because it also serves as a good sanity check in case there are some other changes to code that might move/scale the UI unwantingly. I tested this patch both on macOS Ventura and Windows 11, with `d9c091f` all tests pass while without `d9c091f` on Windows tests `testShowAndSetScene` and `testSecondSetScene` fail as expected. - Commit messages: - SetSceneScalingTest: Properly time test elements - Add test for various setScene-show interactions - Fetch View's size after it has a valid Window ptr Changes: https://git.openjdk.org/jfx/pull/1054/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1054&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299968 Stats: 250 lines in 8 files changed: 247 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1054.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1054/head:pull/1054 PR: https://git.openjdk.org/jfx/pull/1054
Re: Promote addEventHandler/removeEventHandler to EventTarget interface
An incompatible change in a fundamental interface such as EventTarget is not something we would want to do, so any proposed changes will need to retain compatibility (both binary and source). At a minimum, this means that any newly added methods would need to have a default implementation, and we would need to look closely at other aspects of compatibility. -- Kevin On 3/7/2023 1:24 AM, John Hendrikx wrote: Hi Michael, Did you file a JIRA issue for this one? I've recently also been doing some rewriting to use the Event system more. I'm removing custom Scene walking code (and looking at Node.getProperties) to do "event handling", and I've now discovered that it could be done quite a bit nicer by using the provided event mechanism. I've encountered a few things that annoy me about the event system: 1) I'm making use of Presentation classes (Models) that only associate with things in javafx.base (Event, EventTarget, Properties). These presentations need to register event handlers at some point, but this can only be done on Nodes; having this logic close to the Presentation code makes sense, but it would make them dependent on javafx.graphics; if I could do this via EventTarget, the dependency would not be needed. 2) When you fire an event (via Node.fireEvent for example), there is no feedback you can obtain whether the event was consumed or not as the final event is not returned to check `isConsumed` on (internal calls do have this information, but it is not exposed at the last step). 3) Related to 2), I think EventTarget could also have a method to dispatch events (so you can dispatch an event easily to any target, not just via the convenience method Node#fireEvent). See this ticket: https://bugs.openjdk.org/browse/JDK-8303209 Perhaps we can work together on this, or if you're not currently working on it I could take a stab at solving all of these issues. --John On 17/03/2022 21:01, Michael Strauß wrote: I'm working on an application that uses the JavaFX event system extensively, and I'm finding it quite hard to use common code for event handler registrations. The problem is that the `EventTarget` interface contains no addEventHandler/removeEventHandler methods, and as a consequence of that, code that uses `EventTarget` ends up requiring lots of brittle instanceof tests to call these methods on all the different implementations of `EventTarget`. There are three buckets of `EventTarget` implementations: 1) Implementations that declare the following methods: void addEventHandler(EventType, EventHandler); void removeEventHandler(EventType, EventHandler); void addEventFilter(EventType, EventHandler); void removeEventFilter(EventType, EventHandler); --> Node, Scene, Window, Transform, Task, Service 2) Implementations that declare the following methods: void addEventHandler(EventType, EventHandler); void removeEventHandler(EventType, EventHandler); --> MenuItem, TreeItem, TableColumnBase (Note that the EventHandler argument ist parameterized as EventHandler, not EventHandler as in the first set of methods.) 3) Implementations that don't declare any methods to add or remove event handlers: --> Dialog, Tab I think the situation can be improved by promoting the bucket 1 methods to the `EventTarget` interface, so they can be used consistently across all implementations of `EventTarget`. This works seamlessly for bucket 1 and bucket 3 implementations. With bucket 2, there's the problem that, inconsistently, the EventHandler argument is not a lower-bounded wildcard. Unfortunately, a method with an EventHandler parameter cannot implement an interface method that expects EventHandler. However, since the erasure of the method remains the same, changing the method signature would technically be a binary-compatible change. Do you think this is a useful improvement?
Re: RFR: 8090123: Items are no longer visible when collection is changed [v6]
On Thu, 23 Feb 2023 17:48:39 GMT, Andy Goryachev wrote: >> Karthik P K has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address review comments > > Noticed a minor behavior issue, on Mac with multiple monitors. The secondary > monitor (scale=1) is positioned above the primary retina screen (scale=2). > When showing a popup in the case of 100 elements, the down arrow at the > bottom cannot be seen. > > In the attached screenshot, one can see the popup clipped at the secondary > monitor bottom edge, the gray bar is the top of the primary screen: > >  > > Perhaps the wrong Screen is used in computation, or not accounting for > getVisualBounds()? > > Since this control is designed to work with a few items, I'd suggest > addressing this issue in a separate PR. Karthik, would you please create a > new bug? @andy-goryachev-oracle can you re-review? - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8090123: Items are no longer visible when collection is changed [v9]
On Tue, 7 Mar 2023 14:09:38 GMT, Karthik P K wrote: >> When a large number of items were scrolled in the `ChoiceBox`, the scrolled >> offset was carried forward when the list is replaced with small number of >> items. Hence the scroll up arrow was displayed with empty popup. >> >> Changed code to scroll to top before popup display when content height of >> `ChoiceBox` is smaller than the scrolled offset. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8090123: Items are no longer visible when collection is changed [v7]
On Tue, 7 Mar 2023 13:33:42 GMT, Kevin Rushforth wrote: >> Updated code to use `Math.ceil()` > > The `Math.ceil` is in the wrong place. It's the result of the division that > will generate the fractional result that needs it: > > > double screenHeight = > Screen.getPrimary().getVisualBounds().getHeight(); > scrollChoiceBox((int) Math.ceil(screenHeight / rowHeight)); Updated code to use `Math.ceil` for the result of division. Please check. - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8090123: Items are no longer visible when collection is changed [v9]
> When a large number of items were scrolled in the `ChoiceBox`, the scrolled > offset was carried forward when the list is replaced with small number of > items. Hence the scroll up arrow was displayed with empty popup. > > Changed code to scroll to top before popup display when content height of > `ChoiceBox` is smaller than the scrolled offset. > > Added system test to validate the fix. Karthik P K has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1039/files - new: https://git.openjdk.org/jfx/pull/1039/files/18b8d74a..492e7708 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1039&range=08 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1039&range=07-08 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1039.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1039/head:pull/1039 PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8090123: Items are no longer visible when collection is changed [v7]
On Tue, 7 Mar 2023 07:18:07 GMT, Karthik P K wrote: >> tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java >> line 144: >> >>> 142: double rowHeight = >>> ContextMenuContentShim.getContextMenuRowHeight(popup); >>> 143: double screenHeight = >>> Screen.getPrimary().getBounds().getHeight(); >>> 144: scrollChoiceBox((int) (screenHeight / rowHeight)); >> >> This seems to work, but it might be more robust to use `Math.ceil()` before >> casting to int, especially if you make the change to use the visual bounds. > > Updated code to use `Math.ceil()` The `Math.ceil` is in the wrong place. It's the result of the division that will generate the fractional result that needs it: double screenHeight = Screen.getPrimary().getVisualBounds().getHeight(); scrollChoiceBox((int) Math.ceil(screenHeight / rowHeight)); - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8303740 JavaFX - Leak in Logging, Logging remembers last exception
On Tue, 7 Mar 2023 11:59:20 GMT, Florian Kirmaier wrote: > When an exception is logged in JavaFX, then the exception is kept in a > reference. > This way, always the last logged exception is retained. > > This is a memory-leak. > This was done to write unit-tests to ensure certain error-cases are logged. > > A simple fix is, to add a flag, to enable/disable retaining the exception. Storing the class is, in my opinion also wrong. Especially OSGI and other multi-classloader environments suffer from this issue. IMO it's okay to test things this way - but I guess that doesn't matter because I only want to fix this big. - PR: https://git.openjdk.org/jfx/pull/1053
Re: RFR: 8303740 JavaFX - Leak in Logging, Logging remembers last exception
On Tue, 7 Mar 2023 11:59:20 GMT, Florian Kirmaier wrote: > When an exception is logged in JavaFX, then the exception is kept in a > reference. > This way, always the last logged exception is retained. > > This is a memory-leak. > This was done to write unit-tests to ensure certain error-cases are logged. > > A simple fix is, to add a flag, to enable/disable retaining the exception. Well... I'm quite surprised to see code testing for log messages -- its a very bad practice. The side effects should be testable with or without the message (ie, if something throws NPE, the old value is restored or not modified, etc). I doubt the log messages are part of any contract that JavaFX specifies. But anyway... I noticed though that the `check` method only checks the class, yet the entire exception is stored. Storing only the class would also alleviate this issue, although it could still prevent class unloading then. - PR: https://git.openjdk.org/jfx/pull/1053
Re: Promote addEventHandler/removeEventHandler to EventTarget interface
Hi Michael, Did you file a JIRA issue for this one? I've recently also been doing some rewriting to use the Event system more. I'm removing custom Scene walking code (and looking at Node.getProperties) to do "event handling", and I've now discovered that it could be done quite a bit nicer by using the provided event mechanism. I've encountered a few things that annoy me about the event system: 1) I'm making use of Presentation classes (Models) that only associate with things in javafx.base (Event, EventTarget, Properties). These presentations need to register event handlers at some point, but this can only be done on Nodes; having this logic close to the Presentation code makes sense, but it would make them dependent on javafx.graphics; if I could do this via EventTarget, the dependency would not be needed. 2) When you fire an event (via Node.fireEvent for example), there is no feedback you can obtain whether the event was consumed or not as the final event is not returned to check `isConsumed` on (internal calls do have this information, but it is not exposed at the last step). 3) Related to 2), I think EventTarget could also have a method to dispatch events (so you can dispatch an event easily to any target, not just via the convenience method Node#fireEvent). See this ticket: https://bugs.openjdk.org/browse/JDK-8303209 Perhaps we can work together on this, or if you're not currently working on it I could take a stab at solving all of these issues. --John On 07/03/2023 10:24, John Hendrikx wrote: Hi Michael, Did you file a JIRA issue for this one? I've recently also been doing some rewriting to use the Event system more. I'm removing custom Scene walking code (and looking at Node.getProperties) to do "event handling", and I've now discovered that it could be done quite a bit nicer by using the provided event mechanism. I've encountered a few things that annoy me about the event system: 1) I'm making use of Presentation classes (Models) that only associate with things in javafx.base (Event, EventTarget, Properties). These presentations need to register event handlers at some point, but this can only be done on Nodes; having this logic close to the Presentation code makes sense, but it would make them dependent on javafx.graphics; if I could do this via EventTarget, the dependency would not be needed. 2) When you fire an event (via Node.fireEvent for example), there is no feedback you can obtain whether the event was consumed or not as the final event is not returned to check `isConsumed` on (internal calls do have this information, but it is not exposed at the last step). 3) Related to 2), I think EventTarget could also have a method to dispatch events (so you can dispatch an event easily to any target, not just via the convenience method Node#fireEvent). See this ticket: https://bugs.openjdk.org/browse/JDK-8303209 Perhaps we can work together on this, or if you're not currently working on it I could take a stab at solving all of these issues. --John On 17/03/2022 21:01, Michael Strauß wrote: I'm working on an application that uses the JavaFX event system extensively, and I'm finding it quite hard to use common code for event handler registrations. The problem is that the `EventTarget` interface contains no addEventHandler/removeEventHandler methods, and as a consequence of that, code that uses `EventTarget` ends up requiring lots of brittle instanceof tests to call these methods on all the different implementations of `EventTarget`. There are three buckets of `EventTarget` implementations: 1) Implementations that declare the following methods: void addEventHandler(EventType, EventHandler); void removeEventHandler(EventType, EventHandler); void addEventFilter(EventType, EventHandler); void removeEventFilter(EventType, EventHandler); --> Node, Scene, Window, Transform, Task, Service 2) Implementations that declare the following methods: void addEventHandler(EventType, EventHandler); void removeEventHandler(EventType, EventHandler); --> MenuItem, TreeItem, TableColumnBase (Note that the EventHandler argument ist parameterized as EventHandler, not EventHandler as in the first set of methods.) 3) Implementations that don't declare any methods to add or remove event handlers: --> Dialog, Tab I think the situation can be improved by promoting the bucket 1 methods to the `EventTarget` interface, so they can be used consistently across all implementations of `EventTarget`. This works seamlessly for bucket 1 and bucket 3 implementations. With bucket 2, there's the problem that, inconsistently, the EventHandler argument is not a lower-bounded wildcard. Unfortunately, a method with an EventHandler parameter cannot implement an interface method that expects EventHandler. However, since the erasure of the method remains the same, changing the method signature would technically be a binary-compatible chan
RFR: 8303740 JavaFX - Leak in Logging, Logging remembers last exception
When an exception is logged in JavaFX, then the exception is kept in a reference. This way, always the last logged exception is retained. This is a memory-leak. This was done to write unit-tests to ensure certain error-cases are logged. A simple fix is, to add a flag, to enable/disable retaining the exception. - Commit messages: - JDK-8303740 Changes: https://git.openjdk.org/jfx/pull/1053/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1053&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303740 Stats: 33 lines in 3 files changed: 32 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1053.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1053/head:pull/1053 PR: https://git.openjdk.org/jfx/pull/1053
Re: RFR: 8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession
On Tue, 7 Mar 2023 00:00:39 GMT, Kevin Rushforth wrote: >> would it be possible to update the JBS ticket with a valid code example? > > That would be very helpful. Oh, I didn't notice that I did upload the wrong file. I've now uploaded the correct test application! - PR: https://git.openjdk.org/jfx/pull/1052
Re: RFR: 8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession
On Mon, 6 Mar 2023 21:02:43 GMT, Kevin Rushforth wrote: >> Possible fix for VirtualFlow freeze. >> >> I encountered the problem when experimenting with VirtualFlow. >> >> Guess @johanvos should take a look. >> All tests are still green, so with some luck, this doesn't break anything >> but fixes some known and unknown bugs. > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 1718: > >> 1716: // Finally, update the scroll bars >> 1717: updateScrollBarsAndCells(false); >> 1718: lastPosition = getPosition(); > > This is not a formal review -- I'll let Andy and Johan do that -- but can you > explain why this is the right fix? Presumably there was at least some > reasoning behind updating `lastPosition`. Are there any side effects of not > doing that? I've written more about the explanation below. Actually, it's a good question whether there is reasoning behind updating lastPosition. Wouldn't be surprised, if it is just an artifact of some bug-fixing attempt. - PR: https://git.openjdk.org/jfx/pull/1052
Re: Promote addEventHandler/removeEventHandler to EventTarget interface
Hi Michael, Did you file a JIRA issue for this one? I've recently also been doing some rewriting to use the Event system more. I'm removing custom Scene walking code (and looking at Node.getProperties) to do "event handling", and I've now discovered that it could be done quite a bit nicer by using the provided event mechanism. I've encountered a few things that annoy me about the event system: 1) I'm making use of Presentation classes (Models) that only associate with things in javafx.base (Event, EventTarget, Properties). These presentations need to register event handlers at some point, but this can only be done on Nodes; having this logic close to the Presentation code makes sense, but it would make them dependent on javafx.graphics; if I could do this via EventTarget, the dependency would not be needed. 2) When you fire an event (via Node.fireEvent for example), there is no feedback you can obtain whether the event was consumed or not as the final event is not returned to check `isConsumed` on (internal calls do have this information, but it is not exposed at the last step). 3) Related to 2), I think EventTarget could also have a method to dispatch events (so you can dispatch an event easily to any target, not just via the convenience method Node#fireEvent). See this ticket: https://bugs.openjdk.org/browse/JDK-8303209 Perhaps we can work together on this, or if you're not currently working on it I could take a stab at solving all of these issues. --John On 17/03/2022 21:01, Michael Strauß wrote: I'm working on an application that uses the JavaFX event system extensively, and I'm finding it quite hard to use common code for event handler registrations. The problem is that the `EventTarget` interface contains no addEventHandler/removeEventHandler methods, and as a consequence of that, code that uses `EventTarget` ends up requiring lots of brittle instanceof tests to call these methods on all the different implementations of `EventTarget`. There are three buckets of `EventTarget` implementations: 1) Implementations that declare the following methods: void addEventHandler(EventType, EventHandler); void removeEventHandler(EventType, EventHandler); void addEventFilter(EventType, EventHandler); void removeEventFilter(EventType, EventHandler); --> Node, Scene, Window, Transform, Task, Service 2) Implementations that declare the following methods: void addEventHandler(EventType, EventHandler); void removeEventHandler(EventType, EventHandler); --> MenuItem, TreeItem, TableColumnBase (Note that the EventHandler argument ist parameterized as EventHandler, not EventHandler as in the first set of methods.) 3) Implementations that don't declare any methods to add or remove event handlers: --> Dialog, Tab I think the situation can be improved by promoting the bucket 1 methods to the `EventTarget` interface, so they can be used consistently across all implementations of `EventTarget`. This works seamlessly for bucket 1 and bucket 3 implementations. With bucket 2, there's the problem that, inconsistently, the EventHandler argument is not a lower-bounded wildcard. Unfortunately, a method with an EventHandler parameter cannot implement an interface method that expects EventHandler. However, since the erasure of the method remains the same, changing the method signature would technically be a binary-compatible change. Do you think this is a useful improvement?
Re: RFR: 8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession
On Mon, 6 Mar 2023 16:04:02 GMT, Florian Kirmaier wrote:
> Possible fix for VirtualFlow freeze.
>
> I encountered the problem when experimenting with VirtualFlow.
>
> Guess @johanvos should take a look.
> All tests are still green, so with some luck, this doesn't break anything but
> fixes some known and unknown bugs.
Some explanation:
The layout method contains logic, to skip the layouting.
if (width == lastWidth &&
height == lastHeight &&
cellCount == lastCellCount &&
isVertical == lastVertical &&
position == lastPosition &&
! cellSizeChanged)
{
// TODO this happens to work around the problem tested by
//
testCellLayout_LayoutWithoutChangingThingsUsesCellsInSameOrderAsBefore
// but isn't a proper solution. Really what we need to do is,
when
// laying out cells, we need to make sure that if a cell is
pressed
// AND we are doing a full rebuild then we need to make sure we
// use that cell in the same physical location as before so that
// it gets the mouse release event.
return;
}
So, when the position is changed, then the content should be layout again.
But when we reset the oldPosition, to the current position, somewhere else in
the contain - then we interfere with this logic.
This has the effect, that the layout is skipped.
For that reason, it seems wrong to me, to change the "last-something" values
outside of this method.
So I removed the resetting of the lastPosition in the scrollPixels method.
`lastPosition = getPosition();`
I also don't see a reason, why the lastPosition should be set anyways.
But I have to admit, that I don't understand the class well enough to ensure
this is correct based on logic. This fixes the unit test and doesn't seem to
break tests or something obvious in applications according to my tests.
-
PR: https://git.openjdk.org/jfx/pull/1052
