Dialog Resizing
I just noticed that Dialogs set to be resizable don’t have any constraints on min/max size. So no min/max size properties on Dialog, nor does it limit resizing to within the constraints of the DialogPane. Am I missing something, or shall I file an enhancement request? Scott
Project Proposal: Extending the PNG image loader to handle APNGs
I am new here, but I have contributed to other open source projects before. I have signed the contributor agreement and read through the contributor info provided on the GitHub repository. The GitHub page says to discuss new features here, so here is my proposal. Summary --- I would like to contribute code with the purpose of extending the PNG image loader to handle animated PNGs as specified by the APNG standard. The APNG specification can be found here: https://wiki.mozilla.org/APNG_Specification Goals - Enable JavaFX to be able to load APNGs as animated images. Motivation -- APNG is an extension of the PNG format that allows for animation. APNG images can be used as a higher quality alternative to GIF. The APNG standard is supported by all major modern internet browsers. JavaFX can already load animated GIFs, so additionally supporting animated PNGs makes sense. Description --- To support APNGs, the PNGImageLoader2 and PNGIDATChunkInputStream classes need to be modified so they can process the animation chunks acTL, fcTL, fdAT. Values from the acTL and fcTL chunks can simply be read like the image header chunk. The metadata for the frame can be updated based on these values. The fdAT chunks store data in exactly the same way as the IDAT chunks and can be handled by the PNGIDATChunkInputStream class with little modification. Once a new frame of image data has been decompressed, it will need to be composited with the frame buffer based on the blend OP code. Then, the frame buffer needs to be updated based on the disposal OP code. The composting process can be handled in a similar way to how JavaFX handles GIFs. After compositing, the ImageFrame can be returned. Testing --- Other than unit tests, I am not sure. I would like some advice on what to include for testing. Dependencies This change should not require any additional dependencies. Thanks, -Ethan Hall
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v16]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294589: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/906/files - new: https://git.openjdk.org/jfx/pull/906/files/2a9d2095..bdb8d828 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=906=15 - incr: https://webrevs.openjdk.org/?repo=jfx=906=14-15 Stats: 66 lines in 1 file changed: 17 ins; 18 del; 31 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 21:24:02 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8294589: cleanup
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 374:
>
>> 372:
>> 373: // When the parent window looses focus - menu selection
>> should be cleared
>> 374:
>> sceneListenerHelper.addChangeListener(scene.windowProperty(), true, (sr,
>> oldw, w) -> {
>
> Suggestion:
>
> sceneListenerHelper.addChangeListener(scene.windowProperty(),
> true, w -> {
my version does not create extra object(s).
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 381:
>
>> 379:
>> 380: if (w != null) {
>> 381: windowFocusHelper =
>> sceneListenerHelper.addChangeListener(w.focusedProperty(), true, (s, p,
>> focused) -> {
>
> Suggestion:
>
> windowFocusHelper =
> sceneListenerHelper.addChangeListener(w.focusedProperty(), true, focused -> {
my version does not create extra object(s).
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 387:
>
>> 385: });
>> 386: }
>> 387: });
>
> The goal seems to be to subscribe to changes on scene->window->focused.
>
> This can be done with `flatMap` / `orElse`.
>
> Suggestion:
>
> // When the parent window looses focus - menu selection
> should be cleared
> sceneListenerHelper.addChangeListener(
> scene.windowProperty()
> .flatMap(Window::focusedProperty)
> .orElse(false),
> true,
> focused -> {
> if (!focused) {
> unSelectMenus();
> }
> }
> );
>
>
> You could even do this at the top level I think:
>
> lh.addChangeListener(
> control.sceneProperty()
> .flatMap(Scene::windowProperty)
> .flatMap(Window::focusedProperty)
> .orElse(false),
> true,
> focused -> {
> if (!focused) {
> unSelectMenus();
> }
> }
> );
>
> Also available is to make use of `Subscription`:
>
> Subscription sub =
> Subscription.subscribe(scene.windowProperty().flatMap(Window::focusedProperty).orElse(false),
> focused -> {
> if (!focused) {
> unSelectMenus();
> }
> });
>
> The subscription can then be integrated with `ListenerHelper` again (or even
> more directly if it accepted `Subscription`):
>
> lh.addDisconnectable(sub::unsubscribe);
Good suggestions.
It is basically the existing code - I'd prefer to keep it as is, since
re-writing it might introduce regression.
-
PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
On Fri, 28 Oct 2022 13:54:40 GMT, eduardsdv wrote: >> This fixes a race condition between application and 'Print Job Thread' >> threads when printing. >> >> The race condition occurs when application thread calls `endJob()`, which in >> effect sets the `jobDone` flag to true, >> and when the 'Print Job Thread' thread was in the `synchronized` block in >> `waitForNextPage()` at that time. >> The 'Print Job Thread' thread checks `jobDone` flag after exiting the >> `synchronized` block and, if it is true, skips the last page. >> >> In this fix, not only the `jobDone` is checked, but also that there is no >> other page to be printed. >> It was also needed to introduce a new flag 'jobCanceled', to skip the page >> if the printing was canceled by 'cancelJob()'. > > eduardsdv has updated the pull request incrementally with two additional > commits since the last revision: > > - 8295324: Fix skipping of pages when printing > >This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag >was previously set by the 'endJob()' method. > - 8295324: Adjust the J2DPrinterJobTest > >The test now also checks for the second race condition around 'jobDone' >flag, which is in the print(Graphics, PageFormat, int) method. I think this is OK, but whilst I can see you went to some lengths to create a test case, I do not think it justifies the refactoring you had to do in order to create the test program. So I'd prefer that ALL refactoring be reverted, even if it means no test program. - PR: https://git.openjdk.org/jfx/pull/916
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 21:08:15 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8294589: cleanup
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 293:
>
>> 291:
>> 292: if (scene != null ) {
>> 293: sceneListenerHelper = new
>> ListenerHelper(MenuBarSkin.this);
>
> Why does this (still) need to rely on a weak reference? Skins have a well
> specified life cycle do they not?
You are right: some weak listeners remain, I did not want to re-write the whole
thing for a fear of introducing regression and to keep the changes to a minimum.
The second `ListenerHelper` (line 293) gets disconnected in dispose(), or when
the skin is collected (since the skin may not be explicitly uninstalled, but
instead the whole component or `Pane` might be removed from the scene and
discarded.
-
PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 20:47:33 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8294589: cleanup
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 436:
>
>> 434: if (weakSceneAltKeyEventHandler != null) {
>> 435: t.removeEventHandler(KeyEvent.ANY,
>> weakSceneAltKeyEventHandler);
>> 436: }
>
> So, am I correct that `MenuBarSkin` was badly broken before as it never
> re-registers these weak handlers when the scene changes? It does re-register
> the F10 accelerator, but that's all I can see.
>
> So a scenario where I have a MenuBar, and I move it to another Scene, it
> would basically no longer fully function?
Indeed, there were many, many problems with skins.
Had to create a tester to exercise all these scenarios -
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java
I wonder if I should move it to manual tests.
-
PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 19:08:31 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8294589: cleanup
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 286:
>
>> 284: ParentHelper.setTraversalEngine(getSkinnable(), engine);
>> 285:
>> 286: lh.addChangeListener(control.sceneProperty(), true, (scene) -> {
>
> minor: generally, the parenthesis are omitted for lambda's with one parameter
> (multiple occurences)
I like consistency:
`() ->` use parentheses
`a ->` don't use, why?
`(a,b) ->` use parentheses
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 1180:
>
>> 1178:
>> 1179: private static final CssMetaData ALIGNMENT =
>> 1180: new CssMetaData<>("-fx-alignment", new
>> EnumConverter(Pos.class), Pos.TOP_LEFT ) {
>
> minor: You can use the diamond operator here, probably came from the merge
> conflict
>
> Suggestion:
>
> new CssMetaData<>("-fx-alignment", new
> EnumConverter<>(Pos.class), Pos.TOP_LEFT) {
indeed, thanks!
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java
> line 227:
>
>> 225: ComboBox.class,
>> 226: DatePicker.class,
>> 227: //MenuBar.class,
>
> minor: commented out code
intentionally, to avoid merge conflicts.
-
PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v15]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with two additional commits since the last revision: - 8293119: more integers - 8293119: more integers - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/82a3d920..51294663 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=897=14 - incr: https://webrevs.openjdk.org/?repo=jfx=897=13-14 Stats: 28 lines in 1 file changed: 0 ins; 15 del; 13 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v30]
On Tue, 22 Nov 2022 01:40:02 GMT, Thiago Milczarek Sayao wrote: >> This cleans size and positioning code, reducing special cases, code >> complexity and size. >> >> Changes: >> >> - cached extents: 28, 1, 1, 1 are old defaults - modern gnome uses different >> sizes. It does not assume any size because it varies - it does cache because >> it's unlikely to vary on the same system - but if it does occur, it will >> only waste a resize event. >> - window geometry, min/max size are centralized in >> `update_window_constraints`; >> - Frame extents (the window decoration size used for "total window size"): >> - frame extents are received in `process_property_notify`; >> - removed quirks in java code; >> - When received, call `set_bounds` again to adjust the size (to account >> decorations later received); >> - Removed `activate_window` because it's the same as focusing the window. >> `gtk_window_present` will deiconify and focus it. >> - `ensure_window_size` was a quirk - removed; >> - `requested_bounds` removed - not used anymore; >> - `window_configure` incorporated in `set_bounds` with `gtk_window_move` and >> `gtk_window_resize`; >> - `process_net_wm_property` is a work-around for Unity only (added a check >> if Unity - but it can probably be removed at some point) >> - `restack` split in `to_front()` and `to_back()` to conform to managed code; >> - Set `gtk_window_set_focus_on_map` to FALSE because if TRUE the Window >> Manager changes the window ordering in the "focus stealing" mechanism - this >> makes possible to remove the quirk on `request_focus()`; >> - Note: `geometry_get_*` and `geometry_set_*` moved location but unchanged. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Fix View position One problem I noticed is that when a Stage is shown from a JavaFX applications launched from a terminal, it appears behind the terminal window in the stacking order. With the existing code, a Stage is initially shown above the terminal window. This was also on Ubuntu 16.04, so I'll try it on a more recent version. - PR: https://git.openjdk.org/jfx/pull/915
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v14]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8293119: use integers for rounded values - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/9512f3ca..82a3d920 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=897=13 - incr: https://webrevs.openjdk.org/?repo=jfx=897=12-13 Stats: 68 lines in 1 file changed: 3 ins; 1 del; 64 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v30]
On Tue, 22 Nov 2022 01:40:02 GMT, Thiago Milczarek Sayao wrote: >> This cleans size and positioning code, reducing special cases, code >> complexity and size. >> >> Changes: >> >> - cached extents: 28, 1, 1, 1 are old defaults - modern gnome uses different >> sizes. It does not assume any size because it varies - it does cache because >> it's unlikely to vary on the same system - but if it does occur, it will >> only waste a resize event. >> - window geometry, min/max size are centralized in >> `update_window_constraints`; >> - Frame extents (the window decoration size used for "total window size"): >> - frame extents are received in `process_property_notify`; >> - removed quirks in java code; >> - When received, call `set_bounds` again to adjust the size (to account >> decorations later received); >> - Removed `activate_window` because it's the same as focusing the window. >> `gtk_window_present` will deiconify and focus it. >> - `ensure_window_size` was a quirk - removed; >> - `requested_bounds` removed - not used anymore; >> - `window_configure` incorporated in `set_bounds` with `gtk_window_move` and >> `gtk_window_resize`; >> - `process_net_wm_property` is a work-around for Unity only (added a check >> if Unity - but it can probably be removed at some point) >> - `restack` split in `to_front()` and `to_back()` to conform to managed code; >> - Set `gtk_window_set_focus_on_map` to FALSE because if TRUE the Window >> Manager changes the window ordering in the "focus stealing" mechanism - this >> makes possible to remove the quirk on `request_focus()`; >> - Note: `geometry_get_*` and `geometry_set_*` moved location but unchanged. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Fix View position I'm starting to test this, and have asked a couple others to do the same. I ran the automated tests on Ubuntu 16.04 (yes, I know it's an old system) using the default GTK3 library, and I consistently get 16 unit test failures. Based on the error messages, it looks related to focus. We'll report results on other versions of Linux as they come in. We'll also do some manual testing. __Platform: Ubuntu 16.04__ $ gradle --info -PFULL_TEST=true -PUSE_ROBOT=true sdk :systemTests:test ... SwingNodeScaleTest > initializationError FAILED java.lang.AssertionError: Failed to launch FX application class test.javafx.embed.swing.SwingNodeScaleTest$MyApp within 50 sec. at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.util.Util.launch(Util.java:350) at test.javafx.embed.swing.SwingNodeScaleTest.setupOnce(SwingNodeScaleTest.java:67) RestoreSceneSizeTest > testUnfullscreenSize FAILED java.lang.AssertionError: Scene got wrong width expected:<234.0> but was:<1855.0> at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.failNotEquals(Assert.java:835) at org.junit.Assert.assertEquals(Assert.java:555) at test.javafx.scene.RestoreSceneSizeTest.testUnfullscreenSize(RestoreSceneSizeTest.java:127) CanvasTest > testCanvasRect FAILED java.lang.AssertionError: Timeout when waiting for focus change at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.javafx.scene.web.CanvasTest.testCanvasRect(CanvasTest.java:128) HTMLEditorTest > checkStyleWithCSS FAILED java.lang.AssertionError: Timeout when waiting for focus change at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.javafx.scene.web.HTMLEditorTest.checkStyleWithCSS(HTMLEditorTest.java:222) HTMLEditorTest > selectFontFamilyWithSpace FAILED java.lang.AssertionError: Timeout when waiting for focus change at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.javafx.scene.web.HTMLEditorTest.selectFontFamilyWithSpace(HTMLEditorTest.java:355) HTMLEditorTest > checkFontSizeOnSelectAll_Shift_LeftArrowKey FAILED java.lang.AssertionError: Timeout while waiting for test html text setup at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.javafx.scene.web.HTMLEditorTest.checkFontSizeOnSelectAll_Shift_LeftArrowKey(HTMLEditorTest.java:448) HTMLEditorTest > checkFontSizeOnSelectAll_ctrl_A FAILED java.lang.AssertionError: Timeout while waiting for test html text setup at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.javafx.scene.web.HTMLEditorTest.checkFontSizeOnSelectAll_ctrl_A(HTMLEditorTest.java:398) HTMLEditorTest > checkStyleProperty FAILED java.lang.AssertionError: Timeout when waiting for focus change at org.junit.Assert.fail(Assert.java:89) at
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v13]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8293119: tester - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/6a340d31..9512f3ca Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=897=12 - incr: https://webrevs.openjdk.org/?repo=jfx=897=11-12 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
On Wed, 30 Nov 2022 18:43:42 GMT, Andy Goryachev wrote:
>> Fixed memory leak by using weak listeners and making sure to undo everything
>> that has been done to MenuBar/Skin in dispose().
>>
>> This PR depends on a new internal class ListenerHelper, a replacement for
>> LambdaMultiplePropertyChangeListenerHandler.
>> See https://github.com/openjdk/jfx/pull/908
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8294589: cleanup
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 286:
> 284: ParentHelper.setTraversalEngine(getSkinnable(), engine);
> 285:
> 286: lh.addChangeListener(control.sceneProperty(), true, (scene) -> {
minor: generally, the parenthesis are omitted for lambda's with one parameter
(multiple occurences)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 293:
> 291:
> 292: if (scene != null ) {
> 293: sceneListenerHelper = new
> ListenerHelper(MenuBarSkin.this);
Why does this (still) need to rely on a weak reference? Skins have a well
specified life cycle do they not?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 360:
> 358: break;
> 359: default:
> 360: break;
minor: indent is incorrect (also in original)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 374:
> 372:
> 373: // When the parent window looses focus - menu selection
> should be cleared
> 374:
> sceneListenerHelper.addChangeListener(scene.windowProperty(), true, (sr,
> oldw, w) -> {
Suggestion:
sceneListenerHelper.addChangeListener(scene.windowProperty(),
true, w -> {
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 381:
> 379:
> 380: if (w != null) {
> 381: windowFocusHelper =
> sceneListenerHelper.addChangeListener(w.focusedProperty(), true, (s, p,
> focused) -> {
Suggestion:
windowFocusHelper =
sceneListenerHelper.addChangeListener(w.focusedProperty(), true, focused -> {
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 387:
> 385: });
> 386: }
> 387: });
The goal seems to be to subscribe to changes on scene->window->focused.
This can be done with `flatMap` / `orElse`.
Suggestion:
// When the parent window looses focus - menu selection should
be cleared
sceneListenerHelper.addChangeListener(
scene.windowProperty()
.flatMap(Window::focusedProperty)
.orElse(false),
true,
focused -> {
if (!focused) {
unSelectMenus();
}
}
);
You could even do this at the top level I think:
lh.addChangeListener(
control.sceneProperty()
.flatMap(Scene::windowProperty)
.flatMap(Window::focusedProperty)
.orElse(false),
true,
focused -> {
if (!focused) {
unSelectMenus();
}
}
);
Also available is to make use of `Subscription`:
Subscription sub =
Subscription.subscribe(scene.windowProperty().flatMap(Window::focusedProperty).orElse(false),
focused -> {
if (!focused) {
unSelectMenus();
}
});
The subscription can then be integrated with `ListenerHelper` again (or even
more directly if it accepted `Subscription`):
lh.addDisconnectable(sub::unsubscribe);
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 436:
> 434: if (weakSceneAltKeyEventHandler != null) {
> 435: t.removeEventHandler(KeyEvent.ANY,
> weakSceneAltKeyEventHandler);
> 436: }
So, am I correct that `MenuBarSkin` was badly broken before as it never
re-registers these weak handlers when the scene changes? It does re-register
the F10 accelerator, but that's all I can see.
So a scenario where I have a MenuBar, and I move it to another Scene, it would
basically no longer fully function?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 1180:
> 1178:
> 1179: private static final CssMetaData ALIGNMENT =
> 1180: new CssMetaData<>("-fx-alignment", new
> EnumConverter(Pos.class), Pos.TOP_LEFT ) {
minor: You can use the diamond operator here, probably came from the merge
conflict
Suggestion:
new CssMetaData<>("-fx-alignment", new EnumConverter<>(Pos.class),
Re: RFR: 8295426: MenuButtonSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 17:00:55 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 170) and a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Also applies to SplitMenuButton, since they share the same base class >> MenuButtonSkinBase. >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or >> SPLIT_MENU_BUTTON >> >> In addition, there seems to be another failure scenario when simply >> replacing the skin - no menu is shown upon a click. To reproduce, launch >> LeakTest and click once on the [Replace Skin] button. Second click restores >> the function. >> >> caused by: >> - adding and not removing EventHandlers >> - setting and not clearing onAction handlers >> - incorrect logic in setting mousePressed/mouse/Released handlers > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 37 commits: > > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - 8295426: listener helper update > - Merge remote-tracking branch 'origin/8294809.listener.helper' into > 8295426.menu.button.skin > - 8294809: review comments > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - 8294809: whitespace > - 8294809: no public api > - 8294809: map change listener > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - ... and 27 more: https://git.openjdk.org/jfx/compare/0a785ae0...800d3f1e @aghaisas : Could you please review this next? There is another PR that touches the same area, #937. Thanks! - PR: https://git.openjdk.org/jfx/pull/919
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v12]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8293119: descriptions - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/7d290b65..6a340d31 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=897=11 - incr: https://webrevs.openjdk.org/?repo=jfx=897=10-11 Stats: 30 lines in 2 files changed: 29 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v11]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8293119: all columns - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/6cc8b33b..7d290b65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=897=10 - incr: https://webrevs.openjdk.org/?repo=jfx=897=09-10 Stats: 38 lines in 1 file changed: 1 ins; 25 del; 12 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed [v2]
On Mon, 7 Nov 2022 12:03:38 GMT, Dean Wookey wrote: >> When menu buttons are added and removed from the scene, an accelerator >> change listener is added to each menu item in the menu. There is nothing >> stopping the same change listener being added multiple times. >> >> MenuButtonSkinBase calls the >> ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), >> getSkinnable()); method each time the button is added to the scene, but >> that method itself also registers a listener to call itself. Each time the >> button is added to the scene, the method is called at least twice. >> >> When it's removed from the scene, the remove accelerator method is also >> called twice, but only the first call removes a change listener attached to >> the accelerator because the first call removes the entry from the hashmap >> changeListenerMap. The second call finds nothing in the map, and doesn't >> remove the additional instance. >> >> This pull request just removes the redundant code in the MenuButtonSkinBase. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added changing scene tests for accelerator change listeners Probably better to just wait then. - PR: https://git.openjdk.org/jfx/pull/937
Re: RFR: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed [v2]
On Wed, 30 Nov 2022 19:32:37 GMT, Kevin Rushforth wrote: > Doing this makes it harder for reviewers to see what is actually being > changed. This is true, but it will the actual review process (especially, testing) move faster. Or, we can wait for #919 to get integrated first. - PR: https://git.openjdk.org/jfx/pull/937
Re: RFR: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed [v2]
On Wed, 30 Nov 2022 19:10:20 GMT, Andy Goryachev wrote: > base on top of https://github.com/openjdk/jfx/pull/919 Doing this makes it harder for reviewers to see what is actually being changed. Unless / until we enable the Skara support for dependent PRs (which the `jdk` repo enables, but so far we haven't), I would probably just recommend waiting on the final review until after the dependent PR, #919 in this case, is integrated and then merge master at that time. - PR: https://git.openjdk.org/jfx/pull/937
Re: RFR: 8187145: (Tree)TableView with null selectionModel: throws NPE on sorting [v12]
> Setting a null selection model in TableView and TreeTableView produce NPE on > sorting (and probably in some other situations) because the check for null is > missing in several places. > > Setting a null selection model is a valid way to disable selection in a > (tree)table. > > There is also a similar issue with ListView > [JDK-8279640](https://bugs.openjdk.org/browse/JDK-8279640). > > changes: > - added check for null selection model where it was missing > - clear focused row index on setting null selection model in TreeTableView > - clear selected cells on setting a new selection model Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - 8187145: cleanup - 8187145: also tree table view - 8187145: whitespace - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - 8187145: review comments - 8187145: github - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - 8187145: added tests - ... and 9 more: https://git.openjdk.org/jfx/compare/0a785ae0...03409ea5 - Changes: https://git.openjdk.org/jfx/pull/876/files Webrev: https://webrevs.openjdk.org/?repo=jfx=876=11 Stats: 474 lines in 15 files changed: 378 ins; 12 del; 84 mod Patch: https://git.openjdk.org/jfx/pull/876.diff Fetch: git fetch https://git.openjdk.org/jfx pull/876/head:pull/876 PR: https://git.openjdk.org/jfx/pull/876
Re: RFR: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed
On Tue, 8 Nov 2022 10:11:40 GMT, Dean Wookey wrote: >> Reviewers: @arapte @andy-goryachev-oracle > > @kevinrushforth, @arapte I clicked the request review on andy and it removed > arapte for some reason. I guess that button shouldn't be clicked. Oops. @DeanWookey : Would it be possible to modify this PR to use the alternative solution https://github.com/openjdk/jfx/commit/80971d89c46f3f74cb8584d4907cc6818809e2f6 ? I think the code in this PR as it stands right now removes a bit of functionality that is needed, while the alternative does not. Also, would it be possible to base your changes on top of https://github.com/openjdk/jfx/pull/919 since the skin got reworked to eliminate a memory leak, and it might be easier to base your changes on top of that rather than try to resolve non-trivial merge conflicts. What do you think? - PR: https://git.openjdk.org/jfx/pull/937
Re: RFR: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed [v2]
On Mon, 7 Nov 2022 12:03:38 GMT, Dean Wookey wrote: >> When menu buttons are added and removed from the scene, an accelerator >> change listener is added to each menu item in the menu. There is nothing >> stopping the same change listener being added multiple times. >> >> MenuButtonSkinBase calls the >> ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), >> getSkinnable()); method each time the button is added to the scene, but >> that method itself also registers a listener to call itself. Each time the >> button is added to the scene, the method is called at least twice. >> >> When it's removed from the scene, the remove accelerator method is also >> called twice, but only the first call removes a change listener attached to >> the accelerator because the first call removes the entry from the hashmap >> changeListenerMap. The second call finds nothing in the map, and doesn't >> remove the additional instance. >> >> This pull request just removes the redundant code in the MenuButtonSkinBase. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added changing scene tests for accelerator change listeners change requested: - use alternative code - base on top of https://github.com/openjdk/jfx/pull/919 - Changes requested by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/937
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v3]
On Thu, 13 Oct 2022 04:25:19 GMT, Alexander Matveev wrote: >> - Added support for JAR and JRT protocol to AVFoundation platform. >> - Removed H.264/MP3 and AAC support from GStreamer platform, which was >> primary used to playback these formats for JAR and JRT protocols. >> - Added ability to FXMediaPlayer sample to generate playlist for JAR and >> JRT protocols for testing. See FXMedia.java for how to use it. >> - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. >> Before it did not work because GStreamer platform did not support H.265/HEVC >> and AVFoundation did not support JAR/JRT protocols. >> - Minor code clean up. >> >> After this changes: >> GSTPlatform: AIFF and WAV for all protocols. >> AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols. >> >> This change is transparent for end user and does not affect list of >> supported formats by JavaFX Media. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8287822: [macos] Remove support of duplicated formats from macOS [v3] all my comments got explained and/or resolved. thank you! - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294589: cleanup - Changes: - all: https://git.openjdk.org/jfx/pull/906/files - new: https://git.openjdk.org/jfx/pull/906/files/45f3e374..2a9d2095 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=906=14 - incr: https://webrevs.openjdk.org/?repo=jfx=906=13-14 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v14]
On Wed, 30 Nov 2022 18:36:13 GMT, Andy Goryachev wrote: >> Fixed memory leak by using weak listeners and making sure to undo everything >> that has been done to MenuBar/Skin in dispose(). >> >> This PR depends on a new internal class ListenerHelper, a replacement for >> LambdaMultiplePropertyChangeListenerHandler. >> See https://github.com/openjdk/jfx/pull/908 > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 46 commits: > > - Merge remote-tracking branch 'origin/master' into >8294589.menubarskin.leak > - 8294589: testing github merge-conflict label behavior > - 8294589: cleanup > - Merge remote-tracking branch 'origin/master' into >8294589.menubarskin.leak > - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak > - 8294809: generics > - 8294589: owner > - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak > - 8294809: is alive > - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak > - ... and 36 more: https://git.openjdk.org/jfx/compare/0a785ae0...45f3e374 @hjohn : could you please take a look at this, since it had a non-trivial merge? thanks! - PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v14]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits: - Merge remote-tracking branch 'origin/master' into 8294589.menubarskin.leak - 8294589: testing github merge-conflict label behavior - 8294589: cleanup - Merge remote-tracking branch 'origin/master' into 8294589.menubarskin.leak - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak - 8294809: generics - 8294589: owner - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak - 8294809: is alive - Merge branch '8294809.listener.helper' into 8294589.menubarskin.leak - ... and 36 more: https://git.openjdk.org/jfx/compare/0a785ae0...45f3e374 - Changes: https://git.openjdk.org/jfx/pull/906/files Webrev: https://webrevs.openjdk.org/?repo=jfx=906=13 Stats: 465 lines in 2 files changed: 235 ins; 198 del; 32 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8294589: MenuBarSkin: memory leak when changing skin [v13]
> Fixed memory leak by using weak listeners and making sure to undo everything > that has been done to MenuBar/Skin in dispose(). > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294589: testing github merge-conflict label behavior - Changes: - all: https://git.openjdk.org/jfx/pull/906/files - new: https://git.openjdk.org/jfx/pull/906/files/405b14b7..e1b7ea65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=906=12 - incr: https://webrevs.openjdk.org/?repo=jfx=906=11-12 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/906.diff Fetch: git fetch https://git.openjdk.org/jfx pull/906/head:pull/906 PR: https://git.openjdk.org/jfx/pull/906
Re: RFR: 8295806: TableViewSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 179) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > caused by: > - adding and not removing listeners > - adding and not removing event handlers/filters > > NOTE: > this fix requires [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) > ListenerHelper Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits: - Merge remote-tracking branch 'origin/master' into 8295806.table.view.skin - Merge remote-tracking branch 'origin/master' into 8295806.table.view.skin - 8295806: placeholder - 8295806: plugged the leak - Merge remote-tracking branch 'origin/8294809.listener.helper' into 8295806.table.view.skin - 8294809: map change listener - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper - Merge remote-tracking branch 'origin/master' into 8295806.table.view.skin - 8294809: generics - 8294809: is alive - ... and 16 more: https://git.openjdk.org/jfx/compare/0a785ae0...8c5f4df9 - Changes: https://git.openjdk.org/jfx/pull/929/files Webrev: https://webrevs.openjdk.org/?repo=jfx=929=01 Stats: 200 lines in 7 files changed: 95 ins; 57 del; 48 mod Patch: https://git.openjdk.org/jfx/pull/929.diff Fetch: git fetch https://git.openjdk.org/jfx pull/929/head:pull/929 PR: https://git.openjdk.org/jfx/pull/929
Re: RFR: 8245145: Spinner: throws IllegalArgumentException when replacing skin [v2]
> Fixed SpinnerSkin initialization using new Skin.install(). Also in this PR - > fixing memory leaks in SpinnerSkin by properly removing all listeners and > event filters added in the constructor, as well as any child Nodes. > > NOTE: the fix requires both ListenerHelper > [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) and Skin.install() > [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) changes. Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 50 commits: - Merge remote-tracking branch 'origin/master' into 8245145.spinner.skin - 8245145: cleanup - 8245145: cleanup - 8245145: cleanup - Merge remote-tracking branch 'origin/master' into 8245145.spinner.skin - 8245145: listener helper - Merge branch '8294809.listener.helper' into 8245145.spinner.skin - 8294809: whitespace - 8294809: no public api - 8294809: map change listener - ... and 40 more: https://git.openjdk.org/jfx/compare/0a785ae0...ac984b3d - Changes: https://git.openjdk.org/jfx/pull/904/files Webrev: https://webrevs.openjdk.org/?repo=jfx=904=01 Stats: 58 lines in 3 files changed: 33 ins; 12 del; 13 mod Patch: https://git.openjdk.org/jfx/pull/904.diff Fetch: git fetch https://git.openjdk.org/jfx pull/904/head:pull/904 PR: https://git.openjdk.org/jfx/pull/904
Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 165) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.BUTTON_BAR; > > > caused by: > - adding and not removing listeners > - adding and not removing children Nodes Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin - 8294809: generics - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin - 8294809: is alive - Revert "8294809: removed weak listeners support" This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. - 8295506: button bar skin - 8294809: removed weak listeners support - 8294809: use weak reference correctly this time - ... and 13 more: https://git.openjdk.org/jfx/compare/0a785ae0...759ecaf4 - Changes: https://git.openjdk.org/jfx/pull/921/files Webrev: https://webrevs.openjdk.org/?repo=jfx=921=01 Stats: 24 lines in 2 files changed: 17 ins; 2 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/921.diff Fetch: git fetch https://git.openjdk.org/jfx pull/921/head:pull/921 PR: https://git.openjdk.org/jfx/pull/921
Re: RFR: 8295175: SplitPaneSkin: memory leak when changing skin [v2]
> Fixed memory leak by removing all the listeners in dispose(); > > This PR depends on a new internal class ListenerHelper, a replacement for > LambdaMultiplePropertyChangeListenerHandler. > See https://github.com/openjdk/jfx/pull/908 Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 27 commits: - Merge remote-tracking branch 'origin/master' into 8295175.splitpaneskin.with.helper - Merge remote-tracking branch 'origin/master' into 8295175.splitpaneskin.with.helper - Merge branch '8294809.listener.helper' into 8295175.splitpaneskin.with.helper - 8294809: generics - Merge branch '8294809.listener.helper' into 8295175.splitpaneskin.with.helper - 8294809: is alive - Merge branch '8294809.listener.helper' into 8295175.splitpaneskin.with.helper - Revert "8294809: removed weak listeners support" This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. - Merge branch '8294809.listener.helper' into 8295175.splitpaneskin.with.helper - 8294809: removed weak listeners support - ... and 17 more: https://git.openjdk.org/jfx/compare/0a785ae0...88e1da58 - Changes: https://git.openjdk.org/jfx/pull/911/files Webrev: https://webrevs.openjdk.org/?repo=jfx=911=01 Stats: 70 lines in 2 files changed: 31 ins; 18 del; 21 mod Patch: https://git.openjdk.org/jfx/pull/911.diff Fetch: git fetch https://git.openjdk.org/jfx pull/911/head:pull/911 PR: https://git.openjdk.org/jfx/pull/911
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v10]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 59 commits: - Merge remote-tracking branch 'origin/master' into 8293119.constrained - 8293119: cleanup - 8293119: subsequent columns - 8293119: review comments - Merge remote-tracking branch 'origin/master' into 8293119.constrained - 8293119: no link - 8293119: review comments - Merge remote-tracking branch 'origin/master' into 8293119.constrained - 8293119: moved to com.sun - 8293119: newline - ... and 49 more: https://git.openjdk.org/jfx/compare/0a785ae0...6cc8b33b - Changes: https://git.openjdk.org/jfx/pull/897/files Webrev: https://webrevs.openjdk.org/?repo=jfx=897=09 Stats: 2263 lines in 14 files changed: 2003 ins; 250 del; 10 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8295426: MenuButtonSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 170) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Also applies to SplitMenuButton, since they share the same base class > MenuButtonSkinBase. > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or > SPLIT_MENU_BUTTON > > In addition, there seems to be another failure scenario when simply replacing > the skin - no menu is shown upon a click. To reproduce, launch LeakTest and > click once on the [Replace Skin] button. Second click restores the function. > > caused by: > - adding and not removing EventHandlers > - setting and not clearing onAction handlers > - incorrect logic in setting mousePressed/mouse/Released handlers Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 37 commits: - Merge remote-tracking branch 'origin/master' into 8295426.menu.button.skin - Merge remote-tracking branch 'origin/master' into 8295426.menu.button.skin - 8295426: listener helper update - Merge remote-tracking branch 'origin/8294809.listener.helper' into 8295426.menu.button.skin - 8294809: review comments - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper - 8294809: whitespace - 8294809: no public api - 8294809: map change listener - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper - ... and 27 more: https://git.openjdk.org/jfx/compare/0a785ae0...800d3f1e - Changes: https://git.openjdk.org/jfx/pull/919/files Webrev: https://webrevs.openjdk.org/?repo=jfx=919=01 Stats: 175 lines in 4 files changed: 109 ins; 32 del; 34 mod Patch: https://git.openjdk.org/jfx/pull/919.diff Fetch: git fetch https://git.openjdk.org/jfx pull/919/head:pull/919 PR: https://git.openjdk.org/jfx/pull/919
Re: RFR: 8295531: ComboBoxBaseSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 166) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Affected skins: > - ColorPicker > - DatePicker > - ComboBox > > caused by: > - out-of-order modification of the control property (skin.install) > - adding skin nodes and not removing them in dispose() > - adding listeners and not removing them in dispose() > > NOTE: the fix will require not only ListenerHelper > [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) but also > Skin.install() [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) > changes. Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 53 commits: - Merge remote-tracking branch 'origin/master' into 8295531.color.picker.skin - Merge remote-tracking branch 'origin/master' into 8295531.color.picker.skin - Merge remote-tracking branch 'origin/master' into 8295531.color.picker.skin - 8295531: whitespace - 8295531: selection model leak - Merge branch '8294809.listener.helper' into 8295531.color.picker.skin - 8294809: generics - Merge branch '8294809.listener.helper' into 8295531.color.picker.skin - 8294809: is alive - Revert "8294809: removed weak listeners support" This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. - ... and 43 more: https://git.openjdk.org/jfx/compare/0a785ae0...f391f41a - Changes: https://git.openjdk.org/jfx/pull/922/files Webrev: https://webrevs.openjdk.org/?repo=jfx=922=01 Stats: 185 lines in 6 files changed: 82 ins; 41 del; 62 mod Patch: https://git.openjdk.org/jfx/pull/922.diff Fetch: git fetch https://git.openjdk.org/jfx pull/922/head:pull/922 PR: https://git.openjdk.org/jfx/pull/922
Re: RFR: 8295500: AccordionSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 164) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.ACCORDION; > > > caused by: > - adding and not removing listeners Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 27 commits: - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin - 8295500: cleanup - Merge remote-tracking branch 'origin/master' into 8295500.accordion.skin - Merge branch '8294809.listener.helper' into 8295500.accordion.skin - 8294809: generics - Merge branch '8294809.listener.helper' into 8295500.accordion.skin - 8294809: is alive - Merge branch '8294809.listener.helper' into 8295500.accordion.skin - Revert "8294809: removed weak listeners support" This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. - 8295500: cleanup - ... and 17 more: https://git.openjdk.org/jfx/compare/0a785ae0...218c6ea9 - Changes: https://git.openjdk.org/jfx/pull/920/files Webrev: https://webrevs.openjdk.org/?repo=jfx=920=01 Stats: 31 lines in 2 files changed: 16 ins; 6 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/920.diff Fetch: git fetch https://git.openjdk.org/jfx pull/920/head:pull/920 PR: https://git.openjdk.org/jfx/pull/920
Integrated: 8295754: PaginationSkin: memory leak when changing skin
On Thu, 20 Oct 2022 20:48:07 GMT, Andy Goryachev wrote: > Fixes memory leaks as determined by SkinMemoryLeakTest (remove line 171) and > a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.PAGINATION; > > Found another issue: Pagination class does not survive replacing its skin > (all components disappear). > > caused by: > - adding and not removing listeners > - adding and not removing children Nodes > - setting control's properties in the constructor > - incorrectly setting a clip rectangle > > NOTE: the fix will requires both ListenerHelper > [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) and Skin.install() > [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) changes. This pull request has now been integrated. Changeset: 0a785ae0 Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/0a785ae036f48c736b65df865a3b93f954d46fe5 Stats: 128 lines in 2 files changed: 56 ins; 44 del; 28 mod 8295754: PaginationSkin: memory leak when changing skin Reviewed-by: kcr, aghaisas - PR: https://git.openjdk.org/jfx/pull/925
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Tue, 22 Nov 2022 21:28:44 GMT, Kevin Rushforth wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix indentation
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/UploadingPainter.java
> line 41:
>
>> 39: * The PresentingPainter is used when we are rendering to the main
>> screen.
>> 40: */
>> 41: final class UploadingPainter extends ViewPainter {
>
> I'm not sure I see the value in making this change.
Since this is an internal class (not public API), I won't object to this
change. I do think it's a rather pointless change, but I'll leave it up to you.
-
PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Mon, 28 Nov 2022 13:39:05 GMT, John Hendrikx wrote: >> - Remove unsupported/unnecessary SuppressWarning annotations >> - Remove reduntant type specifications (use diamond operator) >> - Remove unused or duplicate imports >> - Remove unnecessary casts (type is already correct type or can be autoboxed) >> - Remove unnecessary semi-colons (at end of class definitions, or just >> repeated ones) >> - Remove redundant super interfaces (interface that is already inherited) >> - Remove unused type parameters >> - Remove declared checked exceptions that are never thrown >> - Add missing `@Override` annotations > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix indentation Additional inline comments. - PR: https://git.openjdk.org/jfx/pull/960
Re: RFR: JDK-8297413: Remove easy warnings in javafx.graphics [v2]
On Wed, 30 Nov 2022 03:19:54 GMT, Nir Lisker wrote: >> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java >> line 418: >> >>> 416: scaleFactor = 1.0 / scaleDivider; >>> 417: adjw = (int)Math.round(iw / scaleDivider); >>> 418: adjh = (int)Math.round(ih / scaleDivider); >> >> Same comment here about the old code being clearer. > > `scaleDivider` is defined just 2 lines above as a `double`. I don't see how > the cast helps here. Yes, this one is fine. >> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/WindowStage.java >> line 422: >> >>> 420: } >>> 421: double similarity = (width - adjw) / (double) width + >>> 422: (height - adjh) / (double) height + //Large >>> padding is bad >> >> This change _must_ be reverted. There are cases where doing integer >> arithmetic on intermediate results is not equivalent to doing double >> arithmetic. >> >> Consider this: >> >> >> int i = Integer.MAX_VALUE; >> int j = -1; >> double d1 = (double) i - (double) j; >> double d2 = i - j; >> >> >> `d1` will be `2147483648` and `d2` will be `-2147483648`. >> >> I can't say whether it is possible in practice, but this change is not >> acceptable, and is a good example of the general concern I raised. > > If the casts in the numerator actually matter, then the cast in the > denominator can be removed. The latter are the ones that Eclipse flags for me > as unnecessary. I still think that any change here would be a very low value change. If done incorrectly, as it was in the initial attempt, it can introduce bugs. Even if done correctly, I see no point in it. - PR: https://git.openjdk.org/jfx/pull/960
Integrated: JDK-8297414: Remove easy warnings in javafx.controls
On Tue, 22 Nov 2022 12:08:44 GMT, John Hendrikx wrote:
> Note: I ran into a `javac` compiler bug while replacing types with diamond
> operators (ecj has no issues). I had two options, add a
> `SuppressWarnings("unused")` or to use a lambda instead of a method reference
> to make `javac` happy. I choose the later and added a comment so it can be
> fixed once the bug is fixed. I've reported the issue here:
> https://bugs.openjdk.org/browse/JDK-8297428
>
> - Remove unsupported/unnecessary SuppressWarning annotations
> - Remove reduntant type specifications (use diamond operator)
> - Remove unused or duplicate imports
> - Remove unnecessary casts (type is already correct type or can be autoboxed)
> - Remove unnecessary semi-colons (at end of class definitions, or just
> repeated ones)
> - Remove redundant super interfaces (interface that is already inherited)
> - Remove unused type parameters
> - Remove declared checked exceptions that are never thrown
> - Add missing `@Override` annotations
This pull request has now been integrated.
Changeset: 2fa9f4b9
Author:John Hendrikx
Committer: Nir Lisker
URL:
https://git.openjdk.org/jfx/commit/2fa9f4b9e53a43de0676631ab5d93334d2a3b2dd
Stats: 2290 lines in 319 files changed: 159 ins; 303 del; 1828 mod
8297414: Remove easy warnings in javafx.controls
Reviewed-by: angorya, kcr, nlisker
-
PR: https://git.openjdk.org/jfx/pull/959
Re: RFR: 8295754: PaginationSkin: memory leak when changing skin
On Thu, 20 Oct 2022 20:48:07 GMT, Andy Goryachev wrote: > Fixes memory leaks as determined by SkinMemoryLeakTest (remove line 171) and > a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.PAGINATION; > > Found another issue: Pagination class does not survive replacing its skin > (all components disappear). > > caused by: > - adding and not removing listeners > - adding and not removing children Nodes > - setting control's properties in the constructor > - incorrectly setting a clip rectangle > > NOTE: the fix will requires both ListenerHelper > [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) and Skin.install() > [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) changes. Fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/925
Integrated: JDK-8297412: Remove easy warnings in javafx.fxml, javafx.media, javafx.swing, javafx.swt and javafx.web
On Tue, 22 Nov 2022 11:13:40 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations This pull request has now been integrated. Changeset: 7cb408bd Author:John Hendrikx Committer: Nir Lisker URL: https://git.openjdk.org/jfx/commit/7cb408bdbf2603738e35166b16da4c181d3dedef Stats: 683 lines in 145 files changed: 216 ins; 107 del; 360 mod 8297412: Remove easy warnings in javafx.fxml, javafx.media, javafx.swing, javafx.swt and javafx.web Reviewed-by: angorya, nlisker - PR: https://git.openjdk.org/jfx/pull/958
