Re: RFR: 8273096: Add support for H.265/HEVC to JavaFX Media [v2]
On Tue, 16 Nov 2021 02:24:11 GMT, Alexander Matveev wrote: >> - Added support for H.265/HEVC for all 3 platforms. >> - Support is added only for .mp4 files over FILE/HTTP/HTTPS protocols. HTTP >> Live Streaming with H.265/HEVC is not supported. >> - On Windows mfwrapper was introduced which uses Media Foundation APIs to >> decode HEVC. >> - 10 and 12-bit HEVC was tested and also supported, however due to graphics >> pipeline not supporting 10-bit YUV rendering we will use color converter to >> convert video frame to 8-bit before sending it for rendering. >> - Resolution upto 4k is supported. >> >> Additional runtime dependency requirements: >> Windows: Windows 10 with HEVC Video Extensions installed. >> macOS: macOS High Sierra and later >> Linux: at least libavcodec56 and libswscale5 >> >> Additional build dependency: >> Linux: libswscale-dev > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8273096: Add support for H.265/HEVC to JavaFX Media [v3] Added new patch. libswscale will be loaded dynamically on Linux for H.265/HEVC 10/12-bit. Playback will not fail for other formats if it is not present. Windows should play other formats without "HEVC Video Extensions" without any issues. Same for macOS. Following MediaException will be thrown on Linux if libswscale is not present. Output is from FXMediaPlayer. onError: MediaException: PLAYBACK_ERROR : [com.sun.media.jfxmediaimpl.platform.gstreamer.GSTMediaPlayer@a88d9ab] ERROR_MISSING_LIBSWSCALE: ERROR_MISSING_LIBSWSCALE - PR: https://git.openjdk.java.net/jfx/pull/649
Re: RFR: 8273096: Add support for H.265/HEVC to JavaFX Media [v2]
> - Added support for H.265/HEVC for all 3 platforms. > - Support is added only for .mp4 files over FILE/HTTP/HTTPS protocols. HTTP > Live Streaming with H.265/HEVC is not supported. > - On Windows mfwrapper was introduced which uses Media Foundation APIs to > decode HEVC. > - 10 and 12-bit HEVC was tested and also supported, however due to graphics > pipeline not supporting 10-bit YUV rendering we will use color converter to > convert video frame to 8-bit before sending it for rendering. > - Resolution upto 4k is supported. > > Additional runtime dependency requirements: > Windows: Windows 10 with HEVC Video Extensions installed. > macOS: macOS High Sierra and later > Linux: at least libavcodec56 and libswscale5 > > Additional build dependency: > Linux: libswscale-dev Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8273096: Add support for H.265/HEVC to JavaFX Media [v3] - Changes: - all: https://git.openjdk.java.net/jfx/pull/649/files - new: https://git.openjdk.java.net/jfx/pull/649/files/62ba1ec8..6cb1ac5c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=649&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=649&range=00-01 Stats: 134 lines in 6 files changed: 113 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jfx/pull/649.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/649/head:pull/649 PR: https://git.openjdk.java.net/jfx/pull/649
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v6]
> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests > still work. Also added a single JUnit 5 tests, and confirmed it works. > > I've updated the Eclipse project file for the base module only. John Hendrikx has updated the pull request incrementally with two additional commits since the last revision: - Fix white space error - Allow apiguardian as dependency - Changes: - all: https://git.openjdk.java.net/jfx/pull/633/files - new: https://git.openjdk.java.net/jfx/pull/633/files/7d56bcd0..c5b76ee8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=633&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=633&range=04-05 Stats: 15 lines in 2 files changed: 8 ins; 3 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/633.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/633/head:pull/633 PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Fri, 24 Sep 2021 20:45:23 GMT, Kevin Rushforth wrote: > As mentioned in JBS, any new third-party libraries require prior third-party > license approval. And we will need to work with you on sponsoring this (as > you can't contribute any third-party code under the OCA). > > Speaking of which, there are more libraries added to > `gradle-verification.xml` than I would expect. Each one will need third-party > approval, if they are required. Since this is for internal use (build / test > only) and not something we redistribute, that makes it easier, although it > still depends on the license for each piece. > > Finally, we have some closed tests that we need to ensure aren't impacted by > this, so that's one more area we need to coordinate. > > We can take a look, but it won't be right away. Can you also start a thread > on openjfx-dev? I'd like to gauge the level of interest in enabling JUnit 5 > for new tests. Github is showing this as a "requested change" but I can't resolve this. @kevinrushforth anything you can do on your end? All the issues in this comment were addresses AFAIK. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Thu, 11 Nov 2021 15:11:32 GMT, Kevin Rushforth wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Readd junit declaration in allprojects and set junit version to 4.13.2
>
> build.gradle line 1953:
>
>> 1951: testRuntimeOnly(group: "org.junit.vintage", name:
>> "junit-vintage-engine", version: "5.8.1") {
>> 1952: exclude(group: "org.apiguardian", module:
>> "apiguardian-api")
>> 1953: }
>
> As it turns out, `apiguardian` no longer needs to be blocked, so you can
> restore it if it is easier.
Okay, I'll remove the exclusion on apiguardian then as it would just be
confusing why it was excluded in the first place. Hamcrest was already
included as JUnit 5 wouldn't work without it.
-
PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Sat, 25 Sep 2021 13:55:15 GMT, John Hendrikx wrote: >> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests >> still work. Also added a single JUnit 5 tests, and confirmed it works. >> >> I've updated the Eclipse project file for the base module only. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Readd junit declaration in allprojects and set junit version to 4.13.2 These are good points regarding test migration. I withdraw the suggestion of test segregation. I would not want to see a wholesale migration of all tests to JUnit5, but I do like the idea of migrating on a test-by-test basis if new tests are being added to an existing test class where using JUnit 5 would be a benefit. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Mon, 15 Nov 2021 16:42:04 GMT, Michael Strauß wrote: > One comment about adding new JUnit 5 tests and migrating existing tests. I > think there could be value in organizing the tests such that all of the JUnit > 5 tests are grouped, rather than mixing tests in the same directory such that > some use JUnit 5 and others use JUnit 4. What do you (or others) think? We > could either do this with a new JUnit 5 source set in each project, or by > using a package naming convention for JUnit 5 tests like we do for robot > tests. Maybe `test5.some.pkg`. This needs more thought. I don't think this is really needed, since the introduction of JUnit 5 I've written numerous tests in projects with the bulk of their test classes still using JUnit 4. In all cases, the new tests were just mixed with the old tests. The mixing of the tests has so far caused no confusion, and in these projects the teams involved also didn't feel any need to prioritize migrating all tests to JUnit 5. Conversion to JUnit 5 usually happens on a test by test basis when other changes are made to the test -- existing code is rewritten as needed, usually limited to introducing `assertThrows` to replace `@Test(expected = Throwable.class)` and updating all static asserts to use the `jupiter` package hierarchy instead of the `org.junit` one (just import changes). Usually this is done in a separate commit (a "chore" commit) with another commit containing the actual change and new test cases. - PR: https://git.openjdk.java.net/jfx/pull/633
RFR: 8274967: KeyCharacterCombinations for punctuation and symbols fail on non-US keyboards
The code that maps from a Windows virtual key code to a Java key code in `getKeyCodeForChar` did not match the similar code in `ViewContainer::HandleViewKeyEvent`. The OEM keys are assigned to printable punctuation and symbols in way which varies across layouts and even keyboards. To determine the correct Java key code you have to query the layout to determine the character the key would generate without modifiers and map that character to the key code. `ViewContainer::HandleViewKeyEvent` did this, `getKeyCodeForChar` did not. This PR copies a few snippets of code from `ViewContainer::HandleViewKeyEvent` to make the two algorithms match. - Commit messages: - getKeyCodeForChar works on non-US layouts for symbols and punctuation Changes: https://git.openjdk.java.net/jfx/pull/672/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=672&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274967 Stats: 70 lines in 1 file changed: 67 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/672.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/672/head:pull/672 PR: https://git.openjdk.java.net/jfx/pull/672
[jfx17u] Integrated: 8274022: Additional Memory Leak in ControlAcceleratorSupport
On Mon, 15 Nov 2021 18:48:51 GMT, Kevin Rushforth wrote: > Clean backport to `jfx17u`. This pull request has now been integrated. Changeset: e3b061c9 Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx17u/commit/e3b061c9d3667bd7ec327d9c9869d6415fd33414 Stats: 46 lines in 2 files changed: 33 ins; 0 del; 13 mod 8274022: Additional Memory Leak in ControlAcceleratorSupport Backport-of: 0d5b8f8b3f71b1301a65adcc41fe0c7308d4ff01 - PR: https://git.openjdk.java.net/jfx17u/pull/21
[jfx17u] Integrated: 8274022: Additional Memory Leak in ControlAcceleratorSupport
Clean backport to `jfx17u`. - Commit messages: - 8274022: Additional Memory Leak in ControlAcceleratorSupport Changes: https://git.openjdk.java.net/jfx17u/pull/21/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u&pr=21&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274022 Stats: 46 lines in 2 files changed: 33 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jfx17u/pull/21.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/21/head:pull/21 PR: https://git.openjdk.java.net/jfx17u/pull/21
RFR: 8276142: Update gradle to version 7.3
This bumps the version of gradle used to build JavaFX to 7.3 (from the current 7.0.1). Among other things, gradle 7.3 adds support for JDK 17, so this is a prerequisite to update the boot JDK to JDK 17.x -- see [JDK-8276144](https://bugs.openjdk.java.net/browse/JDK-8276144). This will not affect the minimum version of gradle needed to build JavaFX, which remains at 6.3 (although that version requires an even older boot JDK than we currently use, so is not recommended). I included a `gradle/UPDATING.txt ` file to document the steps needed to update to a new version of gradle. NOTE: The boot JDK remains at 16.0.2 with this PR, but the intent is to update the boot JDK soon after this is integrated. To that end, I have created PR #670 as a Draft PR to bump the boot JDK to 17.0.1. It necessarily includes the fix from _this_ PR to update gradle to 7.3, otherwise the GitHub Actions build would have failed. You can look at the test results from that Draft PR to see that JDK 17.0.1 works when buliding with gradle 7.3. - Commit messages: - 8276142: Update gradle to version 7.3 Changes: https://git.openjdk.java.net/jfx/pull/671/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=671&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276142 Stats: 227 lines in 7 files changed: 102 ins; 49 del; 76 mod Patch: https://git.openjdk.java.net/jfx/pull/671.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/671/head:pull/671 PR: https://git.openjdk.java.net/jfx/pull/671
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Thu, 11 Nov 2021 15:18:25 GMT, Kevin Rushforth wrote: > I left a few comments on the dependencies. Will review / test the PR later. > > One comment about adding new JUnit 5 tests and migrating existing tests. I > think there could be value in organizing the tests such that all of the JUnit > 5 tests are grouped, rather than mixing tests in the same directory such that > some use JUnit 5 and others use JUnit 4. What do you (or others) think? We > could either do this with a new JUnit 5 source set in each project, or by > using a package naming convention for JUnit 5 tests like we do for robot > tests. Maybe `test5.some.pkg`. This needs more thought. I don't like the idea of separating source sets or packages just based on the version of our unit test framework. In my opinion, sources should be organized by function, and not based on a technicality. A very common case we'll encounter will be adding a new unit test to an existing test class. What's our guidance in this case? If we continue to use JUnit4 for new unit tests, that means we'll effectively persist this version of the framework forever in the test sources. Another option would be to migrate the entire test class once it's updated with new code. However, that would be a significant amount of work required from the author of a new unit test. A third option would be to actually migrate all unit test classes as a series of refactoring PRs. - PR: https://git.openjdk.java.net/jfx/pull/633
Integrated: 8227371: Drag&Drop while holding the CMD key does not work on macOS
On Fri, 22 Oct 2021 17:32:10 GMT, Martin Fox wrote: > During a drag-and-drop operation on the Mac the Command key will filter out > every drag source operation except `NSDragOperationGeneric` (this behavior is > provided by the OS). JavaFX drag sources only set the Move, Copy, and Link > drag operation bits so the Command key reduces the set of available > operations to nothing. > > This PR changes nothing about how JavaFX behaves when the dnd operation > involves an external application. As a drag source the same set of operations > will be broadcast and as a drag destination the operations will be > interpreted as they have always been. > > For internal dnd within the JavaFX app `NSDragOperationMove` and > `NSDragOperationGeneric` will both be set if the drag source specifies > `TransferMode.MOVE`. On the other end a drag destination will interpret > `NSDragOperationGeneric` and `NSDragOperationMove` as synonyms. > > As part of this PR it was necessary to verify that `NSDragOperationGeneric` > was not already being used during an internal drag operation. There's a > clause in `mapJavaMaskToNsOperation:` which translates > `com_sun_glass_ui_Clipboard_ACTION_ANY` to `NSDragOperationEvery` and this > includes the Generic bit. I believe this is a red herring, the Java dnd code > converts `TransferMode.ANY` to a bitwise-OR of COPY, MOVE, and REFERENCE so > `com_sun_glass_ui_Clipboard_ACTION_ANY` will never be passed down to the > platform level. This pull request has now been integrated. Changeset: f939d094 Author:Martin Fox Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/f939d094e3f02575c77a12c3a986d5f601a65a1d Stats: 87 lines in 3 files changed: 78 ins; 1 del; 8 mod 8227371: Drag&Drop while holding the CMD key does not work on macOS Reviewed-by: pbansal, kcr - PR: https://git.openjdk.java.net/jfx/pull/651
Integrated: 8274022 Additional Memory Leak in ControlAcceleratorSupport
On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier wrote: > This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. > To fix it, I've made the Value of the WeakHashMap also weak. > We only keep this value to remove it as a listener later on. Therefore there > shouldn't be issues by making this value weak. > > > I've seen this Bug very very often, in the last weeks. Most of the > applications I've seen are somehow affected by this bug. > > It basically **breaks every application with menu bars and multiple stages** > - which is the majority of enterprise applications. It's especially annoying > because it takes some time until the application gets unstable. > > Therefore **I would recommend** after this fix is approved, **to make a new > version for JavaFX17** with this fix because this bug is so severe. This pull request has now been integrated. Changeset: 0d5b8f8b Author:Florian Kirmaier Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/0d5b8f8b3f71b1301a65adcc41fe0c7308d4ff01 Stats: 46 lines in 2 files changed: 33 ins; 0 del; 13 mod 8274022: Additional Memory Leak in ControlAcceleratorSupport Reviewed-by: mstrauss, kcr - PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 Additional Memory Leak in ControlAcceleratorSupport [v5]
On Mon, 15 Nov 2021 08:24:04 GMT, Florian Kirmaier wrote: >> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. >> To fix it, I've made the Value of the WeakHashMap also weak. >> We only keep this value to remove it as a listener later on. Therefore there >> shouldn't be issues by making this value weak. >> >> >> I've seen this Bug very very often, in the last weeks. Most of the >> applications I've seen are somehow affected by this bug. >> >> It basically **breaks every application with menu bars and multiple stages** >> - which is the majority of enterprise applications. It's especially annoying >> because it takes some time until the application gets unstable. >> >> Therefore **I would recommend** after this fix is approved, **to make a new >> version for JavaFX17** with this fix because this bug is so severe. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8274022 > the logic to remove the keys from the map, is now more consistent Looks good to me. - Marked as reviewed by mstrauss (Author). PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 Additional Memory Leak in ControlAcceleratorSupport [v3]
On Tue, 2 Nov 2021 09:41:09 GMT, Michael Strauß wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8274022 >> Simplified code related to WeakHashMaps > > Marked as reviewed by mstrauss (Author). @mstr2 can you re-review this? - PR: https://git.openjdk.java.net/jfx/pull/659
RFR: 8277122: SplitPane divider drag can hang the layout
When a divider is moved via drag or code it will call **requestLayout()** for the **SplitPane**. While this is fine, it is also called when the **layoutChildren(..)** method is repositioning the divider. This makes no sense since we are currently layouting everything, so we don't need to request it again (-> We are doing it right now). After positioning the dividers the **SplitPane** content is layouted (based of the may new positioned dividers). This PR fixes this by not requesting layout when we are currently doing it (and thus repositioning the dividers as part of it). Note: I also fixed a simple typo of a private method in SplitPaneSkin: initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers - Commit messages: - 8277122: SplitPane divider drag can hang the layout Changes: https://git.openjdk.java.net/jfx/pull/669/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=669&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277122 Stats: 61 lines in 2 files changed: 54 ins; 1 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/669.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/669/head:pull/669 PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8274022 Additional Memory Leak in ControlAcceleratorSupport [v5]
On Mon, 15 Nov 2021 08:24:04 GMT, Florian Kirmaier wrote: >> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. >> To fix it, I've made the Value of the WeakHashMap also weak. >> We only keep this value to remove it as a listener later on. Therefore there >> shouldn't be issues by making this value weak. >> >> >> I've seen this Bug very very often, in the last weeks. Most of the >> applications I've seen are somehow affected by this bug. >> >> It basically **breaks every application with menu bars and multiple stages** >> - which is the majority of enterprise applications. It's especially annoying >> because it takes some time until the application gets unstable. >> >> Therefore **I would recommend** after this fix is approved, **to make a new >> version for JavaFX17** with this fix because this bug is so severe. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8274022 > the logic to remove the keys from the map, is now more consistent Great, I've just updated the title! - PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v5]
On Mon, 15 Nov 2021 08:24:04 GMT, Florian Kirmaier wrote: >> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. >> To fix it, I've made the Value of the WeakHashMap also weak. >> We only keep this value to remove it as a listener later on. Therefore there >> shouldn't be issues by making this value weak. >> >> >> I've seen this Bug very very often, in the last weeks. Most of the >> applications I've seen are somehow affected by this bug. >> >> It basically **breaks every application with menu bars and multiple stages** >> - which is the majority of enterprise applications. It's especially annoying >> because it takes some time until the application gets unstable. >> >> Therefore **I would recommend** after this fix is approved, **to make a new >> version for JavaFX17** with this fix because this bug is so severe. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8274022 > the logic to remove the keys from the map, is now more consistent Looks good. Btw, you still need to change the title of this PR to match the JBS title. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 1 Nov 2021 12:59:42 GMT, Marius Hanl wrote: >> well .. that would be a merge conflict, had you updated the code comment in >> your PR 😁 As noted in my comments to Ajit's review, the listener >> registration is simply moved (including the code comment .. belatedly :) >> >> Not sure how to handle it from here - following the rules, we might need a >> follow-up issue to the issue fixed in your PR? > > My PR is already merged, so this is not a problem. :) > I dont know, but since this is only fixing a (also before) wrong comment it > might be okay as it is very minor? :) FYI: now the listener registration - including the incorrect code comment (which is the same as in current master) - is back at the old location in the re-inserted setupTreeTableViewListeners. So still need input whether it's okay to correct the code comment here. - PR: https://git.openjdk.java.net/jfx/pull/632
Integrated: 8232812: [MacOS] Double click title bar does not restore window size
On Tue, 5 Oct 2021 20:36:37 GMT, Martin Fox wrote: > The test case for JDK-8160241 creates a window in a zoomed state (as defined > by macOS). When the OS later goes to unzoom the window it will try to shrink > it down to 1 point wide. This was entered as JDK-8163137 but the fix for that > bug inadvertently disabled unzooming altogether. This PR fixes 8163137 in a > slightly different way. > > Access to the zoom/unzoom feature has changed with newer versions of macOS. > To reproduce this you have to change `System Preferences > Dock & Menu Bar > > Double-click a window's title bar` to `zoom`. Then use double-clicks in the > title bar to test the feature. The green button in the title bar no longer > has anything to do with zoom/unzoom. This pull request has now been integrated. Changeset: 7ddd6427 Author:Martin Fox Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/7ddd6427814877ddf191b83e371f166b326ce821 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod 8232812: [MacOS] Double click title bar does not restore window size Reviewed-by: kcr, pbansal - PR: https://git.openjdk.java.net/jfx/pull/639
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v4]
> Cleanup of Tree-/TableRowSkin to support switching skins > > The misbehavior/s > - memory leaks due to manually registered listeners that were not removed > - side-effects due to listeners still active on old skin (like NPEs) > > Fix > - use skin api for all listener registration (for automatic removal in > dispose) > - do not install listeners that are not needed (fixedCellSize, same as in fix > of ListCellSkin > [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) > > Added tests for each listener involved in the fix to guarantee it's still > working and does not have unwanted side-effects after switching skins. > > Note: there are pecularities in row skins (like not updating themselves on > property changes of its control, throwing NPEs when not added to a > VirtualFlow) which are not part of this issue but covered in > [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: reverted fixedCellSize handling - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/fc71c931..603b08af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=02-03 Stats: 312 lines in 6 files changed: 243 ins; 50 del; 19 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
RFR: 8160597: IllegalArgumentException when we initiate drag on Image
This PR fixes an exception that can happen when dragging an image from a WebView on macOS. Dragging an image that is directly included like `` works fine, without exception. However, there are (at least) two other cases when the IAE is thrown: - Dragging an image that is part of an hyperlink, like ``. - Dragging an image encoded as base64. The IAE happens only on macOS, where a native image is created in `GlassPasteboard.m` from an URL, and in those two cases the generated image has 0x0 dimensions: `` which leads to `ByteArrayFromPixels` being called with a null byte array. The included test reproduces the case by directly pushing the `MacPasteboard` content (instead of adding a more complex test with a webView and a drag and drop gesture). - Commit messages: - Remove unused imports - Check if MacPasteboard contains a valid image, including test Changes: https://git.openjdk.java.net/jfx/pull/668/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=668&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8160597 Stats: 155 lines in 5 files changed: 154 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/668.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/668/head:pull/668 PR: https://git.openjdk.java.net/jfx/pull/668
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v7]
On Mon, 18 Oct 2021 08:20:22 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 > Fixing toggle fullscreen! I've now added a check for null when calling the initScreens method. This should fix the null pointer exception. - PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v8]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/622/files - new: https://git.openjdk.java.net/jfx/pull/622/files/4dcbcc22..faddeb60 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=622&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=622&range=06-07 Stats: 8 lines in 1 file changed: 4 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/622.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/622/head:pull/622 PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Fri, 12 Nov 2021 23:42:30 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8274022
>> Simplified code related to WeakHashMaps
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
> line 291:
>
>> 289: WeakReference> listenerW
>> = changeListenerMap.get(menuitem);
>> 290: ChangeListener listener = listenerW ==
>> null ? null : listenerW.get();
>> 291: if (listener != null) {
>
> This will fail to remove a weak reference to a listener that has been
> collected. It is a `WeakHashMap`, so the `WeakReference` to the listener will
> be reclaimed as soon as the `menuItem` is no longer reachable, but if you
> used the same logic as you did for the scene listener (e.g., on lines
> 254-261), it would be removed sooner. I don't know whether it matters in this
> case.
Yes, you are right. I now use the same logic in all 3 places.
I guess it doesn't matter because the key always references the listener - but
the code is now more straight line.
-
PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v5]
> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. > To fix it, I've made the Value of the WeakHashMap also weak. > We only keep this value to remove it as a listener later on. Therefore there > shouldn't be issues by making this value weak. > > > I've seen this Bug very very often, in the last weeks. Most of the > applications I've seen are somehow affected by this bug. > > It basically **breaks every application with menu bars and multiple stages** > - which is the majority of enterprise applications. It's especially annoying > because it takes some time until the application gets unstable. > > Therefore **I would recommend** after this fix is approved, **to make a new > version for JavaFX17** with this fix because this bug is so severe. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: 8274022 the logic to remove the keys from the map, is now more consistent - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/d2a1c448..873c3090 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=03-04 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v4]
> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. > To fix it, I've made the Value of the WeakHashMap also weak. > We only keep this value to remove it as a listener later on. Therefore there > shouldn't be issues by making this value weak. > > > I've seen this Bug very very often, in the last weeks. Most of the > applications I've seen are somehow affected by this bug. > > It basically **breaks every application with menu bars and multiple stages** > - which is the majority of enterprise applications. It's especially annoying > because it takes some time until the application gets unstable. > > Therefore **I would recommend** after this fix is approved, **to make a new > version for JavaFX17** with this fix because this bug is so severe. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: 8274022 removed an unused import - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/bbc39f24..d2a1c448 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Fri, 12 Nov 2021 22:42:35 GMT, Kevin Rushforth wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8274022 >> Simplified code related to WeakHashMaps > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java > line 31: > >> 29: import javafx.beans.property.ReadOnlyObjectProperty; >> 30: import javafx.beans.value.ChangeListener; >> 31: import javafx.beans.value.WeakChangeListener; > > Unused import. removed! - PR: https://git.openjdk.java.net/jfx/pull/659
