Re: JDK-8177945 : Single cell selection flickers when adding data to TableView
Thanks for taking a look at this David. Danny > On 3 Mar 2020, at 16:50, David Grieve wrote: > > The importance of 05afad6 Is there in the commit itself: > > +// > +// One idiom employed by developers is to, during the layout pass, > +// add or remove nodes from the scene. For example, a ScrollPane > +// might add scroll bars to itself if it determines during layout > +// that it needs them, or a ListView might add cells to itself if > +// it determines that it needs to. In such situations we must > +// apply the CSS immediately and not add it to the scene's queue > +// for deferred action. > + > > If you revert 05afad6, you'll break this. > > This is the first time I've become aware of this flickering issue. I'll have > to look at it. > I wonder if the fix for https://bugs.openjdk.java.net/browse/JDK-8193445 has > any impact on this. > >> -Original Message- >> From: openjfx-dev On Behalf Of >> Danny Gonzalez >> Sent: Tuesday, March 3, 2020 11:14 AM >> To: openjfx-dev@openjdk.java.net >> Subject: [EXTERNAL] JDK-8177945 : Single cell selection flickers when adding >> data to TableView >> >> >> There is currently an open bug to do with the issue of selection flickering >> when using single cell selection and when adding data to a TableView. >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs >> .java.com%2Fbugdatabase%2Fview_bug.do%3Fbug_id%3D8177945da >> ta=02%7C01%7Cdavid.grieve%40microsoft.com%7C91a16d9ab05340719f3008 >> d7bf8e3410%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63718848 >> 9816399816sdata=wdPRh3VnN%2BfJw%2BatKOyBi9%2Ba2%2FidMJJb >> 8AwcRXVrfLo%3Dreserved=0 >> >> This bug seems to be low priority because it hasn’t been looked at since I >> submitted it at the start of 2017. >> >> This is quite a major issue for us so I have narrowed down when the issue >> was first introduced. >> >> Here is the changeset: >> >> commit 05afad6b528e871d607b76aea2642cf788b417fe >> Author: David Grieve >> mailto:dgri...@openjdk.org>> >> Date: Tue Apr 15 11:51:38 2014 -0400 >> >>RT-36672: move code to process css during layout back to impl_reapplyCSS, >> which is where it was priort to RT-36559 >> >> >> I can’t search the bug database for this bug ID as I believe it was submitted >> when a previous bug tracking system was in use. >> >> Does anyone have any knowledge as to why this fix was needed and what >> the repercussions would be if I reverted it out for our local OpenJFX build. >> >> Alternatively I would be glad to receive any suggestions of how to fix the >> flickering issue if this changeset is important to leave in. >> >> Thanks >> >> Danny
Re: [Rev 01] RFR: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
On Tue, 3 Mar 2020 13:06:52 GMT, Ambarish Rapte wrote: >> The selection model of ListView stores a strong reference to the most >> recently changed item in >> `SelectedItemsReadOnlyObservableList.itemsListChange`, which causes a leak. >> >> Fix: >> The below member variables and method of class >> SelectedItemsReadOnlyObservableList are not required anymore. >> Variables: itemsList, itemsListChanged, itemsListChange, itemsListListener. >> Method: setItemsList >> >> These members were added when this class was created for >> [JDK-8154216](https://bugs.openjdk.java.net/browse/JDK-8154216). >> But after the fix for >> [JDK-8152396](https://bugs.openjdk.java.net/browse/JDK-8152396), these class >> members are not required. >> >> Verification: >> No failure in existing tests and added a new test. > > The pull request has been updated with 1 additional commit. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/132
RFR: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang
While testing JavaFX using gradle 6.3-nightly and JDK 14, I ran into what
turned out to be a latent test bug in the AppJSCallback* apps that are launched
by ModuleLauncherTest. The launched apps construct a WebView instance, obtain a
WebEngine instance from the WebView, add a state listener to the WebEngine,
load the web content, and then return from the Application::start method. The
WebView instance is held a local variable, and so is subject to garbage
collection once it goes out of scope when the start method returns.
In addition, the test apps did not check for successful launching of the
application or successful loading of the web content, which led to the test
suite hanging (else it would have been a simple test failure).
Note that I don't know (nor care) what changed in JDK 14 to make this more
likely to occur, but since the test itself is demonstrably wrong, it needs to
be fixed.
The main part of the fix was to make the WebEngine an instance variable. In
order to make the test more robust, I also modified it to launch the
application in another thread, and having the main thread check that the
application launched successfully and that the web content was loaded
successfully, after which I did the assertion check for the correct number of
callbacks.
The 5 different apps only differ from each other in the name of the class, the
name of the package from which MyCallback is imported, and possibly the
expected number of callbacks. I diffed AppJSCallbackExported.java against the
other 4 test files and the diffs are essentially the same as they were before
this change. Reviewers thus only need to review one of the tests in detail.
For example, here are the diffs between AppJSCallbackExported and
AppJSCallbackUnexported:
$ diff .../AppJSCallbackExported.java .../AppJSCallbackUnexported.java
37c37
< import myapp5.pkg2.MyCallback;
---
> import myapp5.pkg1.MyCallback;
45c45
< public class AppJSCallbackExported extends Application {
---
> public class AppJSCallbackUnexported extends Application {
77c77
< Util.assertEquals(1, callbackCount);
---
> Util.assertEquals(0, callbackCount);
-
Commits:
- c70d3995: 8240466: AppJSCallback* apps launched by ModuleLauncherTest
intermittently hang
Changes: https://git.openjdk.java.net/jfx/pull/134/files
Webrev: https://webrevs.openjdk.java.net/jfx/134/webrev.00
Issue: https://bugs.openjdk.java.net/browse/JDK-8240466
Stats: 265 lines in 5 files changed: 205 ins; 5 del; 55 mod
Patch: https://git.openjdk.java.net/jfx/pull/134.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/134/head:pull/134
PR: https://git.openjdk.java.net/jfx/pull/134
RE: JDK-8177945 : Single cell selection flickers when adding data to TableView
The importance of 05afad6 Is there in the commit itself: +// +// One idiom employed by developers is to, during the layout pass, +// add or remove nodes from the scene. For example, a ScrollPane +// might add scroll bars to itself if it determines during layout +// that it needs them, or a ListView might add cells to itself if +// it determines that it needs to. In such situations we must +// apply the CSS immediately and not add it to the scene's queue +// for deferred action. + If you revert 05afad6, you'll break this. This is the first time I've become aware of this flickering issue. I'll have to look at it. I wonder if the fix for https://bugs.openjdk.java.net/browse/JDK-8193445 has any impact on this. > -Original Message- > From: openjfx-dev On Behalf Of > Danny Gonzalez > Sent: Tuesday, March 3, 2020 11:14 AM > To: openjfx-dev@openjdk.java.net > Subject: [EXTERNAL] JDK-8177945 : Single cell selection flickers when adding > data to TableView > > > There is currently an open bug to do with the issue of selection flickering > when using single cell selection and when adding data to a TableView. > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs > .java.com%2Fbugdatabase%2Fview_bug.do%3Fbug_id%3D8177945da > ta=02%7C01%7Cdavid.grieve%40microsoft.com%7C91a16d9ab05340719f3008 > d7bf8e3410%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63718848 > 9816399816sdata=wdPRh3VnN%2BfJw%2BatKOyBi9%2Ba2%2FidMJJb > 8AwcRXVrfLo%3Dreserved=0 > > This bug seems to be low priority because it hasn’t been looked at since I > submitted it at the start of 2017. > > This is quite a major issue for us so I have narrowed down when the issue > was first introduced. > > Here is the changeset: > > commit 05afad6b528e871d607b76aea2642cf788b417fe > Author: David Grieve > mailto:dgri...@openjdk.org>> > Date: Tue Apr 15 11:51:38 2014 -0400 > > RT-36672: move code to process css during layout back to impl_reapplyCSS, > which is where it was priort to RT-36559 > > > I can’t search the bug database for this bug ID as I believe it was submitted > when a previous bug tracking system was in use. > > Does anyone have any knowledge as to why this fix was needed and what > the repercussions would be if I reverted it out for our local OpenJFX build. > > Alternatively I would be glad to receive any suggestions of how to fix the > flickering issue if this changeset is important to leave in. > > Thanks > > Danny
RFR: 8240451: JavaFX javadoc build fails with JDK 14
The JDK 14 javadoc will emit a warning for an `@pram` tag of a generic parameter if not surrounded by `<` and `>`. This will in turn fail the build, since we treat warnings as errors. There are 5 classes in JavaFX with this error. The fix is to replace `@param T` with `@param ` in those 5 places. I have tested this fix using javadoc from both JDK 13 and JDK 14. - Commits: - f68f8e69: 8240451: JavaFX javadoc build fails with JDK 14 Changes: https://git.openjdk.java.net/jfx/pull/133/files Webrev: https://webrevs.openjdk.java.net/jfx/133/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8240451 Stats: 6 lines in 5 files changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/133.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/133/head:pull/133 PR: https://git.openjdk.java.net/jfx/pull/133
JDK-8177945 : Single cell selection flickers when adding data to TableView
There is currently an open bug to do with the issue of selection flickering when using single cell selection and when adding data to a TableView. https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8177945 This bug seems to be low priority because it hasn’t been looked at since I submitted it at the start of 2017. This is quite a major issue for us so I have narrowed down when the issue was first introduced. Here is the changeset: commit 05afad6b528e871d607b76aea2642cf788b417fe Author: David Grieve mailto:dgri...@openjdk.org>> Date: Tue Apr 15 11:51:38 2014 -0400 RT-36672: move code to process css during layout back to impl_reapplyCSS, which is where it was priort to RT-36559 I can’t search the bug database for this bug ID as I believe it was submitted when a previous bug tracking system was in use. Does anyone have any knowledge as to why this fix was needed and what the repercussions would be if I reverted it out for our local OpenJFX build. Alternatively I would be glad to receive any suggestions of how to fix the flickering issue if this changeset is important to leave in. Thanks Danny
Re: [Integrated] RFR: 8208761: Update constant collections to use the new immutable collections
Changeset: 960f0390 Author:Nir Lisker Date: 2020-03-03 14:04:34 + URL: https://git.openjdk.java.net/jfx/commit/960f0390 8208761: Update constant collections to use the new immutable collections Reviewed-by: arapte, kcr ! modules/javafx.controls/src/main/java/javafx/scene/control/skin/ColorPickerSkin.java ! modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/builder/ProxyBuilder.java ! modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/ES2Backend.java ! modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/GLSLBackend.java ! modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/HLSLBackend.java ! modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacAccessible.java ! modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ios/IosImageLoader.java ! modules/javafx.graphics/src/main/java/com/sun/javafx/scene/input/InputEventUtils.java ! modules/javafx.graphics/src/main/java/com/sun/prism/impl/PrismSettings.java ! modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinter.java ! modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/prism/PrRenderer.java ! modules/javafx.graphics/src/main/java/javafx/scene/paint/Color.java ! modules/javafx.graphics/src/main/java/javafx/scene/paint/Stop.java ! modules/javafx.web/src/main/java/com/sun/javafx/webkit/KeyCodeMap.java ! modules/javafx.web/src/main/java/com/sun/webkit/network/DateParser.java ! modules/javafx.web/src/main/java/com/sun/webkit/network/URLs.java ! modules/javafx.web/src/main/java/com/sun/webkit/text/TextCodec.java ! modules/javafx.web/src/main/java/javafx/scene/web/WebView.java
Re: RFR: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
On Tue, 3 Mar 2020 12:56:28 GMT, Ambarish Rapte wrote: >> Is it possible to create a test for the leak? > > Hi Kevin, I have updated the PR according to both the comments. Please take a > look. Thanks. I'll put it on my review queue. - PR: https://git.openjdk.java.net/jfx/pull/132
Re: RFR: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
On Mon, 2 Mar 2020 16:47:50 GMT, Kevin Rushforth wrote: >> As I mentioned in the bug report for >> [JDK-8240287](https://bugs.openjdk.java.net/browse/JDK-8240287), I recommend >> using [JDK-8227619](https://bugs.openjdk.java.net/browse/JDK-8227619) as the >> bug ID for this fix (and closing JDK-8240287 in JBS as a duplicate). I think >> that you should be able to just change the title of this PR. > > Is it possible to create a test for the leak? Hi Kevin, I have updated the PR according to both the comments. Please take a look. - PR: https://git.openjdk.java.net/jfx/pull/132
Re: [Rev 01] RFR: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
> The selection model of ListView stores a strong reference to the most > recently changed item in > `SelectedItemsReadOnlyObservableList.itemsListChange`, which causes a leak. > > Fix: > The below member variables and method of class > SelectedItemsReadOnlyObservableList are not required anymore. > Variables: itemsList, itemsListChanged, itemsListChange, itemsListListener. > Method: setItemsList > > These members were added when this class was created for > [JDK-8154216](https://bugs.openjdk.java.net/browse/JDK-8154216). > But after the fix for > [JDK-8152396](https://bugs.openjdk.java.net/browse/JDK-8152396), these class > members are not required. > > Verification: > No failure in existing tests and added a new test. The pull request has been updated with 1 additional commit. - Added commits: - 42b454e0: adding test Changes: - all: https://git.openjdk.java.net/jfx/pull/132/files - new: https://git.openjdk.java.net/jfx/pull/132/files/c859a929..42b454e0 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/132/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/132/webrev.00-01 Stats: 12 lines in 1 file changed: 10 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/132.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/132/head:pull/132 PR: https://git.openjdk.java.net/jfx/pull/132
Re: RFR: 8236651: Simplify and update glass gtk backend
On Thu, 13 Feb 2020 23:56:42 GMT, Kevin Rushforth wrote: >> **Code Changes** >> >> * glass_window.cpp / glass_window.h >> * Removed WindowContextPlug and WindowContextChild (that were used for >> applets / web start) and moved everything to >> one class named WindowContext (since inheritance was no required anymore) >> * Changed set_enabled() to use gtk_widget_set_sensitive instead of >> custom code; >> * Moved to gtk signals instead of gdk events (to use >> gtk_widget_set_sensitive and gtk_grab_add); >> * Frame Extents: Removed the code to request extents and gtk already >> does it by default; >> * Size calculation: Reworked size calculation code. In general, X >> windows are content size instead of whole window size (considering extents - >> frame decorations). OpenJfx uses "whole window size" as window sizes, so it >> requires a "hack" to recalculate sizes when set_bounds() is called with >> window sizes instead of content sizes. The rework was to simplify code paths >> and make it more straightforward. >> * Other Size calculation changes: >> * Use gtk_window_set_default_size() for initial size which is the >> appropriate function; >> * Gravity is now ignored as it is on Windows glass impl; >> * Avoid sending same sizes to Java; >> * Introduced calculate_adjustments() which is a fallback when frame >> extents is not present (it's optional to window managers to implement it); >> * Geometry: Min / Max sizes - reworked it to simplify / Concentrated >> geometry changes on >> apply_geometry(). >> * Mouse grab: Reworked it to use to correct functions according to gtk+ >> version; >> * Draw: Reworked it to use the correct calls accord to gtk+ version >> changes; >> * Fixed JDK-8237491; >> * Moved background code to paint() as gtk3 uses styles to set the >> background and other functions were deprecated; >> * Reorganized function order on glass_window.cpp to match glass_window.h >> >> >> * GlassCursor.cpp >> * Gtk+3 uses a name-like-css approach - so it was properly ported to >> gtk3 way; >> * Reworked Gtk+2 to use a function instead of manual calls to find the >> cursor; >> >> * GtkWindow.java >> * Moved the extents special case to native glass; >> >> * GlassApplication.cpp >> * Removed Gdk events where possible (it's now on glass_window as >> signals); >> * Removed applet/web start code; >> >> * GlassView.cpp >> * Changes to reflect frame extents rework on glass_window >> >> * GlassWindow.cpp >> * WindowContextTop -> WindowContext >> * Removed applet / web start code; >> * Removed frame extents (which is not called anymore due to >> GtkWindow.java change); >> >> * glass_general.cpp >> * Removed functions that became unused; >> * Added is_grab_disabled() that is used on glass_window >> >> * glass_window_ime.cpp >> * WindowContextTop -> WindowContext; >> >> * glass_dnd.cpp / glass_dnd.h >> * Ported to Gtk signals; >> * Use all possible image formats (supported by GdkPixbuf / OpenJfx) - >> .gif is now possible (for ex.); >> * Allow COMPOUND_TEXT; >> * Do not request content while dragging; >> * Reduce overall code size. > > This is going to need further discussion on the mailing list as indicated > above, so it is still premature to review it (i.e., it should still be > considered effectively a "WIP" until that discussion happens). Additionally, > this is a significant and risky change, so I'd like additional eyes on it > when we do get to the point of reviewing it. I have been testing this for several days on ubuntu 18.04 and it's working good. It pass system tests, runs Ensemble 8 and Scenebuilder. Will have to do some tests on 16.04. - PR: https://git.openjdk.java.net/jfx/pull/77
