Re: How to set WM_CLASS for GNOME?
Hi Alex,
Do you have the following entry in the .desktop file of your application[1]:
StartupWMClass=com.foo.bar.FooApplication
-Abhinay
[1] https://github.com/gluonhq/scenebuilder/pull/407
From: openjfx-dev on behalf of Alex Orlov
Sent: Thursday, May 26, 2022 10:50 PM
To: openjfx-dev
Subject: How to set WM_CLASS for GNOME?
Hello,
I develop a JavaFX application for Linux. And on my Ubuntu 20 I have a problem.
I do `stage.setTitle("Foo");` and
I have a window with title `Foo`, but in Ubuntu top bar I see the name of the
class `com.foo.bar.FooApplication`.
As I understand I should set correctly WM_CLASS. The only solution I found is
this one
https://stackoverflow.com/a/54467323/5057736
However, I work in JPMS and I can’t access com.sun.glass.ui.Application. Could
anyone say how I can make my
JavaFx application show "Foo" instead of class name in Ubuntu top bar?
--
Best regards, Alex Orlov
Re: RFR: JDK-8269921 Text in Textflow and listeners on bounds can cause endless loop/crash and other performance issues [v2]
On Tue, 6 Jul 2021 15:12:23 GMT, Florian Kirmaier wrote: >> It's "a bit" complicated. >> In some situations, getRuns get's called because listeners on bounds are set. >> This causes TextFlow to layout to compute the runs. >> Afterward, the bounds of the parents get updated. >> This triggers a call to compute bounds - which cascades up to the children. >> When the geometry of the previous Text gets computed in this big stack - it >> throws an nullpointer. >> The Text doesn't have its runs, and calling TextFlow.layout is now a noop >> (it detects repeated calls in the same stack) >> >> In the case it happened - it didn't repair and the application kinda crashed. >> This bug most likely can also be triggered by ScenicView or similar tools, >> which sets listeners to the bounds. >> It also can cause unpredictable performance issues. >> >> Unit test and example stacktrace are in the ticket. >> >> The suggested fix makes sure that recomputing the geometry of the Text, >> doesn't trigger the layout of the TextFlow. >> The Textflow should be layouting by the Parent. >> This might change the behavior in some cases, but as far as I've tested it >> works without issues in TextFlow Heavy applications. >> >> Benefits: >> * Better Tooling Support For ScenicView etc. >> * Fixes complicated but reproducible crashes >> * Might fix some rare crashes, which are hard to reproduce >> * Likely improves performance - might fix some edge cases with >> unpredictable bad performance > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8269921 > Added a copyright header How to proceed to get this PR reviewed? - PR: https://git.openjdk.java.net/jfx/pull/564
Re: RFR: 8256397: MultipleSelectionModel throws IndexOutOfBoundException [v3]
On Mon, 10 Jan 2022 12:29:04 GMT, Florian Kirmaier wrote: >> Fixing IndexOutOfBoundsException in the MultipleSelectionModelBase and added >> a unit-test for it. >> ticket: https://bugs.openjdk.java.net/browse/JDK-8256397 >> run test: `./gradlew --continue -PFULL_TEST=true controls:test --tests >> "*MultipleSelectionModelImplTest*"` > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK_8256397 > Added a small test-factory, to test more cases How to proceed to get this PR reviewed? - PR: https://git.openjdk.java.net/jfx/pull/353
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v5]
On Thu, 27 Jan 2022 14:50:16 GMT, Florian Kirmaier wrote: >> After thinking about this issue for some time, I've now got a solution. >> I know put the scene in the state it is, before is was shown, when the >> dirtyNodes are unset, the whole scene is basically considered dirty. >> This has the drawback of rerendering, whenever a window is "reshown", but it >> restores sanity about memory behaviour, which should be considered more >> important. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8269907 > Removed the sync methods for the scene, because they don't work when peer > is null, and they are not necessary. How to proceed to get this PR reviewed? - PR: https://git.openjdk.java.net/jfx/pull/584
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v8]
On Thu, 14 Oct 2021 17:14:11 GMT, Kevin Rushforth wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8273485 >> Added check for null when calling initScreens > > modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line > 1349: > >> 1347: [self->nativeFullScreenModeWindow >> performSelector:@selector(toggleFullScreen:) withObject:nil]; >> 1348: // wait until the operation is complete >> 1349: [GlassApplication enterFullScreenExitingLoop]; > > You will need to restore the call to `toggleFullScreen` in order to support > exiting from full-screen. This was further discussed in the discussion below. So this is no longer up to date and therefore resolved. - PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v8]
On Mon, 15 Nov 2021 10:05:10 GMT, Florian Kirmaier wrote: >> When using Swing it's possible to generate a Deadlock. >> It's related to the nested eventloop started in enterFullScreenExitingLoop >> - and the RenderLock aquired when using setView in Scene. >> Sample Programm and Threaddump are added to the ticket. >> >> Removing the nested loop fixes the Problem. >> I hope this doesn't have any side effect - so far i don't know of any. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8273485 > Added check for null when calling initScreens How to proceed to get this PR reviewed? - PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8271395: Fixing crash at printing [v2]
On Wed, 8 Sep 2021 07:52:31 GMT, Florian Kirmaier wrote: >> This PR switches the Thread to the QuantumRenderer, in the case the Disposer >> is called from another Thread - the printing Thread. >> I'm open for better solutions on how to fix this Issue. >> Initially i thought there is also a Race Condition in the resource pool, but >> a new one is created for the Printing Thread. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8271395 > QuantumRenderer is no longer public How to proceed to get this PR reviewed? - PR: https://git.openjdk.java.net/jfx/pull/618
Re: RFR: 8277848 Binding and Unbinding to List leads to memory leak [v4]
On Tue, 11 Jan 2022 19:04:56 GMT, Florian Kirmaier wrote: >> Making the initial listener of the ListProperty weak fixes the problem. >> The same is fixed for Set and Map. >> Due to a smart implementation, this is done without any performance drawback. >> (The trick is to have an object, which is both the WeakReference and the >> Changelistener) >> By implying the same trick to the InvalidationListener, this should even >> improve the performance of the collection properties. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8277848 > Added the 3 requests whitespaces How to proceed to get this PR reviewed? - PR: https://git.openjdk.java.net/jfx/pull/689
Re: Updating printers while application is running?
On 08.06.22 21:14, Philip Race wrote: I am referring to windows native code we have in Java 2D. It is completely invisible at the API level of Java 2D. I understand that the public API does not provide this feature. Java 2D itself listens to the events from Windows and updates the array of PrintServices behind PrintServiceLookup accordingly. Sadly it does not expose the events. Well yes, there's got to be a comparison. No one said "I don't know what to do here" .. Sorry! I don't want to tell you, what to do or how to do it. I simply hoped to find an easy solution, which might have been overlooked.
Re: Updating printers while application is running?
On 6/8/22 11:34 AM, Tobias Oelgarte wrote: I don't know if there is an common event available in the Java2D backend to detect added/removed printers directly. So far i could not see anything like that. I am referring to windows native code we have in Java 2D. It is completely invisible at the API level of Java 2D. Every PrintService instance should implement the equals() [1] method to determine uniqueness. This could be used to compare/intersect the observable set of printers hold by javafx.print.Printer#getAllPrinters() against the array of PrintService instances exposed by PrintServiceLookup.getPrintServices(...). That way it should be possible to detect if a printer was removed or added. It could also be used to update the default printer property (on access and/or periodically). Well yes, there's got to be a comparison. No one said "I don't know what to do here" .. -phil. [1] https://docs.oracle.com/en/java/javase/18/docs/api/java.desktop/javax/print/PrintService.html#equals(java.lang.Object) On 07.06.22 21:44, Philip Race wrote: Ahem .. well .. I guess we (I) forgot to get back to that. Since the current implementation on Windows wraps the Java 2D printing code and it registers with windows to be told about changes in printers then the calls being made in the method with that comment would (well, should) provide the updated printers .. The comment was (I think) referring to the need to know when there was actually a change there to avoid perpetual re-creation. If the 2D side is working as expected I'll see if there's something I can do along the lines of a quick check every time that the number of underlying printers is the same and then if not it is worthwhile, coupled with a refresh every 2 minutes so it should never get too stale. -phil. On 6/7/22 11:39 AM, Tobias Oelgarte wrote: Is there some way to update the ObservableSet of Printers once the Application is running? Calling Printer.getAllPrinters() always returns the same OberservableSet and it does not change - ever. [1] We have an JavaFx based application running under Windows 10/11 that is used to print on wireless printers, that are not always available. Having to restart the the application every time a printer goes away or comes available is a serious pain for the users. Is there any kind of workaround? I doubt it will be fixed in the near future, since this comment is from 6 years ago [2]: // This is static. Eventually I want it to be dynamic, but first // it needs to be enhanced to only create new instances where // there really has been a change, which will be rare. [1] https://stackoverflow.com/questions/38470568/javafx-doesnt-detect-changes-of-available-printers [2] https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java#L99
Re: Updating printers while application is running?
I don't know if there is an common event available in the Java2D backend to detect added/removed printers directly. So far i could not see anything like that. Every PrintService instance should implement the equals() [1] method to determine uniqueness. This could be used to compare/intersect the observable set of printers hold by javafx.print.Printer#getAllPrinters() against the array of PrintService instances exposed by PrintServiceLookup.getPrintServices(...). That way it should be possible to detect if a printer was removed or added. It could also be used to update the default printer property (on access and/or periodically). [1] https://docs.oracle.com/en/java/javase/18/docs/api/java.desktop/javax/print/PrintService.html#equals(java.lang.Object) On 07.06.22 21:44, Philip Race wrote: Ahem .. well .. I guess we (I) forgot to get back to that. Since the current implementation on Windows wraps the Java 2D printing code and it registers with windows to be told about changes in printers then the calls being made in the method with that comment would (well, should) provide the updated printers .. The comment was (I think) referring to the need to know when there was actually a change there to avoid perpetual re-creation. If the 2D side is working as expected I'll see if there's something I can do along the lines of a quick check every time that the number of underlying printers is the same and then if not it is worthwhile, coupled with a refresh every 2 minutes so it should never get too stale. -phil. On 6/7/22 11:39 AM, Tobias Oelgarte wrote: Is there some way to update the ObservableSet of Printers once the Application is running? Calling Printer.getAllPrinters() always returns the same OberservableSet and it does not change - ever. [1] We have an JavaFx based application running under Windows 10/11 that is used to print on wireless printers, that are not always available. Having to restart the the application every time a printer goes away or comes available is a serious pain for the users. Is there any kind of workaround? I doubt it will be fixed in the near future, since this comment is from 6 years ago [2]: // This is static. Eventually I want it to be dynamic, but first // it needs to be enhanced to only create new instances where // there really has been a change, which will be rare. [1] https://stackoverflow.com/questions/38470568/javafx-doesnt-detect-changes-of-available-printers [2] https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java#L99
Re: Updating printers while application is running?
I already thought about using PrintServiceLookup. I could use it to show the user the currently available printers, but I can't use it to create a javafx.print.Printer instance without reflective access (JPMS). At the moment I'm stuck with a an unmodifiable OberservableSet that never updates. On 07.06.22 21:50, Thiago Milczarek Sayão wrote: Hi, I use it through the javax.print API: ObservableList printServices = FXCollections.observableArrayList( PrintServiceLookup.lookupPrintServices(null, null)); Maybe it may help for now, but to me it seems it should not be static. Em ter., 7 de jun. de 2022 às 15:39, Tobias Oelgarte escreveu: Is there some way to update the ObservableSet of Printers once the Application is running? Calling Printer.getAllPrinters() always returns the same OberservableSet and it does not change - ever. [1] We have an JavaFx based application running under Windows 10/11 that is used to print on wireless printers, that are not always available. Having to restart the the application every time a printer goes away or comes available is a serious pain for the users. Is there any kind of workaround? I doubt it will be fixed in the near future, since this comment is from 6 years ago [2]: // This is static. Eventually I want it to be dynamic, but first // it needs to be enhanced to only create new instances where // there really has been a change, which will be rare. [1] https://stackoverflow.com/questions/38470568/javafx-doesnt-detect-changes-of-available-printers [2] https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java#L99
Re: Updating printers while application is running?
Hi, I use it through the javax.print API: ObservableList printServices = FXCollections.observableArrayList( PrintServiceLookup.lookupPrintServices(null, null)); Maybe it may help for now, but to me it seems it should not be static. Em ter., 7 de jun. de 2022 às 15:39, Tobias Oelgarte < tobias.oelga...@gmail.com> escreveu: > Is there some way to update the ObservableSet of Printers once the > Application is running? Calling Printer.getAllPrinters() always returns > the same OberservableSet and it does not change - ever. [1] > > We have an JavaFx based application running under Windows 10/11 that is > used to print on wireless printers, that are not always available. > Having to restart the the application every time a printer goes away or > comes available is a serious pain for the users. > > Is there any kind of workaround? > > I doubt it will be fixed in the near future, since this comment is from > 6 years ago [2]: > > // This is static. Eventually I want it to be dynamic, but first > // it needs to be enhanced to only create new instances where > // there really has been a change, which will be rare. > > [1] > > https://stackoverflow.com/questions/38470568/javafx-doesnt-detect-changes-of-available-printers > > [2] > > https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java#L99 >
Re: Updating printers while application is running?
Ahem .. well .. I guess we (I) forgot to get back to that. Since the current implementation on Windows wraps the Java 2D printing code and it registers with windows to be told about changes in printers then the calls being made in the method with that comment would (well, should) provide the updated printers .. The comment was (I think) referring to the need to know when there was actually a change there to avoid perpetual re-creation. If the 2D side is working as expected I'll see if there's something I can do along the lines of a quick check every time that the number of underlying printers is the same and then if not it is worthwhile, coupled with a refresh every 2 minutes so it should never get too stale. -phil. On 6/7/22 11:39 AM, Tobias Oelgarte wrote: Is there some way to update the ObservableSet of Printers once the Application is running? Calling Printer.getAllPrinters() always returns the same OberservableSet and it does not change - ever. [1] We have an JavaFx based application running under Windows 10/11 that is used to print on wireless printers, that are not always available. Having to restart the the application every time a printer goes away or comes available is a serious pain for the users. Is there any kind of workaround? I doubt it will be fixed in the near future, since this comment is from 6 years ago [2]: // This is static. Eventually I want it to be dynamic, but first // it needs to be enhanced to only create new instances where // there really has been a change, which will be rare. [1] https://stackoverflow.com/questions/38470568/javafx-doesnt-detect-changes-of-available-printers [2] https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java#L99
Re: RFR: 8287604: Update MarlinFX to 0.9.4.5 [v2]
On Tue, 31 May 2022 22:25:23 GMT, Kevin Rushforth wrote: >> Hi, >> Time is going short before openjfx 19... >> >> 2 options: >> - keep dpqs for corner cases and keep my coding life simple >> - disable or remove dpqs code in marlinFX for openjfx, so my branches will >> diverge... >> >> Any advice, @kevinrushforth ? > >> keep dpqs for corner cases and keep my coding life simple > > I think this approach is fine. Diverging the code would likely make it less > stable, and you answered the question about the provenance of the code, so > there's no issue there. We should try to get this in before RDP1 of JavaFX 19 > if possible. > > One more thing, as I wrote in my above comment: > >> We should file a new JBS Enhancement issue -- similar to what was done for >> Marlin 0.9.2 via >> [JDK-8204621](https://bugs.openjdk.java.net/browse/JDK-8204621) rather than >> using a narrow bug, since that bug is only part of what's being done. The >> current bug can either be added to the PR, or else that JBS bug >> (JDK-8274066) can be closed as a duplicate of the new Enhancement. > > I took the liberty of filing > [JDK-8287604](https://bugs.openjdk.java.net/browse/JDK-8287604) for this > enhancement. Can you change the title to: > > 8287604: Update MarlinFX to 0.9.4.5 Thx @kevinrushforth I will look at fixing the test with hidpi very soon - PR: https://git.openjdk.java.net/jfx/pull/674
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused comments, clean constructor modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 95: > 93: > 94: /* > 95: * Computes the light's contribution by using the Phong shading model. A > contribution consist of a diffuse component and a Tiny nit: consist => consists - PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8277756: DatePicker listener might not be added when using second constructor
On Tue, 24 May 2022 21:35:15 GMT, Marius Hanl wrote: > The `valueProperty()` and `chronologyProperty()` listener are now added in > the second constructor of `DatePicker` > (`public DatePicker(LocalDate localDate)`) instead of the first one (`public > DatePicker()`). > Therefore, both listener are now added no matter which constructor is used. Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/801
Re: RFR: 8284654: Modal behavior returns to wrong stage [v3]
On Tue, 10 May 2022 12:56:53 GMT, Thiago Milczarek Sayao wrote: >> When there's an APPLICATION_MODAL window, all other windows are disabled and >> re-enabled when the APPLICATION_MODAL window closes. This causes >> `requestToFront()` to be called on every window, and it does not guarantee >> order. >> >> The fix also works for: >> https://bugs.openjdk.java.net/browse/JDK-8269429 >> >> But this one might need another fix: >> https://bugs.openjdk.java.net/browse/JDK-8240640 > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Set the focus on the latest window when re-enabling them" > > This reverts commit b024de586c187f9000f791dab99507a4979cc027. Looks good now. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/784
Re: RFR: 8274066: Polygon filled outside its area when very large coordinates are used [v2]
On Wed, 25 May 2022 07:37:38 GMT, Laurent Bourgès wrote: > keep dpqs for corner cases and keep my coding life simple I think this approach is fine. Diverging the code would likely make it less stable, and you answered the question about the provenance of the code, so there's no issue there. We should try to get this in before RDP1 of JavaFX 19 if possible. One more thing, as I wrote in my above comment: > We should file a new JBS Enhancement issue -- similar to what was done for > Marlin 0.9.2 via > [JDK-8204621](https://bugs.openjdk.java.net/browse/JDK-8204621) rather than > using a narrow bug, since that bug is only part of what's being done. The > current bug can either be added to the PR, or else that JBS bug (JDK-8274066) > can be closed as a duplicate of the new Enhancement. I took the liberty of filing [JDK-8287604](https://bugs.openjdk.java.net/browse/JDK-8287604) for this enhancement. Can you change the title to: 8287604: Update MarlinFX to 0.9.4.5 - PR: https://git.openjdk.java.net/jfx/pull/674
Re: RFR: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView [v2]
On Tue, 24 May 2022 09:51:29 GMT, Jose Pereda wrote: >> This PR fixes an issue with selection of multiple items in TableView and >> TreeTableView controls that gets moved unexpectedly when new items are added >> even way below the selected items. >> >> A couple of tests have been added. They fail without this PR (first >> selected item gets deselected, and table anchor gets moved), and pass with >> this PR (first selected item keeps selected, and table anchor remains at the >> same place). > > Jose Pereda has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into 8284665-tableviewanchor > - Only shift anchor if existing one is affected by the change event, and > tests Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/790
Re: JDK-8091393: Observable collections for ObservableMap views
Then maybe a solution would be around adding new methods like
observableKeySet(). These will need to be default methods, and the
implementation could test if keySet() already returns an ObservableSet, in
which case it returns it, and if not it wraps the Set in an
ObservableSetWrapper or something like that.
On Mon, May 30, 2022 at 11:50 AM John Hendrikx
wrote:
> Sorry, I misunderstood, I missed that the methods weren't already
> defined in ObservableMap, so no existing signature is changed.
>
> --John
>
> On 30/05/2022 09:39, Tom Schindl wrote:
> > Hi,
> >
> > Well the binary compat IMHO is not a problem. If your subtype
> > overwrites the return type of a method the compiler will inserts a
> > bridge method:
> >
> > Take this example
> >
> > package bla;
> >
> > import java.util.ArrayList;
> > import java.util.Collection;
> > import java.util.List;
> >
> > public class Test {
> > public interface IB {
> > public Collection get();
> > }
> >
> > public interface I extends IB {
> > public List get();
> > }
> >
> > public class C implements I {
> > public ArrayList get() {
> > return new ArrayList();
> > }
> > }
> > }
> >
> > if you look at C with javap you'll notice
> >
> > Compiled from "Test.java"
> > public class bla.Test$C implements bla.Test$I {
> > final bla.Test this$0;
> > public bla.Test$C(bla.Test);
> > public java.util.ArrayList get();
> > public java.util.Collection get();
> > public java.util.List get();
> > }
> >
> >
> > The problem is more that if someone directly implemented ObservableMap
> > him/her self that it won't compile anymore. So it is a source
> > incompatible change.
> >
> > Tom
> >
> > Am 30.05.22 um 08:58 schrieb John Hendrikx:
> >> It's not binary compatible, as changing the return type results in a
> >> new method that compiled code won't be able to find.
> >>
> >> See also "change result type (including void)" here:
> >>
> https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods
> >>
> >>
> >> --John
> >>
> >> On 30/05/2022 03:22, Nir Lisker wrote:
> >>> Hi,
> >>>
> >>> Picking up an old issue, JDK-8091393 [1], I went ahead and looked at
> >>> the
> >>> work needed to implement it.
> >>>
> >>> keySet() and entrySet() can both be made to return ObservableSet rather
> >>> easily. values() will probably require an ObservableCollection type.
> >>>
> >>> Before discussing these details, my question is: is it backwards
> >>> compatible
> >>> to require that these methods now return a more refined type? I
> >>> think that
> >>> it will break implementations of ObservableMap out in the wild if it
> >>> overrides these methods in Map. What is the assessment here?
> >>>
> >>> https://bugs.openjdk.java.net/browse/JDK-8091393
>
Re: JDK-8091393: Observable collections for ObservableMap views
Sorry, I misunderstood, I missed that the methods weren't already
defined in ObservableMap, so no existing signature is changed.
--John
On 30/05/2022 09:39, Tom Schindl wrote:
Hi,
Well the binary compat IMHO is not a problem. If your subtype
overwrites the return type of a method the compiler will inserts a
bridge method:
Take this example
package bla;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
public interface IB {
public Collection get();
}
public interface I extends IB {
public List get();
}
public class C implements I {
public ArrayList get() {
return new ArrayList();
}
}
}
if you look at C with javap you'll notice
Compiled from "Test.java"
public class bla.Test$C implements bla.Test$I {
final bla.Test this$0;
public bla.Test$C(bla.Test);
public java.util.ArrayList get();
public java.util.Collection get();
public java.util.List get();
}
The problem is more that if someone directly implemented ObservableMap
him/her self that it won't compile anymore. So it is a source
incompatible change.
Tom
Am 30.05.22 um 08:58 schrieb John Hendrikx:
It's not binary compatible, as changing the return type results in a
new method that compiled code won't be able to find.
See also "change result type (including void)" here:
https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods
--John
On 30/05/2022 03:22, Nir Lisker wrote:
Hi,
Picking up an old issue, JDK-8091393 [1], I went ahead and looked at
the
work needed to implement it.
keySet() and entrySet() can both be made to return ObservableSet rather
easily. values() will probably require an ObservableCollection type.
Before discussing these details, my question is: is it backwards
compatible
to require that these methods now return a more refined type? I
think that
it will break implementations of ObservableMap out in the wild if it
overrides these methods in Map. What is the assessment here?
https://bugs.openjdk.java.net/browse/JDK-8091393
Re: RFR: [WIP] 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v7]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Precalculate size of cells that are likely going to be used in rendering. This avoid resizing cells after their position has been laid out, leading to misalignments. Relaxed some tests that check on the number of invocations on updateItem. We heavily use the accumcell for calculating sizes, and that cell is released every time, leading to a call to updateItem as well (but this call should not do any CPU intensive work) - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/c7d722d3..67b351ac Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=05-06 Stats: 95 lines in 4 files changed: 65 ins; 18 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: JDK-8091393: Observable collections for ObservableMap views
Hi,
Well the binary compat IMHO is not a problem. If your subtype overwrites
the return type of a method the compiler will inserts a bridge method:
Take this example
package bla;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
public interface IB {
public Collection get();
}
public interface I extends IB {
public List get();
}
public class C implements I {
public ArrayList get() {
return new ArrayList();
}
}
}
if you look at C with javap you'll notice
Compiled from "Test.java"
public class bla.Test$C implements bla.Test$I {
final bla.Test this$0;
public bla.Test$C(bla.Test);
public java.util.ArrayList get();
public java.util.Collection get();
public java.util.List get();
}
The problem is more that if someone directly implemented ObservableMap
him/her self that it won't compile anymore. So it is a source
incompatible change.
Tom
Am 30.05.22 um 08:58 schrieb John Hendrikx:
It's not binary compatible, as changing the return type results in a new
method that compiled code won't be able to find.
See also "change result type (including void)" here:
https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods
--John
On 30/05/2022 03:22, Nir Lisker wrote:
Hi,
Picking up an old issue, JDK-8091393 [1], I went ahead and looked at the
work needed to implement it.
keySet() and entrySet() can both be made to return ObservableSet rather
easily. values() will probably require an ObservableCollection type.
Before discussing these details, my question is: is it backwards
compatible
to require that these methods now return a more refined type? I think
that
it will break implementations of ObservableMap out in the wild if it
overrides these methods in Map. What is the assessment here?
https://bugs.openjdk.java.net/browse/JDK-8091393
Re: JDK-8091393: Observable collections for ObservableMap views
It's not binary compatible, as changing the return type results in a new method that compiled code won't be able to find. See also "change result type (including void)" here: https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods --John On 30/05/2022 03:22, Nir Lisker wrote: Hi, Picking up an old issue, JDK-8091393 [1], I went ahead and looked at the work needed to implement it. keySet() and entrySet() can both be made to return ObservableSet rather easily. values() will probably require an ObservableCollection type. Before discussing these details, my question is: is it backwards compatible to require that these methods now return a more refined type? I think that it will break implementations of ObservableMap out in the wild if it overrides these methods in Map. What is the assessment here? https://bugs.openjdk.java.net/browse/JDK-8091393
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v9]
On Fri, 27 May 2022 16:47:04 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Adingef else block to unregisterListener LGTM - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v14]
On Fri, 27 May 2022 23:31:42 GMT, Kevin Rushforth wrote:
> I reviewed the public API changes, and this looks like a great addition to
> JavaFX bindings. I think there might be time to get this into JavaFX 19,
> presuming that there are no issues with the testing or implementation, so
> let's proceed down that path.
>
> I left one comment on the API docs as well as pointed out the public methods
> that will need an `@since 19` javadoc tag.
>
> Once that is updated you can propagate the javadoc changes to the CSR
> (including the `@since` tags) and move it to "Proposed". I'll formally review
> it later, once the code review is closer to being done.
Thanks, I've made the changes and updated the CSR with the latest docs. It's
now proposed.
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 205:
>
>> 203: * resulting value is {@code null}. If the mapping resulted in
>> {@code null}, then the
>> 204: * resulting value is also {@code null}.
>> 205: *
>
> It might be worth borrowing some language from `Optional::flatMap`. Maybe
> something like this?
>
>
> This method is similar to {@link #map(Function)}, but the mapping function is
> one whose result is already an ObservableValue, and if invoked, flatMap does
> not wrap it within an additional ObservableValue.
Added this and put code tags where necessary.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v15]
> This is an implementation of the proposal in
> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
> (@nlisker) have been working on. It's a complete implementation including
> good test coverage.
>
> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
> API footprint. Compared to the PoC this is lacking public API for
> subscriptions, and does not include `orElseGet` or the `conditionOn`
> conditional mapping.
>
> **Flexible Mappings**
> Map the contents of a property any way you like with `map`, or map nested
> properties with `flatMap`.
>
> **Lazy**
> The bindings created are lazy, which means they are always _invalid_ when not
> themselves observed. This allows for easier garbage collection (once the last
> observer is removed, a chain of bindings will stop observing their parents)
> and less listener management when dealing with nested properties.
> Furthermore, this allows inclusion of such bindings in classes such as `Node`
> without listeners being created when the binding itself is not used (this
> would allow for the inclusion of a `treeShowingProperty` in `Node` without
> creating excessive listeners, see this fix I did in an earlier PR:
> https://github.com/openjdk/jfx/pull/185)
>
> **Null Safe**
> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
> when the value they would be mapping is `null`. This makes mapping nested
> properties with `flatMap` trivial as the `null` case does not need to be
> taken into account in a chain like this:
> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
> Instead a default can be provided with `orElse`.
>
> Some examples:
>
> void mapProperty() {
> // Standard JavaFX:
> label.textProperty().bind(Bindings.createStringBinding(() ->
> text.getValueSafe().toUpperCase(), text));
>
> // Fluent: much more compact, no need to handle null
> label.textProperty().bind(text.map(String::toUpperCase));
> }
>
> void calculateCharactersLeft() {
> // Standard JavaFX:
>
> label.textProperty().bind(text.length().negate().add(100).asString().concat("
> characters left"));
>
> // Fluent: slightly more compact and more clear (no negate needed)
> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + "
> characters left"));
> }
>
> void mapNestedValue() {
> // Standard JavaFX:
> label.textProperty().bind(Bindings.createStringBinding(
> () -> employee.get() == null ? ""
> : employee.get().getCompany() == null ? ""
> : employee.get().getCompany().getName(),
> employee
> ));
>
> // Standard JavaFX + Optional:
> label.textProperty().bind(Bindings.createStringBinding(
> () -> Optinal.ofNullable(employee.get())
> .map(Employee::getCompany)
> .map(Company::getName)
> .orElse(""),
> employee
> ));
>
> // Fluent: no need to handle nulls everywhere
> label.textProperty().bind(
> employee.map(Employee::getCompany)
> .map(Company::getName)
> .orElse("")
> );
> }
>
> void mapNestedProperty() {
> // Standard JavaFX:
> label.textProperty().bind(
> Bindings.when(Bindings.selectBoolean(label.sceneProperty(), "window",
> "showing"))
> .then("Visible")
> .otherwise("Not Visible")
> );
>
> // Fluent: type safe
> label.textProperty().bind(label.sceneProperty()
> .flatMap(Scene::windowProperty)
> .flatMap(Window::showingProperty)
> .orElse(false)
> .map(showing -> showing ? "Visible" : "Not Visible")
> );
> }
>
> Note that this is based on ideas in ReactFX and my own experiments in
> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
> that this is much better directly integrated into JavaFX, and I'm hoping this
> proof of concept will be able to move such an effort forward.
John Hendrikx has updated the pull request incrementally with two additional
commits since the last revision:
- Expand flatMap javadoc with additional wording from Optional#flatMap
- Add since tags to all new API
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/675/files
- new: https://git.openjdk.java.net/jfx/pull/675/files/e2703e6b..c3523903
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=675=14
- incr: https://webrevs.openjdk.java.net/?repo=jfx=675=13-14
Stats: 9 lines in 2 files changed: 9 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jfx/pull/675.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/675/head:pull/675
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v14]
On Tue, 22 Mar 2022 07:46:40 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Standard JavaFX + Optional:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> Optinal.ofNullable(employee.get())
>> .map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse(""),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix wording
I reviewed the public API changes, and this looks like a great addition to
JavaFX bindings. I think there might be time to get this into JavaFX 19,
presuming that there are no issues with the testing or implementation, so let's
proceed down that path.
I left one comment on the API docs as well as pointed out the public methods
that will need an `@since 19` javadoc tag.
Once that is updated you can propagate the javadoc changes to the CSR
(including the `@since` tags) and move it to "Proposed". I'll formally review
it later, once the code review is closer to being done.
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v9]
On Fri, 27 May 2022 16:47:04 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Adingef else block to unregisterListener Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v9]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adingef else block to unregisterListener - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/79f30067..c609ad9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=07-08 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
On Fri, 27 May 2022 15:42:09 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Change for unregisterDomWindow function and code cleanup > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 57: > >> 55: } >> 56: >> 57: if (it->second && it->second->use_count() > 1) > > Shouldn't this be `else if`? The previous block calls `erase(it)`, so it > isn't valid to dereference `it` after that call. Applied and tested - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
On Fri, 27 May 2022 12:13:43 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Change for unregisterDomWindow function and code cleanup The changes look good. While doing my final review I noticed one more thing that I think should be changed. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 57: > 55: } > 56: > 57: if (it->second && it->second->use_count() > 1) Shouldn't this be `else if`? The previous block calls `erase(it)`, so it isn't valid to dereference `it` after that call. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
On Thu, 26 May 2022 17:00:46 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding space for map include > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 106: > >> 104: else >> 105: isReferringToOtherListener = true; >> 106: } > > I think I now understand what this is trying to do, but it doesn't looks like > it's implemented correctly nor is it optimal. > > It seems that the `isReferringToOtherListener` flag is intended to be true > iff there is any `JavaEventListener` with a ref count > 1 associated with the > Window being unregistered. Ignoring any possible bugs in how it is > implemented, there are two problems with this approach. First, you want to > remove entries associated with a particular listener if _that_ listener isn't > used by another window. So a global "is any listener referring to another > window" isn't what you want. Second, since this is multimap, it would seem > better to remove individual `(key,value)` pairs associated with the specific > window being removed rather than calling erase only for those listeners that > aren't being shared at all. If this is feasible, then you wouldn't have to > even care if that listener is used by a node in another window. I'm not > familiar enough with C++ multimap calls to know how easy this would be, but > presumably, it shouldn't be too hard. > > Not directly related to this, it might be cleaner to move this logic to > `unregisterDOMWindow` instead of having a separate `resetDOMWindow` method, > since this is really part of the same conceptual operation. I have used the suggested method , and tested it works expected. Thanks - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Change for unregisterDomWindow function and code cleanup - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/d6fd438b..79f30067 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=06-07 Stats: 74 lines in 5 files changed: 11 ins; 23 del; 40 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
On Thu, 26 May 2022 07:26:38 GMT, Jay Bhaskar wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue
>> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
>> are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
>> global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the
>> JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with
>> DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being
>> forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered
>> all the event listeners based on the ref counts attached with WebView
>> DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding space for map include
Comments inline.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 106:
> 104: else
> 105: isReferringToOtherListener = true;
> 106: }
I think I now understand what this is trying to do, but it doesn't looks like
it's implemented correctly nor is it optimal.
It seems that the `isReferringToOtherListener` flag is intended to be true iff
there is any `JavaEventListener` with a ref count > 1 associated with the
Window being unregistered. Ignoring any possible bugs in how it is implemented,
there are two problems with this approach. First, you want to remove entries
associated with a particular listener if _that_ listener isn't used by another
window. So a global "is any listener referring to another window" isn't what
you want. Second, since this is multimap, it would seem better to remove
individual `(key,value)` pairs associated with the specific window being
removed rather than calling erase only for those listeners that aren't being
shared at all. If this is feasible, then you wouldn't have to even care if that
listener is used by a node in another window. I'm not familiar enough with C++
multimap calls to know how easy this would be, but presumably, it shouldn't be
too hard.
Not directly related to this, it might be cleaner to move this logic to
`unregisterDOMWindow` instead of having a separate `resetDOMWindow` method,
since this is really part of the same conceptual operation.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 607:
> 605: */
> 606: @Test
> 607: public void TestStrongRefNewContentLoad() throws Exception {
Minor: method names should start with a lower-case letter.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 730:
> 728: });
> 729:
> 730: // Verify that the events are delivered to the listeners (0 and
> 2 are same)
Minor: all three listeners are the same, not just 0 and 2.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 959:
> 957: // Verify that the event is delivered to the listener
> 958: assertEquals("Click count", 1, listeners.get(0).getClickCount());
> 959: assertEquals("Click count", 1, listeners.get(0).getClickCount());
The second check should be `listeners.get(1)`
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adding space for map include - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/4c8e4a5b..d6fd438b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v6]
On Thu, 26 May 2022 07:08:59 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Adding new review change modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 34: > 32: #include "config.h" > 33: > 34: #include nit: space - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v6]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adding new review change - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/2a09a847..4c8e4a5b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=04-05 Stats: 34 lines in 5 files changed: 18 ins; 9 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
On Wed, 25 May 2022 14:57:13 GMT, Ambarish Rapte wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review comments applied > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 109: > >> 107: isReferringToOtherListener = false; >> 108: else >> 109: isReferringToOtherListener = true; > > I have not looked very clearly, but here is a question: Should the loop break > when `if` condition passes ? No, it should not , if condition pass means there is ref count of 1 for the particular ref object. but we have to look for all other objects too , to check whether a ref counted object might be referred by other Dom Window. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
On Wed, 25 May 2022 12:45:59 GMT, Ambarish Rapte wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments applied
>
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 34:
>
>> 32: EventListenerManager& EventListenerManager::get_instance()
>> 33: {
>> 34: static NeverDestroyed sharedManager;
>
> This does behave well as a singleton class. But I think the instance should
> be class member variable or a pointer.
Most of the singleton class implementation is as above in Webkit.
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Fri, 13 May 2022 05:34:08 GMT, Tomator wrote:
> When I used BlueJ, I found a problem with Chinese display.
> /javafx.graphics/com/sun/javafx/font/CompositeGlyphMapper.java#getGlyphCode
> may return a negative number when no font library is specified in Linux,and
> this could cause java.lang.ArrayIndexOutOfBoundsException error.So
> javafx.graphics/com/sun/prism/impl/GlyphCache.java#getCachedGlyph shou check
> the glyphCode.
> The crash demo like this:
> `import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.Menu;
> import javafx.scene.control.MenuBar;
> import javafx.scene.control.MenuItem;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
>
> public class MenuExample extends Application {
> public static void main(String[] args) {
> launch(args);
> }
>
> @Override
> public void start(Stage primaryStage) throws Exception {
> BorderPane root = new BorderPane();
> Scene scene = new Scene(root,200,300);
> MenuBar menubar = new MenuBar();
> Menu FileMenu = new Menu("文本");
> MenuItem filemenu1=new MenuItem("新建");
> MenuItem filemenu2=new MenuItem("Save");
> MenuItem filemenu3=new MenuItem("Exit");
> Menu EditMenu=new Menu("Edit");
> MenuItem EditMenu1=new MenuItem("Cut");
> MenuItem EditMenu2=new MenuItem("Copy");
> MenuItem EditMenu3=new MenuItem("Paste");
> EditMenu.getItems().addAll(EditMenu1,EditMenu2,EditMenu3);
> root.setTop(menubar);
> FileMenu.getItems().addAll(filemenu1,filemenu2,filemenu3);
> menubar.getMenus().addAll(FileMenu,EditMenu);
> primaryStage.setScene(scene);
> primaryStage.setTitle("TEST");
> primaryStage.show();
>
> }
> } `
>
> the error:
> `java.lang.ArrayIndexOutOfBoundsException: Index -25 out of bounds for length
> 32
> at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
> at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
> at
> com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
> at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
> at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
> at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
> at
> com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
> at
> com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
> `
I can repeated this issue on uos when the linux doesn't have
/usr/share/fonts/wps-office/FZSongS_20100603.TTF file ,openjfx version
11,method getGlyphCode will return a negative number
-
PR: https://git.openjdk.java.net/jfx/pull/795
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
On Tue, 24 May 2022 15:20:01 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adding JGObject in plave of raw jni object
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
> line 765:
>
>> 763: assertEquals("Click count", 6,
>> listeners.get(1).get().getClickCount() +
>> listeners.get(0).get().getClickCount());
>> 764:
>> 765: // add events listeners again
>
> that should be "remove"
I think you may have misunderstood my comment. I didn't mean to suggest that
you should remove the blocks of code that you removed in the most recent update
-- it was performing a quite useful check. Rather I meant that since you were
removing the listeners, the comment `add listeners again` was wrong and should
say `remove listeners again`.
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
On Sun, 22 May 2022 14:06:41 GMT, Jay Bhaskar wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue
>> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
>> are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
>> global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the
>> JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with
>> DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being
>> forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered
>> all the event listeners based on the ref counts attached with WebView
>> DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding JGObject in plave of raw jni object
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 764:
> 762: Thread.sleep(100);
> 763: assertEquals("Click count", 6,
> listeners.get(1).get().getClickCount() +
> listeners.get(0).get().getClickCount());
> 764:
The above code should be restored.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 789:
> 787: Thread.sleep(100);
> 788: assertEquals("Click count", 6,
> listeners.get(1).get().getClickCount() +
> listeners.get(0).get().getClickCount());
> 789:
I think the above code should be restored.
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
On Wed, 25 May 2022 10:10:08 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments applied I left a couple comments on one of your test changes. I think the only remaining question (aside from the few minor comments that Ambarish left) is around the logic in `EventListenerManager::resetDOMWindow`. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: Jpopupmenu Bug/Glitch When Showing Submenu
I opened a bug report but they closed it by saying that they can't
reproduce.
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8286713
They test on Windows 10, can this be the cause why they are not able to
reproduce?
Here a gist where the code is better formatted.
https://gist.github.com/sblantipodi/4ed1d3acc04fd6d4f87c00c7e91eb343
This is the glitch I have, better described on a video:
https://www.youtube.com/shorts/IYq_yHemsgA
Using Adopt Open JDK 17, JavaFX 18.0.1 and Windows 11.
If I remove the submenu, problem is solved but I need the submenu.
Any idea?
Thanks
Davide
Il 09/05/2022 18:14, Davide Perini ha scritto:
I add a video that shows the issue.
https://www.youtube.com/shorts/IYq_yHemsgA
as you can see, there is no issue until I put the mouse pointer over
the JMenu (submenu),
once I put the mouse on the JMenu the entire JPopupMenu shows the glitch.
Thanks
Davide
Il 09/05/2022 18:04, Davide Perini ha scritto:
I can't load images unfortunantly but this is the minimum code to
reproduce the issue.
It seems that there is no way to add JMenu to JPopupMenu without
having this glitch.
Is this a problem in JavaFX?
I see non obvious errors in my code.
Thanks
Davide
package org.dpsoftware; import org.dpsoftware.config.Constants;
import org.dpsoftware.utilities.CommonUtility; import javax.swing.*;
import java.awt.*; import java.awt.event.*; import
java.io.IOException; import java.io.PrintWriter; import
java.io.Serial; import java.io.StringWriter; import java.net.URI;
import java.net.URISyntaxException; public class TrayIconMainClass {
private static final StringTRAY_ICON_SKELETON_PROJECT_URL
="https://github.com/Sylvain-Bugat/tray-icon-skeleton;; /** Load the
tray icon image to display in the tray bar*/ private static
ImageTRAY_ICON_IMAGE = Toolkit.getDefaultToolkit().getImage(
TrayIconMainClass.class.getClassLoader().getResource("tray_play.png"
) ); //$NON-NLS-1$ static JMenuaspectRatioSubMenu; /** * Main program
launched in the jar file * * @param args * @throws IOException */
private void initializeImages() {
// load an image }
public static void main(final String args[] )throws IOException {
aspectRatioSubMenu =new JMenu("dadsdsa"); JMenuItem menuItam
=new JMenuItem("dada"); aspectRatioSubMenu.add(menuItam); //Test if
the system support the system tray if( SystemTray.isSupported() ) {
//Try to use the system Look try {
UIManager.setLookAndFeel(
UIManager.getCrossPlatformLookAndFeelClassName() ); }
catch(final ClassNotFoundException exception ) {
//If System Look is not supported, stay with the
default one }
catch(final InstantiationException exception ) {
//If System Look is not supported, stay with the
default one }
catch(final IllegalAccessException exception ) {
//If System Look is not supported, stay with the
default one }
catch(final UnsupportedLookAndFeelException exception ) {
//If System Look is not supported, stay with the
default one }
//Add the icon to the system tray final TrayIcon trayIcon
=new TrayIcon(TRAY_ICON_IMAGE, "Tray icon skeleton" );
trayIcon.setImageAutoSize(true ); // Get the system default browser
to open execution details final Desktop desktop =
Desktop.getDesktop(); //Action listener to get click on top menu
items final ActionListener menuListener =new ActionListener() {
@SuppressWarnings("synthetic-access")
public void actionPerformed(final ActionEvent e) {
if( JMenuItem.class.isInstance( e.getSource() ) ){
JMenuItem jMenuItem = (JMenuItem) e.getSource();
JOptionPane.showMessageDialog(null, "It works, you clicked on:" +
System.lineSeparator() + jMenuItem.getText(), "Your skill is
great!!", JOptionPane.INFORMATION_MESSAGE ); //$NON-NLS-1$ }
}
}; //About menu listener final ActionListener aboutListener
=new ActionListener() {
@SuppressWarnings("synthetic-access")
public void actionPerformed(final ActionEvent e) {
//Open an URL using the system default browser try {
final URI executionURI =new
URI(TRAY_ICON_SKELETON_PROJECT_URL ); desktop.browse( executionURI ); }
catch(final URISyntaxException exception ) {
final StringWriter stringWriter =new
StringWriter(); exception.printStackTrace(new PrintWriter(
stringWriter ) ); JOptionPane.showMessageDialog(null,
exception.getMessage() + System.lineSeparator() +
stringWriter.toString(), "Tray icon skeleton redirection error",
JOptionPane.ERROR_MESSAGE ); //$NON-NLS-1$ }
catch(final IOException exception ) {
final StringWriter stringWriter =new
StringWriter(); exception.printStackTrace(new PrintWriter(
stringWriter ) ); JOptionPane.showMessageDialog(null,
exception.getMessage() + System.lineSeparator() +
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
On Wed, 25 May 2022 10:10:08 GMT, Jay Bhaskar wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue
>> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
>> are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
>> global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the
>> JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with
>> DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being
>> forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered
>> all the event listeners based on the ref counts attached with WebView
>> DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comments applied
My review is not complete. I shall continue later. Providing few comments for
now.
Please take a look.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 34:
> 32: EventListenerManager& EventListenerManager::get_instance()
> 33: {
> 34: static NeverDestroyed sharedManager;
This does behave well as a singleton class. But I think the instance should be
class member variable or a pointer.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 38:
> 36: }
> 37:
> 38: EventListenerManager::EventListenerManager() { }
Empty constructor. Can be moved to .h file.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 52:
> 50: it = listener_lists.find(ptr);
> 51: JNIEnv *env = nullptr;
> 52: env = JavaScriptCore_GetJavaEnv();
env seems unused. should be removed.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 109:
> 107: isReferringToOtherListener = false;
> 108: else
> 109: isReferringToOtherListener = true;
I have not looked very clearly, but here is a question: Should the loop break
when `if` condition passes ?
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 115:
> 113: if (window == win_it->second)
> 114:windowHasEvent.erase(win_it->first);
> 115: }
I would change the code to be something like:
if (!isReferringToOtherListener) {
for (win_it = windowHasEvent.begin(); win_it != windowHasEvent.end();
win_it++) {
if (window == win_it->second)
windowHasEvent.erase(win_it->first);
}
}
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
line 36:
> 34: #include
> 35: #include
> 36: #include
minor: Add space after `#include`, on line 34, 36.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
line 79:
> 77:
> 78: private:
> 79: EventListenerManager();
I would recommend to move it in previous private block. (line 65, 66)
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/JavaEventListener.cpp
line 50:
> 48: ? static_cast JavaEventListener*>()
> 49: : nullptr;
> 50: return jother && (this == jother);
`this` will never be null so the statement can be changed to: `return this ==
jother;`
modules/javafx.web/src/main/native/Source/WebCore/dom/Node.cpp line 2151:
> 2149: #if PLATFORM(JAVA)
> 2150:
> EventListenerManager::get_instance().registerDOMWindow(targetNode->document().domWindow(),
> 2151: static_cast
> (().get()));
minor: Alignment correction, should move few spaces to left.
-
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Review comments applied - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/43f8e549..2a09a847 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=03-04 Stats: 80 lines in 3 files changed: 6 ins; 59 del; 15 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8274066: Polygon filled outside its area when very large coordinates are used [v2]
On Mon, 10 Jan 2022 00:04:00 GMT, Laurent Bourgès wrote: >> Changelog for this MarlinFX 0.9.4.5 release: >> >> The Marlin-renderer 0.9.4.5 release provides bug fixes on Marlin's path >> clipper: >> - improved Stroker to handle huge coordinates, up to 1E15 >> - improved PathClipFilter (filler) to handle huge coordinates, up to 1E15 >> >> >> This is the Marlin-renderer 0.9.4.3 release providing few bug / enhancement >> fixes in the MarlinRenderingEngine: >> - Update DPQS to latest OpenJDK 14 patch >> - Improve cubic curve offset computation >> >> >> The Marlin-renderer 0.9.4.2 release provides a single long-standing bug fix >> in the MarlinRenderingEngine: >> - JDK-8230728, https://bugs.openjdk.java.net/browse/JDK-8230728. >> >> >> Marlin-renderer 0.9.4.1 provides only a single bug fix in the path clipper, >> reported first against JavaFX 11: >> - JDK-8226789, https://bugs.openjdk.java.net/browse/JDK-8226789. >> >> >> This is the Marlin-renderer 0.9.4 release providing an updated Dual Pivot >> Quick Sort (19.05) as its internal sorter faster than the Marlin's optimized >> MergeSort (x-position + edge indices) for arrays larger than 256. >> >> Special Thanks to Vladimir Yaroslavskiy that provided me up-to-date DPQS >> 19.05 with many variants, improving almost-sorted datasets. We are >> collaborating to provide a complete Sort framework (15 algorithms, many >> various datasets, JMH benchmarks) publicly on github: >> see https://github.com/bourgesl/nearly-optimal-mergesort-code > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > added test for huge polygon coords Hi, Time is going short before openjfx 19... 2 options: - keep dpqs for corner cases and keep my coding life simple - disable or remove dpqs code in marlinFX for openjfx, so my branches will diverge... Any advice, @kevinrushforth ? - PR: https://git.openjdk.java.net/jfx/pull/674
Re: RFR: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
On Tue, 24 May 2022 21:25:23 GMT, Marius Hanl wrote:
> This PR fixes a problem, where the layout is broken when a `(Tree)TableRow`
> has padding.
> As also mentioned in the ticket, the `layoutChildren` method in
> `TableRowSkinBase` is implemented wrong.
>
> The `layoutChildren` method is responsible for layouting all the
> `(Tree)TableCells`.
> When the row has padding, it is subtracted on every `(Tree)TableCell` - which
> is wrong.
>
> Instead the `x` and `y` from `layoutChildren` should be used. (E.g. if `x` is
> 10, then only the first cell should start at 10 and the other cells follow as
> usual)
> Also the `compute...` methods needs to add the padding as well.
>
> **Example:**
> _Row padding left right 0:_
> [Cell1][Cell2][Cell3]
> _Row padding left right 10:_
> [10][Cell1][Cell2][Cell3][10]
> _Same for top bottom._
>
> When a `fixedCellSize` is set, the padding is currently ignored (also after
> this PR).
> Therefore, `y` in the `layoutChildren` method is set to 0 for `fixedCellSize`.
>
> This may can be discussed in the mailing list (Could be a follow up). To
> support padding also when a `fixedCellSize` is set, the `compute...` methods
> needs to also add the padding when a `fixedCellSize` is set (in the `if`
> clauses) and the `VirtualFlow` needs to add the padding to every row instead
> of using the `fixedCellSize` directly.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java
line 255:
> 253: private void removedColumnsShouldRemoveCorrespondingCellsInRowImpl()
> {
> 254: // Remove the last 2 columns.
> 255: tableView.getColumns().remove(tableView.getColumns().size() - 2,
> tableView.getColumns().size());
Note: I added this test in PR: https://github.com/openjdk/jfx/pull/444. While
testing this PR I found out that this removes just one column, therefore I
fixed it right away.
-
PR: https://git.openjdk.java.net/jfx/pull/800
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v14]
On Tue, 22 Mar 2022 07:46:40 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Standard JavaFX + Optional:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> Optinal.ofNullable(employee.get())
>> .map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse(""),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix wording
Not yet, but it's on my list to look at this week.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
On Tue, 24 May 2022 14:54:19 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding JGObject in plave of raw jni object > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 99: > >> 97: } >> 98: >> 99: void EventListenerManager::resetDOMWindow(DOMWindow* window) > > Have you validated this logic? If I understand correctly, the > `isReferringToOtherListener` var will be true if any listener in the list of > listeners for this DOM window has a ref count > 1. Is my understanding > correct? This doesn't seem quite right to me, but I may be missing something. yes , this is the case where we would remove pairs from map. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
On Sun, 22 May 2022 14:06:41 GMT, Jay Bhaskar wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue
>> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
>> are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
>> global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the
>> JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with
>> DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being
>> forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered
>> all the event listeners based on the ref counts attached with WebView
>> DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding JGObject in plave of raw jni object
I left a couple comments and one question on the changes to the fix.
The new tests look good. I left a few comments and suggestions on the test, but
overall they are good additions to verify the fix.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 29:
> 27: #include "JavaEventListener.h"
> 28: #include "DOMWindow.h"
> 29: #include
You probably don't need to include `stdio.h`
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 99:
> 97: }
> 98:
> 99: void EventListenerManager::resetDOMWindow(DOMWindow* window)
Have you validated this logic? If I understand correctly, the
`isReferringToOtherListener` var will be true if any listener in the list of
listeners for this DOM window has a ref count > 1. Is my understanding correct?
This doesn't seem quite right to me, but I may be missing something.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
line 53:
> 51: JavaObjectWrapperHandler(const JLObject& handler) {
> 52: JNIEnv *env = nullptr;
> 53: env = JavaScriptCore_GetJavaEnv();
You can remove these two lines, since `env` is now unused here.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
line 60:
> 58: JNIEnv *env = nullptr;
> 59: env = JavaScriptCore_GetJavaEnv();
> 60: env->DeleteGlobalRef(handler_);
I think you can replace these three lines with: `handler_.clear();`
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 96:
> 94: }
> 95:
> 96: static class MyListener1 implements EventListener {
There is no need for another subclass of `EventListener`. It is unused and can
be removed.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 630:
> 628: */
> 629: @Test
> 630: public void TestStrongRefNewContentLoad() throws Exception {
You should set `webView2 = null` here
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 657:
> 655: });
> 656:
> 657: // Verify that all listeners have not been released
This comment is not right. It should be something like:
// Verify that the click event is not delivered to the event handler.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 664:
> 662: listeners1.clear();
> 663: domNodes1.clear();
> 664: webViewRefs.clear();
This makes the subsequent call to `assertNumActive` ineffective. There should
never be a need for any test method to explicitly modify the global refs lists.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 672:
> 670: // Verify that no listeners are strongly held
> 671: assertNumActive("MyListener", listenerRefs, 0);
> 672: listenerRefs.clear();
It is not necessary to clear the `listenerRefs` list. There is never a need for
a test to explicitly modify this list.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 676:
> 674:
> 675: /**
> 676: * Test that the listener ref cont increase on addevent and decrease
> on remove event
Typo: cont -> count
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 706:
> 704: });
> 705:
> 706: // Verify that the events are delivered to the listeners (0 and
> 2 are same)
Actually all three refer to the same listener.
Re: RFR: [WIP] 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v6]
On Mon, 23 May 2022 20:44:47 GMT, Johan Vos wrote:
>> When the size of a ListCell is changed and a scrollTo method is invoked
>> without having a layout calculation in between, the old (wrong) size is used
>> to calculcate the total estimate. This happens e.g. when the size is changed
>> in the `updateItem` method.
>> This PR will immediately resize the cell and sets the new value in the cache
>> containing the cellsizes.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Try to keep visible cells at their original position when improving the
> estimate.
> This reduces annoying effects when re-layouting cells without relevant
> changes.
>
> This should not be done when we don't have a valid estimate yet/anymore,
> or when the absoluteOffset is set to infinity (which may happen if the
> position is set to infinity, which is the case in some calls in Skin
> classes).
Thank you for the update!
We've tested it, and we made good progress.
In our Application, we often resize the Cells, after the content is loaded.
We've polished the unit test for the following 3 cases.
(the changes are in this comment)
**1.) Not jumping to the last cell.**
Sometimes, when the last cell is very big, the ListView jumps to one cell above
it.
This was supposed to be caught by the previous tests, but due to a bug in the
tests, it was missed.
With this new definition of "shouldScrollToBottom" the bug is caught by the
unit test:
boolean isLastIndex = scrollToIndex == heights.length - 1;
boolean shouldScrollToBottom = (sizeBelow < viewportLength) || (isLastIndex &&
lastCellSize >= viewportLength);
**2.) Jumps on height changes**
When the heights of the cells changes, "everything jumps around".
Would it be possible to have the invariant, that the topmost visible element
shouldn't move?
Or alternatively: The selected element should be stable if it is visible.
If this would be implementable, this would make the VirtualFlow behave much
more "calm".
**3.) Jumps on click after height changes**
When the heights have changed, and an element is selected, then the position
jumps around again.
This is clearly a bug, because selecting a visible element shouldn't have the
effect that cells "fly around".
If they should move due to the height changes, this should happen when the
height is changed, not when a cell is selected.
For 2 and 3, I have some test-code, which can be added to the end of the method
"verifyListViewScrollTo":
// Additional Tests:
double previousLayoutY = scrollToCell.getLayoutY();
System.out.println("previousLayoutY: " + previousLayoutY);
if(previousLayoutY == 0) {
System.out.println("ADDITIONAL CHECKS");
// Upper cell shouldn't move after heights are changed
List alternateHeights = Arrays.stream(heights).map(x -> x +
250).collect(Collectors.toList());
listView.getItems().setAll(alternateHeights);
listView.requestLayout();
Toolkit.getToolkit().firePulse();
assertEquals("Upper cell shouldn't move after changing heights",
previousLayoutY, scrollToCell.getLayoutY(), 1.);
// After clicking, the cells shouldn't move
listView.getSelectionModel().select(scrollToIndex);
listView.requestLayout();
Toolkit.getToolkit().firePulse();
assertEquals("Upper cell shouldn't move after changing heights and
clicking", previousLayoutY, scrollToCell.getLayoutY(), 1.);
}
-
PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView
On Thu, 5 May 2022 16:21:45 GMT, Jose Pereda wrote: > This PR fixes an issue with selection of multiple items in TableView and > TreeTableView controls that gets moved unexpectedly when new items are added > even way below the selected items. > > A couple of tests have been added. They fail without this PR (first selected > item gets deselected, and table anchor gets moved), and pass with this PR > (first selected item keeps selected, and table anchor remains at the same > place). I've merged from head to fix conflicts, I guess it needs to be reviewed again? - PR: https://git.openjdk.java.net/jfx/pull/790
Re: RFR: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView [v2]
> This PR fixes an issue with selection of multiple items in TableView and > TreeTableView controls that gets moved unexpectedly when new items are added > even way below the selected items. > > A couple of tests have been added. They fail without this PR (first selected > item gets deselected, and table anchor gets moved), and pass with this PR > (first selected item keeps selected, and table anchor remains at the same > place). Jose Pereda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' into 8284665-tableviewanchor - Only shift anchor if existing one is affected by the change event, and tests - Changes: https://git.openjdk.java.net/jfx/pull/790/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=790=01 Stats: 101 lines in 4 files changed: 99 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/790.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/790/head:pull/790 PR: https://git.openjdk.java.net/jfx/pull/790
Re: RFR: [WIP] 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v6]
On Mon, 23 May 2022 20:44:47 GMT, Johan Vos wrote: >> When the size of a ListCell is changed and a scrollTo method is invoked >> without having a layout calculation in between, the old (wrong) size is used >> to calculcate the total estimate. This happens e.g. when the size is changed >> in the `updateItem` method. >> This PR will immediately resize the cell and sets the new value in the cache >> containing the cellsizes. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Try to keep visible cells at their original position when improving the > estimate. > This reduces annoying effects when re-layouting cells without relevant > changes. > > This should not be done when we don't have a valid estimate yet/anymore, > or when the absoluteOffset is set to infinity (which may happen if the > position is set to infinity, which is the case in some calls in Skin > classes). To give some background: the reason for this PR is to fix (real or perceived) regression after https://bugs.openjdk.java.net/browse/JDK-8089589 was fixed. The original problem was this: in case a ListView contains items with different heights, the scrollbar can show weird jumps as it considers all cells to have the same height. The "easy" solution to this would be to pre-calculate the height of all cells, but that would be a potential performance issue. The approach taken in JDK-8089589 is to gradually improve the information about the different sizes of the cells, and it fixes the weird jumps (at the very least, it avoids jumping upwards when scrolling down and vice versa). However, this gradual improvements can cause other visual disturbances. Initially, we improved the parameters whenever a layoutChildren was invoked. However, this leads to undesired effects in case layoutChildren is invoked again after a specific goal is asked (e.g. scroll to a cell with a specific index). A consequence of the gradual improvement is that the algorithm realizes the first cell need to move upwards or downwards a bit, but that will destroy the state where a cell should be positioned exactly at a given position. The latest commit fixes this, by modifying the parameters (absoluteOffset and position) in such a way that they lead to improvements, yet keep the visible cells intact. This broke a number of tests, as there are cases where keeping these cells intact is not what we want. When there is no cached estimate, or when the absoluteOffset has a bogus value, we do not want to maintain the previous situation. I'm moving this PR to draft until I have more confirmation about the (perceived) stability. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v6]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Try to keep visible cells at their original position when improving the estimate. This reduces annoying effects when re-layouting cells without relevant changes. This should not be done when we don't have a valid estimate yet/anymore, or when the absoluteOffset is set to infinity (which may happen if the position is set to infinity, which is the case in some calls in Skin classes). - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/aa08ba26..c7d722d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=04-05 Stats: 121 lines in 2 files changed: 75 ins; 29 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v4]
On Wed, 18 May 2022 13:39:12 GMT, Hima Bindu Meda wrote: >> Updated libxml to version 2.9.14.As mentioned in UPDATING.txt, configured >> for windows , linux and Mac platforms and updated the files accordingly. >> >> Updated libxslt to version 1.1.35. Generated the config.h files for windows >> , Mac and linux platforms and updated accordingly. >> >> Verified the build on all the platforms and sanity testing looks fine.No >> regressions observed. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Space correction works with our new build approach. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/797
Re: @font-face question
Hi,
After further investigation, I think there's a bug that font-family is not
working (defining a font family name).
Because regardless of the font-family I use, the font name within the font
is used.
It's openjfx 17.0.2
-- Thiago.
Em sex., 20 de mai. de 2022 às 18:06, Thiago Milczarek Sayão <
thiago.sa...@gmail.com> escreveu:
> Hi,
>
> I want to have one font that is "regular" and a variation of the same font
> as "bold", used when specified "-fx-font-weight: bold".
>
> @font-face {
> font-family: 'Gotham Condensed';
> src: url('../fonts/GothamCondensed-Book.otf');
> }
>
> @font-face {
> font-family: 'Gotham Condensed Bold';
> src: url('../fonts/GothamCondensed-Bold.otf');
> }
>
>
> .text {
> -fx-font-family: 'Gotham Condensed';
> }
>
> I have searched online and there are many variations such as:
>
> - @font-face "font-family" is ignored and you have to use the real font
> name;
> - If you use 'Bold' as a suffix, javafx will use it as bold;
> - you have to specify "font-weight: bold" on the @font-face
>
> The last one is what seems that the CSS specs says, but does not seem to
> work.
>
> Is it possible?
>
> Thanks.
>
>
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adding JGObject in plave of raw jni object - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/5dc52378..43f8e549 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=02-03 Stats: 32 lines in 3 files changed: 25 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
On Thu, 19 May 2022 21:45:00 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
> line 48:
>
>> 46:
>> 47: class JavaObjectWrapperHandler {
>> 48: jobject handler_;
>
> Have you considered using `JGObject` here instead of raw `jobject`?
Applied
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
On Thu, 19 May 2022 21:34:54 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 57:
>
>> 55: if (it->second && it->second->use_count() == 1) {
>> 56: delete it->second;
>> 57: it->second = nullptr;
>
> Do you need to remove the item from the list in this case?
applied new change , for removal of unused pairs
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 91:
>
>> 89: for (win_it = windowHasEvent.begin(); win_it !=
>> windowHasEvent.end(); win_it++) {
>> 90: // de register associated event listeners with window
>> 91: if(window == win_it->second) {
>
> Minor: add space after the `if`.
done
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 92:
>
>> 90: // de register associated event listeners with window
>> 91: if(window == win_it->second) {
>> 92: unregisterListener(win_it->first);
>
> Same question as earlier: do you need to also remove the entries matching the
> `window` from the list here?
applied
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
> line 48:
>
>> 46:
>> 47: class JavaObjectWrapperHandler {
>> 48: jobject handler_;
>
> Have you considered using `JGObject` here instead of raw `jobject`?
No
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v3]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: remove data from map and extend unit test - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/2d5604bf..5dc52378 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=01-02 Stats: 419 lines in 6 files changed: 407 ins; 7 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
On Fri, 6 May 2022 10:16:41 GMT, Jose Pereda wrote: > This PR extends the check if a treeItem is expanded to all its ancestors, as > in case one ancestor is collapsed, all its children will be hidden. > > 4 tests are included, two for TreeView and two for TreeTableView. This looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/791
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
On Thu, 19 May 2022 22:01:31 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
> line 637:
>
>> 635: });
>> 636:
>> 637: assertEquals("Click count", 1,
>> listeners1.get(0).getClickCount());
>
> You should add a comment that this check is testing that the immediately
> previous click does _not_ get delivered since the associated DOM node is not
> part of the page any more. This is why the count remains at 1 (from the first
> click on the original page).
>
>
> Also, I think it would be useful here to clear the references to the
> listeners and WebView and make sure that the listener attached to the
> previously loaded page for this WebView gets released. Something like this as
> the final statements of the method:
>
>
> // Clear strong reference to listener and WebView
> listeners1.clear();
> webView1 = null;
>
> // Verify that there is no strong reference to the WebView
> assertNumActive("WebView", webViewRefs, 0);
>
> // Verify that no listeners are strongly held
> assertNumActive("MyListener", listenerRefs, 0);
You will also need to clear the references to the DOM nodes, and set `webView2
= null;` for this to work.
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8284654: Modal behavior returns to wrong stage [v3]
On Tue, 10 May 2022 12:56:53 GMT, Thiago Milczarek Sayao wrote: >> When there's an APPLICATION_MODAL window, all other windows are disabled and >> re-enabled when the APPLICATION_MODAL window closes. This causes >> `requestToFront()` to be called on every window, and it does not guarantee >> order. >> >> The fix also works for: >> https://bugs.openjdk.java.net/browse/JDK-8269429 >> >> But this one might need another fix: >> https://bugs.openjdk.java.net/browse/JDK-8240640 > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Set the focus on the latest window when re-enabling them" > > This reverts commit b024de586c187f9000f791dab99507a4979cc027. Marked as reviewed by arapte (Reviewer). The fix look good to me. The other behavior is observed on mac but not on windows. The platform behavior itself looks different. Not sure if should be captured in a JBS (at least for documentation purpose) - PR: https://git.openjdk.java.net/jfx/pull/784
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
On Thu, 19 May 2022 15:57:46 GMT, Jay Bhaskar wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue
>> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
>> are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
>> global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the
>> JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with
>> DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being
>> forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered
>> all the event listeners based on the ref counts attached with WebView
>> DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Platform.exit() , removing code block, as it is causing other test fail
The fix looks good with a couple questions about removing entries from the map
when they are done and some cleanup comments.
In addition to the inline comments I left about the test, I think the following
scenarios should be added to the automated test:
1. Multiple listeners single webiew implicit release (i.e., WebView goes out of
scope)
2. Multiple listeners multiple webiew with explicit release of one webview and
implicit release of the other (this one is basically a combination of the
others)
3. Ref count test (single webview is fine) with adding and removing listeners.
It should handle the cases of both increasing and decreasing the ref count, and
adding the listener to another DOM node after its ref count has gone to zero to
make sure that case works.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 57:
> 55: if (it->second && it->second->use_count() == 1) {
> 56: delete it->second;
> 57: it->second = nullptr;
Do you need to remove the item from the list in this case?
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 91:
> 89: for (win_it = windowHasEvent.begin(); win_it != windowHasEvent.end();
> win_it++) {
> 90: // de register associated event listeners with window
> 91: if(window == win_it->second) {
Minor: add space after the `if`.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 92:
> 90: // de register associated event listeners with window
> 91: if(window == win_it->second) {
> 92: unregisterListener(win_it->first);
Same question as earlier: do you need to also remove the entries matching the
`window` from the list here?
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
line 48:
> 46:
> 47: class JavaObjectWrapperHandler {
> 48: jobject handler_;
Have you considered using `JGObject` here instead of raw `jobject`?
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/JavaEventListener.cpp
line 50:
> 48: ? static_cast JavaEventListener*>()
> 49: : nullptr;
> 50: return jother && ( this == jother);
Minor: you can remove the space after `(`
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/JavaEventListener.h
line 32:
> 30: #include "Node.h"
> 31:
> 32: #if PLATFORM(JAVA)
This is a java platform-specific class, so you don't need the `#if
PLATFORM(JAVA)` in this file.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 43:
> 41: import javafx.scene.web.WebEngine;
> 42: import javafx.scene.web.WebView;
> 43: import org.junit.AfterClass;
This import is no longer used.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 53:
> 51: import org.junit.Before;
> 52: import org.w3c.dom.Document;
> 53: import org.w3c.dom.NodeList;
Minor: maybe move these up to the previous block (with the ordinary imports?
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 631:
> 629:
> 630: // Verify that all listeners have not been released
> 631: Thread.sleep(100);
The sleep should be right before the call to `getClickCount` (as in other cases
in the test).
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 637:
> 635: });
> 636:
> 637: assertEquals("Click count", 1,
>
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Platform.exit() , removing code block, as it is causing other test fail - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/a62558ea..2d5604bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=00-01 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
On Thu, 19 May 2022 15:22:08 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
> line 113:
>
>> 111: public static void cleanupOnce() {
>> 112: Platform.exit();
>> 113: }
>
> I just noticed that this will cause subsequent tests to fail. Unit tests
> should not call `Platform.exit()` (as opposed to system tests, which should).
> You can remove the entire method.
applied
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener
On Thu, 19 May 2022 13:13:01 GMT, Jay Bhaskar wrote:
> This PR is new implementation of JavaEvent listener memory management.
> Issue
> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>
> 1. Calling remove event listener does not free jni global references.
> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
> are not being garbage collected.
>
> Solution:
> 1. Detached the jni global reference from JavaEventListener.
> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
> global reference.
> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
> 4. EventListenerManager is a singleton class , which stores the
> JavaObjectWrapperHandler mapped with JavaEventListener.
> 5. EventListenerManager also stores the JavaEventListener mapped with
> DOMWindow.
> 6. When Event listener explicitly removed , JavaEventListener is being
> forwarded to EventListenerManager to clear the listener.
> 7. When WebView goes out of scope, EventListenerManager will de-registered
> all the event listeners based on the ref counts attached with WebView
> DOMWindow.
While testing, I noticed one problem in that the new unit tests calls
`Platform.exit()` (this was my fault for adding it when sending you the test
case), causing any subsequent web test to fail.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 113:
> 111: public static void cleanupOnce() {
> 112: Platform.exit();
> 113: }
I just noticed that this will cause subsequent tests to fail. Unit tests should
not call `Platform.exit()` (as opposed to system tests, which should). You can
remove the entire method.
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]
On Thu, 19 May 2022 14:23:55 GMT, Robert Lichtenberger wrote: >> Separate test class added for TreeTableView case. >> Fix is analogous to JDK-8251480. > > Robert Lichtenberger has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups #2. > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]
On Thu, 19 May 2022 14:23:55 GMT, Robert Lichtenberger wrote: >> Separate test class added for TreeTableView case. >> Fix is analogous to JDK-8251480. > > Robert Lichtenberger has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups #2. > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups. Marked as reviewed by mhanl (Author). - PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]
On Thu, 12 May 2022 05:22:38 GMT, Ajit Ghaisas wrote: >> Robert Lichtenberger has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - 8285197: TableColumnHeader: calc of cell width must respect row styling >> (TreeTableView) >> >>Test class cosmetic cleanups #2. >> - 8285197: TableColumnHeader: calc of cell width must respect row styling >> (TreeTableView) >> >>Test class cosmetic cleanups. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights >> reserved. > > Replace "2018, 2021" with just "2022" as this is newly introduced test file. done > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java > line 51: > >> 49: import java.util.List; >> 50: >> 51: import static org.junit.Assert.*; > > Replace generic import statement with specific ones. done - PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
On Thu, 21 Apr 2022 08:38:20 GMT, Robert Lichtenberger wrote: > Separate test class added for TreeTableView case. > Fix is analogous to JDK-8251480. Finally found the time to cleanup the test class... - PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]
> Separate test class added for TreeTableView case. > Fix is analogous to JDK-8251480. Robert Lichtenberger has updated the pull request incrementally with two additional commits since the last revision: - 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) Test class cosmetic cleanups #2. - 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) Test class cosmetic cleanups. - Changes: - all: https://git.openjdk.java.net/jfx/pull/779/files - new: https://git.openjdk.java.net/jfx/pull/779/files/38d930a8..68f7c806 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=779=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=779=00-01 Stats: 13 lines in 1 file changed: 3 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/779.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/779/head:pull/779 PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8090522: Allow support for disabling stage iconification (minimization)
On Thu, 19 May 2022 12:44:44 GMT, Paweł Kruszczyński wrote: > `Stage` minimalizing (iconified state) cannot be disabled for the user. The > same for `Stage` closable state. > PR adds these features without breaking backward compatibility. Marking this as "Draft" since it is not ready for review. - PR: https://git.openjdk.java.net/jfx/pull/798
Re: RFR: 8090522: Allow support for disabling stage iconification (minimization)
On Thu, 19 May 2022 12:44:44 GMT, Paweł Kruszczyński wrote: > `Stage` minimalizing (iconified state) cannot be disabled for the user. The > same for `Stage` closable state. > PR adds these features without breaking backward compatibility. @xardif It is premature to review this PR. Please read the section on [New features / API additions](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#new-features--api-additions) in the [CONTRIBUTING](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md) guide for necessary steps, which involves discussing any new feature on the openjfx-dev mailing list to see if there is general support for the idea. - PR: https://git.openjdk.java.net/jfx/pull/798
Re: RFR: 8261221: Tooltip bigger than screen size blinks - shows and hides over and over again [v4]
> `Tooltip` is no longer hiding upon receiving > `MouseEvent.MOUSE_ENTERED_TARGET` event inside it. Pressing mouse on > overlaying tooltip also kills the tooltip, so the infinite duration tooltip > can be closed. Paweł Kruszczyński 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 four additional commits since the last revision: - Merge branch 'openjdk:master' into 8261221_tooltip_blinks - Merge branch 'openjdk:master' into 8261221_tooltip_blinks - 8261221: Tooltip bigger than screen size blinks - applying reviewed fixes - 8261221: Tooltip bigger than screen size blinks - shows and hides over and over again - Changes: - all: https://git.openjdk.java.net/jfx/pull/395/files - new: https://git.openjdk.java.net/jfx/pull/395/files/0a175dca..7dc2ff37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=395=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=395=02-03 Stats: 5600 lines in 7 files changed: 5583 ins; 3 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/395.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/395/head:pull/395 PR: https://git.openjdk.java.net/jfx/pull/395
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v9]
> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as > well as the corresponding `:focus-visible` and `:focus-within` CSS > pseudo-classes. Michael Strauß 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 23 additional commits since the last revision: - Merge branch 'master' into feature/focusvisible - Updated since tag - Update copyright headers - Merge branch 'master' into feature/focusvisible - fixed undeterministic test failures - minor wording change - restart github actions - Merge branch 'master' of https://github.com/mstr2/jfx into feature/focusvisible - changes per discussion, added test - wording - ... and 13 more: https://git.openjdk.java.net/jfx/compare/a6d4f5e5...0c7d6e72 - Changes: - all: https://git.openjdk.java.net/jfx/pull/475/files - new: https://git.openjdk.java.net/jfx/pull/475/files/bd57bbac..0c7d6e72 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=475=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=475=07-08 Stats: 101583 lines in 580 files changed: 34334 ins; 31264 del; 35985 mod Patch: https://git.openjdk.java.net/jfx/pull/475.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/475/head:pull/475 PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v14]
On Tue, 22 Mar 2022 20:17:36 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix wording > > Yes, I definitely want to be one of the reviewers. I'll need to review the > CSR in any case. @kevinrushforth have you had a chance to look at this PR? - PR: https://git.openjdk.java.net/jfx/pull/675
Re: Looked-up color fails for -fx-background-color in JavaFX CSS file
I've tried the attached code as well, and I've been unable to reproduce it. I even modified the code a bit to clear and set the stylesheet every frame, and it runs fine. Also used the debugger a bit to see what's going on in CssStyleHelper#calculateValue but didn't see any obvious flaws. If you want, you could put a breakpoint on line 1634 in that class (just after #calculateValue's "catch(ClassCastException)") and see if you can see something odd going on. The stack trace might be useful to have, and perhaps why it occured if you could dig a bit deeper. The only way I could get it to fail is to put an actual bad value in "-theme-button". Are you doing anything else special while running the code? Or do you have a different example case that shows the problem? --John On 18/05/2022 18:05, Davide Perini wrote: Yes, I'm sorry, that was the issue. Thanks Davide Il 18/05/2022 14:10, Kevin Rushforth ha scritto: Hi Davide, Are you referring to https://bugs.openjdk.java.net/browse/JDK-8268657 ? I just tried it again, and it works fine for me. -- Kevin On 5/18/2022 4:12 AM, Davide Perini wrote: Hi all, someone else opened an issue in past on this problem. I have the exact same problem: https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-June/030723.html Kevin closed the issue because it was not able to reproduce but I can reproduce it on JavaFX 18.0.1 Can you double check it please? Thanks Davide
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Fri, 13 May 2022 05:34:08 GMT, Tomator wrote:
> When I used BlueJ, I found a problem with Chinese display.
> /javafx.graphics/com/sun/javafx/font/CompositeGlyphMapper.java#getGlyphCode
> may return a negative number when no font library is specified in Linux,and
> this could cause java.lang.ArrayIndexOutOfBoundsException error.So
> javafx.graphics/com/sun/prism/impl/GlyphCache.java#getCachedGlyph shou check
> the glyphCode.
> The crash demo like this:
> `import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.Menu;
> import javafx.scene.control.MenuBar;
> import javafx.scene.control.MenuItem;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
>
> public class MenuExample extends Application {
> public static void main(String[] args) {
> launch(args);
> }
>
> @Override
> public void start(Stage primaryStage) throws Exception {
> BorderPane root = new BorderPane();
> Scene scene = new Scene(root,200,300);
> MenuBar menubar = new MenuBar();
> Menu FileMenu = new Menu("文本");
> MenuItem filemenu1=new MenuItem("新建");
> MenuItem filemenu2=new MenuItem("Save");
> MenuItem filemenu3=new MenuItem("Exit");
> Menu EditMenu=new Menu("Edit");
> MenuItem EditMenu1=new MenuItem("Cut");
> MenuItem EditMenu2=new MenuItem("Copy");
> MenuItem EditMenu3=new MenuItem("Paste");
> EditMenu.getItems().addAll(EditMenu1,EditMenu2,EditMenu3);
> root.setTop(menubar);
> FileMenu.getItems().addAll(filemenu1,filemenu2,filemenu3);
> menubar.getMenus().addAll(FileMenu,EditMenu);
> primaryStage.setScene(scene);
> primaryStage.setTitle("TEST");
> primaryStage.show();
>
> }
> } `
>
> the error:
> `java.lang.ArrayIndexOutOfBoundsException: Index -25 out of bounds for length
> 32
> at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
> at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
> at
> com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
> at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
> at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
> at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
> at
> com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
> at
> com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
> `
Reading the author's description of this PR, one wonders why the added
condition is not "glyphCode < 0".
``` java
if (glyphCode <= 0) {return null;}
-
PR: https://git.openjdk.java.net/jfx/pull/795
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Tue, 17 May 2022 13:08:10 GMT, Tomator wrote: > Hello,i have signed oca ,and pull /signed command ,so I'm just wait for OCA > to pass？ Yes, at this point you wait for your OCA to be processed. - PR: https://git.openjdk.java.net/jfx/pull/795
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Tue, 17 May 2022 13:34:47 GMT, yosbits wrote:
> Reading the author's description of this PR, one wonders why the added
> condition is not "glyphCode < 0".
>
> ```java
> if (glyphCode <= 0) {return null;}
> ```
Your suggestion is right
-
PR: https://git.openjdk.java.net/jfx/pull/795
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Fri, 13 May 2022 12:21:32 GMT, Kevin Rushforth wrote: > * Submit a bug report with a test case at > [bugreport.java.com](https://bugreport.java.com/) I see that you already have submitted a bug report. You should get an email when it is made public. - PR: https://git.openjdk.java.net/jfx/pull/795
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Fri, 13 May 2022 05:36:03 GMT, Tomator wrote:
>> When I used BlueJ, I found a problem with Chinese display.
>> /javafx.graphics/com/sun/javafx/font/CompositeGlyphMapper.java#getGlyphCode
>> may return a negative number when no font library is specified in Linux,and
>> this could cause java.lang.ArrayIndexOutOfBoundsException error.So
>> javafx.graphics/com/sun/prism/impl/GlyphCache.java#getCachedGlyph shou
>> check the glyphCode.
>> The crash demo like this:
>> `import javafx.application.Application;
>> import javafx.scene.Scene;
>> import javafx.scene.control.Menu;
>> import javafx.scene.control.MenuBar;
>> import javafx.scene.control.MenuItem;
>> import javafx.scene.layout.BorderPane;
>> import javafx.stage.Stage;
>>
>> public class MenuExample extends Application {
>> public static void main(String[] args) {
>> launch(args);
>> }
>>
>> @Override
>> public void start(Stage primaryStage) throws Exception {
>> BorderPane root = new BorderPane();
>> Scene scene = new Scene(root,200,300);
>> MenuBar menubar = new MenuBar();
>> Menu FileMenu = new Menu("文本");
>> MenuItem filemenu1=new MenuItem("新建");
>> MenuItem filemenu2=new MenuItem("Save");
>> MenuItem filemenu3=new MenuItem("Exit");
>> Menu EditMenu=new Menu("Edit");
>> MenuItem EditMenu1=new MenuItem("Cut");
>> MenuItem EditMenu2=new MenuItem("Copy");
>> MenuItem EditMenu3=new MenuItem("Paste");
>> EditMenu.getItems().addAll(EditMenu1,EditMenu2,EditMenu3);
>> root.setTop(menubar);
>> FileMenu.getItems().addAll(filemenu1,filemenu2,filemenu3);
>> menubar.getMenus().addAll(FileMenu,EditMenu);
>> primaryStage.setScene(scene);
>> primaryStage.setTitle("TEST");
>> primaryStage.show();
>>
>> }
>> } `
>>
>> the error:
>> `java.lang.ArrayIndexOutOfBoundsException: Index -25 out of bounds for
>> length 32
>> at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
>> at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
>> at
>> com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
>> at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
>> at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
>> at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at
>> com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
>> at
>> com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
>> at
>> com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
>> `
>
> The crash demo like this:
> `import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.Menu;
> import javafx.scene.control.MenuBar;
> import javafx.scene.control.MenuItem;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
>
> public class MenuExample extends Application {
> public static void main(String[] args) {
> launch(args);
> }
>
> @Override
> public void start(Stage primaryStage) throws Exception {
> BorderPane root = new BorderPane();
> Scene scene = new
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Mon, 16 May 2022 08:12:54 GMT, Tomator wrote:
>> When I used BlueJ, I found a problem with Chinese display.
>> /javafx.graphics/com/sun/javafx/font/CompositeGlyphMapper.java#getGlyphCode
>> may return a negative number when no font library is specified in Linux,and
>> this could cause java.lang.ArrayIndexOutOfBoundsException error.So
>> javafx.graphics/com/sun/prism/impl/GlyphCache.java#getCachedGlyph shou
>> check the glyphCode.
>> The crash demo like this:
>> `import javafx.application.Application;
>> import javafx.scene.Scene;
>> import javafx.scene.control.Menu;
>> import javafx.scene.control.MenuBar;
>> import javafx.scene.control.MenuItem;
>> import javafx.scene.layout.BorderPane;
>> import javafx.stage.Stage;
>>
>> public class MenuExample extends Application {
>> public static void main(String[] args) {
>> launch(args);
>> }
>>
>> @Override
>> public void start(Stage primaryStage) throws Exception {
>> BorderPane root = new BorderPane();
>> Scene scene = new Scene(root,200,300);
>> MenuBar menubar = new MenuBar();
>> Menu FileMenu = new Menu("文本");
>> MenuItem filemenu1=new MenuItem("新建");
>> MenuItem filemenu2=new MenuItem("Save");
>> MenuItem filemenu3=new MenuItem("Exit");
>> Menu EditMenu=new Menu("Edit");
>> MenuItem EditMenu1=new MenuItem("Cut");
>> MenuItem EditMenu2=new MenuItem("Copy");
>> MenuItem EditMenu3=new MenuItem("Paste");
>> EditMenu.getItems().addAll(EditMenu1,EditMenu2,EditMenu3);
>> root.setTop(menubar);
>> FileMenu.getItems().addAll(filemenu1,filemenu2,filemenu3);
>> menubar.getMenus().addAll(FileMenu,EditMenu);
>> primaryStage.setScene(scene);
>> primaryStage.setTitle("TEST");
>> primaryStage.show();
>>
>> }
>> } `
>>
>> the error:
>> `java.lang.ArrayIndexOutOfBoundsException: Index -25 out of bounds for
>> length 32
>> at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
>> at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
>> at
>> com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
>> at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
>> at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
>> at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
>> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
>> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
>> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
>> at
>> com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
>> at
>> com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
>> at
>> com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
>> `
>
>>
>
>
>
>>
>
> thanks,I'll wait for the bug
> @Tomator01 thank you for your interest in contributing to the OpenJFX project.
>
> In order for any comments that you add to this (or any other) PR to be
> retained, you will need to accept the OpenJDK Terms of Use as shown above.
> This is a one-time step.
>
> Please read the
> [CONTRIBUTING](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md)
> guidelines, in particular the [Before submitting a pull
> request](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#before-submitting-a-pull-request)
>
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Fri, 13 May 2022 05:34:08 GMT, Tomator wrote:
> When I used BlueJ, I found a problem with Chinese display.
> /javafx.graphics/com/sun/javafx/font/CompositeGlyphMapper.java#getGlyphCode
> may return a negative number when no font library is specified in Linux,and
> this could cause java.lang.ArrayIndexOutOfBoundsException error.So
> javafx.graphics/com/sun/prism/impl/GlyphCache.java#getCachedGlyph shou check
> the glyphCode.
> The crash demo like this:
> `import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.Menu;
> import javafx.scene.control.MenuBar;
> import javafx.scene.control.MenuItem;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
>
> public class MenuExample extends Application {
> public static void main(String[] args) {
> launch(args);
> }
>
> @Override
> public void start(Stage primaryStage) throws Exception {
> BorderPane root = new BorderPane();
> Scene scene = new Scene(root,200,300);
> MenuBar menubar = new MenuBar();
> Menu FileMenu = new Menu("文本");
> MenuItem filemenu1=new MenuItem("新建");
> MenuItem filemenu2=new MenuItem("Save");
> MenuItem filemenu3=new MenuItem("Exit");
> Menu EditMenu=new Menu("Edit");
> MenuItem EditMenu1=new MenuItem("Cut");
> MenuItem EditMenu2=new MenuItem("Copy");
> MenuItem EditMenu3=new MenuItem("Paste");
> EditMenu.getItems().addAll(EditMenu1,EditMenu2,EditMenu3);
> root.setTop(menubar);
> FileMenu.getItems().addAll(filemenu1,filemenu2,filemenu3);
> menubar.getMenus().addAll(FileMenu,EditMenu);
> primaryStage.setScene(scene);
> primaryStage.setTitle("TEST");
> primaryStage.show();
>
> }
> } `
>
> the error:
> `java.lang.ArrayIndexOutOfBoundsException: Index -25 out of bounds for length
> 32
> at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
> at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
> at
> com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
> at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
> at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
> at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
> at
> com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
> at
> com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
> `
The crash demo like this:
`import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Menu;
import javafx.scene.control.MenuBar;
import javafx.scene.control.MenuItem;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;
public class MenuExample extends Application {
public static void main(String[] args) {
launch(args);
}
@Override
public void start(Stage primaryStage) throws Exception {
BorderPane root = new BorderPane();
Scene scene = new Scene(root,200,300);
MenuBar menubar = new MenuBar();
Menu FileMenu = new Menu("文本");
MenuItem
Re: Looked-up color fails for -fx-background-color in JavaFX CSS file
Yes, I'm sorry, that was the issue. Thanks Davide Il 18/05/2022 14:10, Kevin Rushforth ha scritto: Hi Davide, Are you referring to https://bugs.openjdk.java.net/browse/JDK-8268657 ? I just tried it again, and it works fine for me. -- Kevin On 5/18/2022 4:12 AM, Davide Perini wrote: Hi all, someone else opened an issue in past on this problem. I have the exact same problem: https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-June/030723.html Kevin closed the issue because it was not able to reproduce but I can reproduce it on JavaFX 18.0.1 Can you double check it please? Thanks Davide
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v4]
On Wed, 18 May 2022 13:39:12 GMT, Hima Bindu Meda wrote: >> Updated libxml to version 2.9.14.As mentioned in UPDATING.txt, configured >> for windows , linux and Mac platforms and updated the files accordingly. >> >> Updated libxslt to version 1.1.35. Generated the config.h files for windows >> , Mac and linux platforms and updated accordingly. >> >> Verified the build on all the platforms and sanity testing looks fine.No >> regressions observed. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Space correction Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/797
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v3]
On Wed, 18 May 2022 12:27:46 GMT, Kevin Rushforth wrote: >> Hima Bindu Meda has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update version inof related steps > > modules/javafx.web/src/main/native/Source/ThirdParty/libxml/UPDATING.txt line > 44: > >> 42: 9.2 Copy libxml\src\config.h to libxml\linux\config.h >> 43: >> 44: 10. Update version info in >> 'modules/javafx.web/src/main/legal/libxml.md'.Also, update copyright if any >> new files are added. > > Minor: missing space before `Also`. done > modules/javafx.web/src/main/native/Source/ThirdParty/libxslt/UPDATING.txt > line 37: > >> 35: 9.1 Copy `libxslt/src/config.h` to `libxslt/linux/config.h` and follow >> same guidelines as Windows to retain changes from our repo. >> 36: >> 37: 10. Update version info in >> 'modules/javafx.web/src/main/legal/libxslt.md'.Also, update copyright if any >> new files are added. > > Minor: missing space before `Also`. done - PR: https://git.openjdk.java.net/jfx/pull/797
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v4]
> Updated libxml to version 2.9.14.As mentioned in UPDATING.txt, configured for > windows , linux and Mac platforms and updated the files accordingly. > > Updated libxslt to version 1.1.35. Generated the config.h files for windows , > Mac and linux platforms and updated accordingly. > > Verified the build on all the platforms and sanity testing looks fine.No > regressions observed. Hima Bindu Meda has updated the pull request incrementally with one additional commit since the last revision: Space correction - Changes: - all: https://git.openjdk.java.net/jfx/pull/797/files - new: https://git.openjdk.java.net/jfx/pull/797/files/df0207f3..e3ea6633 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=797=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=797=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/797.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/797/head:pull/797 PR: https://git.openjdk.java.net/jfx/pull/797
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v3]
On Wed, 18 May 2022 07:26:56 GMT, Hima Bindu Meda wrote: >> Updated libxml to version 2.9.14.As mentioned in UPDATING.txt, configured >> for windows , linux and Mac platforms and updated the files accordingly. >> >> Updated libxslt to version 1.1.35. Generated the config.h files for windows >> , Mac and linux platforms and updated accordingly. >> >> Verified the build on all the platforms and sanity testing looks fine.No >> regressions observed. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Update version inof related steps @johanvos or @tiainen Can one of you be the second reviewer on this? - PR: https://git.openjdk.java.net/jfx/pull/797
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v2]
On Tue, 17 May 2022 17:25:16 GMT, Kevin Rushforth wrote: >> Hima Bindu Meda has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add UPDATING.txt for libxslt update > > modules/javafx.web/src/main/native/Source/ThirdParty/libxslt/linux/libexslt/exsltconfig.h > line 21: > >> 19: * the version string like "1.2.3" >> 20: */ >> 21: #define LIBEXSLT_DOTTED_VERSION "0.8.20" > > Shouldn't this be `1.1.35`? Actually, now that I see it, it looks correct as is, so you can ignore this question. - PR: https://git.openjdk.java.net/jfx/pull/797
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v3]
On Wed, 18 May 2022 07:26:56 GMT, Hima Bindu Meda wrote: >> Updated libxml to version 2.9.14.As mentioned in UPDATING.txt, configured >> for windows , linux and Mac platforms and updated the files accordingly. >> >> Updated libxslt to version 1.1.35. Generated the config.h files for windows >> , Mac and linux platforms and updated accordingly. >> >> Verified the build on all the platforms and sanity testing looks fine.No >> regressions observed. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Update version inof related steps The updated instructions look good with one minor issue (a missing space between a `.` and the start of the next sentence). modules/javafx.web/src/main/native/Source/ThirdParty/libxml/UPDATING.txt line 44: > 42: 9.2 Copy libxml\src\config.h to libxml\linux\config.h > 43: > 44: 10. Update version info in > 'modules/javafx.web/src/main/legal/libxml.md'.Also, update copyright if any > new files are added. Minor: missing space before `Also`. modules/javafx.web/src/main/native/Source/ThirdParty/libxslt/UPDATING.txt line 37: > 35: 9.1 Copy `libxslt/src/config.h` to `libxslt/linux/config.h` and follow > same guidelines as Windows to retain changes from our repo. > 36: > 37: 10. Update version info in > 'modules/javafx.web/src/main/legal/libxslt.md'.Also, update copyright if any > new files are added. Minor: missing space before `Also`. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/797
Re: Looked-up color fails for -fx-background-color in JavaFX CSS file
Hi Davide, Are you referring to https://bugs.openjdk.java.net/browse/JDK-8268657 ? I just tried it again, and it works fine for me. -- Kevin On 5/18/2022 4:12 AM, Davide Perini wrote: Hi all, someone else opened an issue in past on this problem. I have the exact same problem: https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-June/030723.html Kevin closed the issue because it was not able to reproduce but I can reproduce it on JavaFX 18.0.1 Can you double check it please? Thanks Davide
Re: Looked-up color fails for -fx-background-color in JavaFX CSS file
Could you share the program that reproduces this, as I didn't find an issue nor a sample program in the link you shared. You mentioned an issue was closed? Which issue? --John On 18/05/2022 13:12, Davide Perini wrote: Hi all, someone else opened an issue in past on this problem. I have the exact same problem: https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-June/030723.html Kevin closed the issue because it was not able to reproduce but I can reproduce it on JavaFX 18.0.1 Can you double check it please? Thanks Davide
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v2]
On Tue, 17 May 2022 17:24:11 GMT, Kevin Rushforth wrote: >> Hima Bindu Meda has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add UPDATING.txt for libxslt update > > modules/javafx.web/src/main/native/Source/ThirdParty/libxslt/UPDATING.txt > line 15: > >> 13: > cscript configure.js compiler=msvc xslt_debug=no iconv=no >> mem_debug=no modules=no zlib=no profiler=no trio=no >> 14: - Above command generates a header file 'src/config.h' file (on >> all platforms) and libxslt\src\libxslt\xsltconfig.h. >> 15: 4.1 Copy `libxslt\src\config.h` to `libxslt\win32\config.h'. config.h >> file defines several macros to control libxslt features. We do not require >> all of the features to be enabled. > > Minor: trailing whitespace (since this is a text file, jcheck won't complain, > but since you will be editing this file anyway, maybe you can fix this). Done - PR: https://git.openjdk.java.net/jfx/pull/797
Re: RFR: JDK-8286256 : Update libxml2 to 2.9.14 [v3]
> Updated libxml to version 2.9.14.As mentioned in UPDATING.txt, configured for > windows , linux and Mac platforms and updated the files accordingly. > > Updated libxslt to version 1.1.35. Generated the config.h files for windows , > Mac and linux platforms and updated accordingly. > > Verified the build on all the platforms and sanity testing looks fine.No > regressions observed. Hima Bindu Meda has updated the pull request incrementally with one additional commit since the last revision: Update version inof related steps - Changes: - all: https://git.openjdk.java.net/jfx/pull/797/files - new: https://git.openjdk.java.net/jfx/pull/797/files/b9091ece..df0207f3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=797=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=797=01-02 Stats: 12 lines in 2 files changed: 5 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/797.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/797/head:pull/797 PR: https://git.openjdk.java.net/jfx/pull/797
