Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Wed, 12 Jun 2024 11:52:47 GMT, Alexey Ivanov wrote: > How do we remove this constructor? Can it be removed right away? Should it be > deprecated for several releases before it's removed? Just delete it in all versions of 17+? - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2163885450
Re: RFR: 8291472: [macos] jawt 1.4 lock/unlock not supported
On Mon, 10 Jun 2024 21:09:35 GMT, Alisen Chung wrote: > Removing if check for unsupported jawt versions Is it possible to cover this fix by the test? - PR Comment: https://git.openjdk.org/jdk/pull/19639#issuecomment-2159660501
Re: RFR: 8331619: TabbedPane's contentOpaque, tabsOpaque and setOpaque doesn't work properly in Aqua LAF
On Fri, 10 May 2024 17:33:53 GMT, Sergey Bylokhov wrote: >> JTabbedPane's contentOpaque and tabsOpaque properties are not honored in >> Aqua L JTabbedPane's content area and tab background color are not as >> expected when tabbedpane opacity is set to true or false. Fix is to handle >> the opacity behavior correctly and inline with other LAF as well. >> >> Existing test `TestBackgroundScrollPolicy.java` failed with the proposed fix >> and it is updated to run only for linux and windows platform because the >> content area for tabbedpane is rendered to the width and height of >> tabbedpane starting from (0, 0) position >> (https://github.com/openjdk/jdk/blob/cf7c97732320d70de5f5725c920d5c3861a2c9c8/src/java.desktop/macosx/classes/com/apple/laf/AquaTabbedPaneUI.java#L684C16-L684C16) >> and that leaves no place for tab area behind tabs. >> >> CI testing is green after this test update and link posted in JBS. > > The Aqua L mimics the behavior of the native UI on macOS. Is it possible to > change all these properties in the native apps? > @mrserb I am going to close this bug as "Not an issue" as I couldn't find any > ways to check this behavior. sounds good. - PR Comment: https://git.openjdk.org/jdk/pull/19170#issuecomment-2159590964
Re: RFR: 8185429: [macos] After the dialog is closed, there is no window become active
On Thu, 6 Jun 2024 23:28:07 GMT, Alisen Chung wrote:
> Add a check for previous focused window on modal unblocking. If the owner of
> a closing dialog was the last focused window, then the owner of the dialog
> should regain focus.
src/java.desktop/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java line 1062:
> 1060:
> 1061: Window currFocus =
> LWKeyboardFocusManagerPeer.getInstance().getCurrentFocusedWindow();
> 1062: if (!blocked && target.equals(currFocus)) {
it is better to use "==" instead of "equals" which might be overridden by the
app.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19588#discussion_r1631545097
Re: RFR: 8332103: since-checker - Add missing `@since` tags to `java.desktop` [v2]
On Wed, 29 May 2024 11:51:34 GMT, Alexey Ivanov wrote: >> When mapping methods and when they first appeared (by using the historical >> record built into javac) I use an id in the form of >> `method: >> .()` so >> for covariant overrides in general, when the return type changes I consider >> it to be a new method. >> >> Looking at the contents of the dictionnary: >> This explicit constructor existed for a long time but then this new was >> added a new one was added in JDK 16 >> | Key | Value | >> | - | - | >> | `method: void >> javax.swing.plaf.basic.BasicSliderUI.(javax.swing.JSlider):` | 9 | >> | `method: void javax.swing.plaf.basic.BasicSliderUI.():` | 16 | >> >> Note: JDK 9 is used as the "base" as that's how far I can reliably use the >> `--release` info, so if something was added in JDK 2,5.7,9. It has a value >> of "9" in the dictionnary. I mainly check for errors in newer code. > >> Hmm, the _explicit_ default constructor was added in JDK 16, but it was >> implicit before then. So I am not 100% sure what the right answer is - the >> same as the class, or when it was explicitly added. > > I believe there was no default constructor in `BasicSliderUI()` because there > was a constructor with a parameter `BasicSliderUI(JSlider b)`. > > Thus, this case seem to be correct `BasicSliderUI()` is available since 16. > > At the same time, `BasicSliderUI(JSlider b)` has existed since at least 7, > the constructor is present in the history of the file. The history in GitHub > goes up to 1st December 2007 which corresponds to Java 7 timeline. I'm pretty > sure this constructor existed in previous releases, and you have to dig > further to find when it was added. > > Very much likely, the constructor `BasicSliderUI(JSlider b)` was added when > the `BasicSliderUI` class was added. The class does not have `@since` tag, so > it's inherited from the package, isn't it? The same rule applies to the > constructor, doesn't it? It seems that BasicSliderUI() was added by the mistake? it was not mentioned in the bug report...Seems it is too late to delete it? - PR Review Comment: https://git.openjdk.org/jdk/pull/19192#discussion_r1624976828
Integrated: 8331746: Create a test to verify that the cmm id is not ignored
On Mon, 6 May 2024 20:51:55 GMT, Sergey Bylokhov wrote: > The new test to cover the https://bugs.openjdk.org/browse/JDK-8326661 and > verify that the cmm id of the icc profile is properly reported. Before > JDK-8321489 we always report 'lcms' as a cmm id. This pull request has now been integrated. Changeset: 7c750fd9 Author: Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/7c750fd95b83d0a93b0cce681dcfbbae1f220fdd Stats: 69 lines in 1 file changed: 69 ins; 0 del; 0 mod 8331746: Create a test to verify that the cmm id is not ignored Reviewed-by: prr, dmarkov, aivanov - PR: https://git.openjdk.org/jdk/pull/19110
Re: RFR: 8331746: Create a test to verify that the cmm id is not ignored [v3]
> The new test to cover the https://bugs.openjdk.org/browse/JDK-8326661 and > verify that the cmm id of the icc profile is properly reported. Before > JDK-8321489 we always report 'lcms' as a cmm id. Sergey Bylokhov has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8331746 - Update CustomCMMID.java - Update CustomCMMID.java - 8331746: Create a test to verify that the cmm id is not ignored - Changes: - all: https://git.openjdk.org/jdk/pull/19110/files - new: https://git.openjdk.org/jdk/pull/19110/files/6bfb2cb8..c6109b3d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19110=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19110=01-02 Stats: 24165 lines in 537 files changed: 13082 ins; 7446 del; 3637 mod Patch: https://git.openjdk.org/jdk/pull/19110.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19110/head:pull/19110 PR: https://git.openjdk.org/jdk/pull/19110
Re: RFR: 8331746: Create a test to verify that the cmm id is not ignored [v2]
> The new test to cover the https://bugs.openjdk.org/browse/JDK-8326661 and > verify that the cmm id of the icc profile is properly reported. Before > JDK-8321489 we always report 'lcms' as a cmm id. Sergey Bylokhov has updated the pull request incrementally with one additional commit since the last revision: Update CustomCMMID.java - Changes: - all: https://git.openjdk.org/jdk/pull/19110/files - new: https://git.openjdk.org/jdk/pull/19110/files/933de54e..6bfb2cb8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19110=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19110=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19110.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19110/head:pull/19110 PR: https://git.openjdk.org/jdk/pull/19110
Re: RFR: 8331619: TabbedPane's contentOpaque, tabsOpaque and setOpaque doesn't work properly in Aqua LAF
On Fri, 10 May 2024 07:05:11 GMT, Abhishek Kumar wrote: > JTabbedPane's contentOpaque and tabsOpaque properties are not honored in Aqua > L JTabbedPane's content area and tab background color are not as expected > when tabbedpane opacity is set to true or false. Fix is to handle the opacity > behavior correctly and inline with other LAF as well. > > Existing test `TestBackgroundScrollPolicy.java` failed with the proposed fix > and it is updated to run only for linux and windows platform because the > content area for tabbedpane is rendered to the width and height of tabbedpane > starting from (0, 0) position > (https://github.com/openjdk/jdk/blob/cf7c97732320d70de5f5725c920d5c3861a2c9c8/src/java.desktop/macosx/classes/com/apple/laf/AquaTabbedPaneUI.java#L684C16-L684C16) > and that leaves no place for tab area behind tabs. > > CI testing is green after this test update and link posted in JBS. The Aqua L mimics the behavior of the native UI on macOS. Is it possible to change all these properties in the native apps? - PR Comment: https://git.openjdk.org/jdk/pull/19170#issuecomment-2105005574
Re: RFR: 8260633: [macos] java/awt/dnd/MouseEventAfterStartDragTest/MouseEventAfterStartDragTest.html test failed
On Tue, 7 May 2024 18:03:23 GMT, Alisen Chung wrote: > Opening closed dnd test > Test is green on all platforms Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19128#pullrequestreview-2044249314
RFR: 8331746: Create a test to verify that the cmm id is not ignored
The new test to cover the https://bugs.openjdk.org/browse/JDK-8326661 and verify that the cmm id of the icc profile is properly reported. Before JDK-8321489 we always report 'lcms' as a cmm id. - Commit messages: - 8331746: Create a test to verify that the cmm id is not ignored Changes: https://git.openjdk.org/jdk/pull/19110/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19110=00 Issue: https://bugs.openjdk.org/browse/JDK-8331746 Stats: 68 lines in 1 file changed: 68 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19110.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19110/head:pull/19110 PR: https://git.openjdk.org/jdk/pull/19110
Re: RFR: 8280988: [XWayland] Click on title to request focus test failures [v3]
On Thu, 2 May 2024 13:01:12 GMT, Alexander Zvegintsev wrote: >> Some tests try to get the focus of a window by clicking on its title bar. >> >> On XWayland, however, emulating mouse clicks with XTEST currently only >> affects the XWayland server, not the window decorations, so trying to click >> on the window title will have no effect. >> >> So the solution is to click inside the window or call `toFront()` to get the >> window focused. >> >> Some tests have issues with AWT/Swing calls not being on EDT, it is not the >> goal of this change to fix them as they require way too many code changes, >> and make this PR hard to review. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > toFront + clickOnTitle(only for non wayland environment) Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18995#pullrequestreview-2036785634
Re: RFR: 8331516: Tests should not use the "Classpath" exception form of the legal header
On Thu, 2 May 2024 18:45:54 GMT, Phil Race wrote: > Change GPL+CP -> GPL as required for all tests. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19068#pullrequestreview-2036784210
Re: RFR: 8280988: [XWayland] Click on title to request focus test failures [v2]
On Tue, 30 Apr 2024 16:49:49 GMT, Alexander Zvegintsev
wrote:
>> test/jdk/java/awt/Focus/6981400/Test1.java line 186:
>>
>>> 184: Util.clickOnComp(compToClick, robot);
>>> 185:
>>> 186: if (Platform.isOnWayland()) {
>>
>> If the goal is just to move the window to the front then can we try "toFront
>> + click on title" on X11 and just "toFront" on Wayland?
>
> Do we really need an additional click on the title in this case?
> Just `toFront` should be enough on all platforms.
`toFront()` is known to be broken on some versions of metacity/gnome, not sure
it was fixed or not, that was a reason why the "click" workaround was
introduced. If it works fine on Wayland we can just use it and leave the old
x11 workaround as is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18995#discussion_r1586896682
Re: RFR: 8331011: [XWayland] TokenStorage fails under Security Manager [v2]
On Wed, 1 May 2024 16:43:13 GMT, Phil Race wrote: > The issue was discovered by failing a closed test that you wrote some time > ago. =) ok - PR Comment: https://git.openjdk.org/jdk/pull/18950#issuecomment-2089251764
Re: RFR: 8331011: [XWayland] TokenStorage fails under Security Manager [v2]
On Thu, 25 Apr 2024 17:22:48 GMT, Alexander Zvegintsev wrote: >> This fix adds missing doPrivileged calls in TokenStorage, which is used to >> help take screenshots in Wayland. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > fix copyright years Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18950#pullrequestreview-2034575030
Re: RFR: 8331011: [XWayland] TokenStorage fails under Security Manager [v2]
On Thu, 25 Apr 2024 17:22:48 GMT, Alexander Zvegintsev wrote: >> This fix adds missing doPrivileged calls in TokenStorage, which is used to >> help take screenshots in Wayland. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > fix copyright years How hard is it to create a new test for this issue? - PR Comment: https://git.openjdk.org/jdk/pull/18950#issuecomment-2084434625
Re: RFR: 8280988: [XWayland] Click on title to request focus test failures [v2]
On Tue, 30 Apr 2024 02:02:30 GMT, Alexander Zvegintsev
wrote:
>> Some tests try to get the focus of a window by clicking on its title bar.
>>
>> On XWayland, however, emulating mouse clicks with XTEST currently only
>> affects the XWayland server, not the window decorations, so trying to click
>> on the window title will have no effect.
>>
>> So the solution is to click inside the window or call `toFront()` to get the
>> window focused.
>>
>> Some tests have issues with AWT/Swing calls not being on EDT, it is not the
>> goal of this change to fix them as they require way too many code changes,
>> and make this PR hard to review.
>
> Alexander Zvegintsev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review comments
test/jdk/java/awt/Focus/6981400/Test1.java line 186:
> 184: Util.clickOnComp(compToClick, robot);
> 185:
> 186: if (Platform.isOnWayland()) {
If the goal is just to move the window to the front then can we try "toFront +
click on title" on X11 and just "toFront" on Wayland?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18995#discussion_r1584163903
Re: RFR: 8331142: Add test for number of loader threads in BasicDirectoryModel
On Thu, 25 Apr 2024 16:37:39 GMT, Alexey Ivanov wrote: > This PR provides a regression test for > [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179): _Race in > BasicDirectoryModel.validateFileCache_ reviewed in #18111. > > The test is inspired and based on `ConcurrentModification` that I wrote for > [JDK-8327137](https://bugs.openjdk.org/browse/JDK-8327137) / > [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670). > > **How the test works** > > The test creates a temporary directory in the current directory and creates a > number of files in it. (The number of files is controlled by > `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the > temporary directory. > > The test starts several scanner threads, the number is controlled by > `NUMBER_OF_THREADS`, which repeatedly call > `fileChooser.rescanCurrentDirectory()`. This results in calling > `BasicDirectoryModel.validateFileCache` which starts a background thread — > "Basic L File Loading Thread" — to enumerate the files. > > The test runner thread and scanner threads are synchronised with > `CyclicBarrier`, this ensures all the scanner threads start at the same time. > After a short delay, the runner thread takes a snapshot of live threads. > After a longer delay, the operation is repeated. See the `getThreadSnapshot` > method. (The number of snapshots is controlled by `SNAPSHOTS` constant.) > > The number of File Loading Threads in each snapshot is counted. There should > be no more than two threads. It is possible that thread two such threads > after JDK-8325179 is fixed: the existing thread is interrupted but hasn't > exited yet, and a new thread is already created. > > The test fails consistently without the fix for JDK-8325179. On Windows, the > output looks like this: > > > Number of snapshots: 20 > Number of snapshots where number of loader threads: > = 1: 0 > = 2: 0 > > 2: 20 > java.lang.RuntimeException: Detected 20 snapshots with several loading threads > at LoaderThreadCount.runTest(LoaderThreadCount.java:132) > at LoaderThreadCount.wrapper(LoaderThreadCount.java:72) > at java.base/java.lang.Thread.run(Thread.java:1583) > > > On Linux and macOS, there's more variation, for example I got the following > output on one of Linux systems: > > > Number of snapshots: 15 > Number of snapshots where number of loader threads: > = 1: 7 > = 2: 2 > > 2: 6 > > > The test passes on the builds where JDK-8325179 is present. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18957#pullrequestreview-2024039392
Re: RFR: 8319598: SMFParser misinterprets interrupted running status [v2]
On Sat, 11 Nov 2023 12:32:15 GMT, Jan Trukenmüller wrote: >> The MIDI file parser misinterprets events without status byte when they >> appear directly after a Meta of SysEx event. >> >> For my bugfix I had to decide between two possible solutions: >> - Strict solution: Throw an InvalidMidiDataException >> - Tolerant solution: Use the status of the last channel event as running >> status >> >> I used the tolerant solution. >> I will send an email to the list client-libs-dev with my reasons. > > Jan Trukenmüller has updated the pull request incrementally with one > additional commit since the last revision: > > reduced line lengths Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16546#pullrequestreview-2016467422
Re: RFR: 8328896: Fontmetrics for large Fonts has zero width
On Tue, 9 Apr 2024 18:50:17 GMT, Phil Race wrote: > The main problem here is that we do not curate what font point size we passed > to freetype, > and would pass in one larger than freetype's maximum of FT_USHORT_MAX which > is USHRT_MAX. > This isn't documented, SFAICS, and is checked a couple of calls deep from the > specific API we use. > But generally anywhere near that size seems to cause freetype to choke as it > uses signed 16.16 > values, so 32767 is really the max. > But we have no need to go anywhere near that - 16384 seems like a plenty > large enough pt size. > And we can request bigger sizes than that by making use of the transform. > At normal size ranges we use that just to account for rotation and decompose > the glyph transform > into point size and that rotation. > > But at large sizes - which are not usefully rendered anyway - there are no > hints etc to be lost > from not specifying the target point size. So we can extend the range of > sizes we allow. > > If this is still too large to be held decomposed into a pt size in the range > less than 16384 and a scale of up to 32766 then we substitute the null > scaler, as we generally do when values are out of range, such > a for a NaN transform. > > These extreme values aren't useful. > > In looking at this I did find that getGlyphPixelBounds doesn't follow the > maximum image size we use > for rendering. Which is not useful and could lead to inconsistent results. I > fixed that. > > Also whilst macOS didn't have these problems it could be cleaned up a bit for > consistency in the reported sizes for some cases. > > The test is mainly around making sure that "sensible" things are returned for > not sensible input. > There's no 100% right answer to these extreme unrenderable sizes. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18703#pullrequestreview-2016175337
Re: RFR: 8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ [v4]
On Wed, 17 Apr 2024 04:31:16 GMT, Abhishek Kumar wrote: >> Test was failing on GTK and Windows LAF due to pixel color mismatch. Th >> reason behind this issue was the size of image which is different and that >> results in incorrect pixel comparison. Fix is to ensure that correct pixel >> is matched and the pixel color should remain within tolerance. >> For windows LAF the background color is not an exact match and thus added a >> TOLERANCE field to check if the RGB difference is within limits. >> >> `@key headful` added in jtreg tag to ensure that test run for GTK LAF as >> well which was not the case before as it is mentioned in JBS `It does not >> fail in mach5 because on linux + headless mode the gtk L is not supported.` >> >> CI testing is green for the modified test. Link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > testDir moved to method Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18644#pullrequestreview-2009415330
Re: RFR: 8289770: Remove Windows version macro from ShellFolder2.cpp
On Thu, 11 Apr 2024 09:33:09 GMT, Alexey Ivanov wrote: > This clean-up PR removes unused Windows version macro from `ShellFolder2.cpp`. > > `IS_WINVISTA` was not used at all. > > `IS_WINXP` guarded support for icons with alpha channel. It is now safe to > assume Java runs on a Windows version later than Windows XP. Java launchers > specify 6.0 as the minimum OS version which corresponds to Windows Vista. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18736#pullrequestreview-2008154224
Re: RFR: 8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ [v3]
On Tue, 16 Apr 2024 04:16:26 GMT, Abhishek Kumar wrote: >> Test was failing on GTK and Windows LAF due to pixel color mismatch. Th >> reason behind this issue was the size of image which is different and that >> results in incorrect pixel comparison. Fix is to ensure that correct pixel >> is matched and the pixel color should remain within tolerance. >> For windows LAF the background color is not an exact match and thus added a >> TOLERANCE field to check if the RGB difference is within limits. >> >> `@key headful` added in jtreg tag to ensure that test run for GTK LAF as >> well which was not the case before as it is mentioned in JBS `It does not >> fail in mach5 because on linux + headless mode the gtk L is not supported.` >> >> CI testing is green for the modified test. Link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > Review comment update, tolerance check removed As far as I understand the test creates two enabled-JComboBox, then disables both, and then compares the disabled and enabled images. Why there are some differences in sizes and colors? - PR Comment: https://git.openjdk.org/jdk/pull/18644#issuecomment-2058394210
Re: RFR: 8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ [v2]
On Tue, 16 Apr 2024 04:09:47 GMT, Abhishek Kumar wrote:
>> test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java line 58:
>>
>>> 56:
>>> 57: private static void createCombo() {
>>> 58: combo = new JComboBox();
>>
>> If we can use unicode blank character instead of "Simple JComboBox" text. I
>> think we make this test more robust and avoid tolerance checks.
>
> Updated.
Can the initial bug "JComboBox has correct font color when disabled" be
verified with this change?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18644#discussion_r1566836859
Re: RFR: 8289770: Remove Windows version macro from ShellFolder2.cpp
On Thu, 11 Apr 2024 09:33:09 GMT, Alexey Ivanov wrote: > This clean-up PR removes unused Windows version macro from `ShellFolder2.cpp`. > > `IS_WINVISTA` was not used at all. > > `IS_WINXP` guarded support for icons with alpha channel. It is now safe to > assume Java runs on a Windows version later than Windows XP. Java launchers > specify 6.0 as the minimum OS version which corresponds to Windows Vista. src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 131: > 129: #ifndef IS_WINVISTA > 130: #define IS_WINVISTA (!(::GetVersion() & 0x8000) && > LOBYTE(LOWORD(::GetVersion())) >= 6) > 131: #endif there are exactly the same macro in awt.h which are mostly unused except "IS_WIN8" - PR Review Comment: https://git.openjdk.org/jdk/pull/18736#discussion_r1566819367
Re: RFR: 8321428: Deprecate for removal the package java.beans.beancontext [v4]
On Wed, 10 Apr 2024 21:25:06 GMT, Larry Cable wrote: >> the beancontext package was added (by me) in JDK 1.2 to provide >> JavaBeans(tm) with a containment and services hierarchy. >> >> based upon concepts from OpenDoc, which was a popular component model at the >> time, the API pre-dated the addition of language features such as >> annotations, and the invention and adoption of programming design patterns >> such as "Inversion of Control" and/or "Dependency Injection". >> >> This API (package) has not evolved with either the language or current >> design trends, as such it is, at best, anachronistic, and is probably an >> anti-pattern that should be avoided. >> >> This package is therefore deprecated **FOR REMOVAL IN AT FUTURE RELEASE** > > Larry Cable has updated the pull request incrementally with one additional > commit since the last revision: > > changed copyright dates, and normalized @Deprecated formatting Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18569#pullrequestreview-1993256591
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 5 Mar 2024 17:40:01 GMT, Alexey Ivanov wrote: >> The implementation of `Win32ShellFolder2.compareTo` is inconsistent: there >> are cases where >> `a < b & b < c but a == c` >> which *violates its general contract*. >> >> In particular, it happens for the personal folder (*Documents*) if it is >> listed *twice*: as a special and as a regular folder. >> >> The evaluation performed by the submitter of the bug provided enough details >> to create a test, reproduce the problem and finally resolve it. >> >> Without the fix, the regression test always fails: >> >> a < b & b < c but a >= c >> where >> a = C:\Users\Documents(true) >> b = C:\Users(false) >> c = C:\Users\Documents(false) >> >> as well as for the reverse case: `a > b & b > c`. >> >> How it is possible to have the same folder in a list of files twice remains >> unknown. I believe it is another bug in JDK, however, no one has been able >> to reproduce it so far. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Handle fakePersonal on Windows 10 > > The desktop folder on Windows 10 does not include > the Personal (Documents) folder. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18126#pullrequestreview-1987018502
Re: RFR: 8329761: Remove unused KeyBuilder and unusedSets from StyleContext
On Fri, 5 Apr 2024 15:46:06 GMT, Alexey Ivanov wrote: > Remove unused class `KeyBuilder` and unused field `unusedSets`. > Update a comment which used javadoc syntax. > Mark `KeyEnumeration` and `FontKey` classes as *`final`*. > > All the client tests are green. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18659#pullrequestreview-1984044967
Re: RFR: 8329761: Remove unused KeyBuilder and unusedSets from StyleContext
On Fri, 5 Apr 2024 15:46:06 GMT, Alexey Ivanov wrote: > Remove unused class `KeyBuilder` and unused field `unusedSets`. > Update a comment which used javadoc syntax. > Mark `KeyEnumeration` and `FontKey` classes as *`final`*. > > All the client tests are green. Could you please check the history of that file, is these unused due to some functionality was deleted and this was missed, or we planned to implement something and forgot? - PR Comment: https://git.openjdk.org/jdk/pull/18659#issuecomment-2040218603
Re: RFR: 8321428: Deprecate for removal the package java.beans.beancontext
On Mon, 1 Apr 2024 22:53:12 GMT, Larry Cable wrote: > the beancontext package was added (by me) in JDK 1.2 to provide JavaBeans(tm) > with a containment and services hierarchy. > > based upon concepts from OpenDoc, which was a popular component model at the > time, the API pre-dated the addition of language features such as > annotations, and the invention and adoption of programming design patterns > such as "Inversion of Control" and/or "Dependency Injection". > > This API (package) has not evolved with either the language or current design > trends, as such it is, at best, anachronistic, and is probably an > anti-pattern that should be avoided. > > This package is therefore deprecated **FOR REMOVAL IN AT FUTURE RELEASE** > public code corpus should be searched prior to removal in order to determine > impact. What are results of that search? - PR Comment: https://git.openjdk.org/jdk/pull/18569#issuecomment-2032434587
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 29 Mar 2024 20:02:53 GMT, Alexey Ivanov wrote: >>> However, there are cases where instantiating and testing is safe on main >>> thread. >> >> That is my point, make it less safe - when the component is created on one >> thread:EDT and then for some reason accessed on a different thread, the >> rescanCurrentDirectory should still work. > >> > However, there are cases where instantiating and testing is safe on main >> > thread. >> >> That is my point, make it less safe - when the component is created on one >> thread:EDT and then for some reason accessed on a different thread, the >> rescanCurrentDirectory should still work. > > I got your idea! > > However, I still see no benefit for doing it. > > Yes, when `JFileChooser` is instantiated, it creates its UI delegates and the > default model, `BasicDirectoryModel`. The constructor of the model calls > `validateFileCache` which starts the background thread and will update > `fileCache`. > > It does not make the initialisation less thread-safe: the same two threads > are involved — `FilesLoader` and EDT. Yet there's no race yet… > > The `ConcurrentModificationException` is thrown when the background > `FilesLoader` is iterating over `fileCache` while EDT runs > `DoChangeContents.run` which adds elements to `fileCache` or removes elements > from it. > > By the time the `Scanner` threads start, a race becomes possible. If the > initial `FilesLoader` completes and posts the `DoChangeContents` object to > EDT, the `FilesLoader` threads created by the scanners will race… but > `fileCache` is empty initially, thus iterating over it is very quick. > > Therefore getting `ConcurrentModificationException` is unlikely until > `fileCache` contains a list of files. This state is reached later in the > timeline of the test. > > The initial `FilesLoader` thread that's started when `JFileChooser` is > instantiated plays a role too… Yet it's running concurrently with the > `Scanner` threads whether the `JFileChooser` object is created on the current > thread or on EDT. > > --- > > Taking the above into account, instantiating `JFileChooser` on EDT adds > complexity to the test but brings no benefits. ok, I agree with you. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1545858033
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 15 Mar 2024 14:45:36 GMT, Alexey Ivanov wrote: >> I'm adding a regression test for >> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and >> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091); it's also a >> regression test for >> [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690). >> >> I referenced this test in PR #17462 in [this >> comment](https://github.com/openjdk/jdk/pull/17462#issuecomment-1914844026). >> I fine-tuned the test since that time. >> >> The test doesn't fail all the time without the fix for JDK-8323670, however, >> it fails sometimes. If you run the test several times, it will likely fail >> _without the fix_. >> >> For me, the test fails about 10 times from 40 runs in the CI. It fails on >> macOS more frequently than on Linux. >> >> When the test passes, it usually completes in 5 minutes. >> >> **How the test works** >> >> The test creates a temporary directory in the current directory and creates >> a number of files in it. (The number of files is controlled by >> `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the >> temporary directory. >> >> The test starts several _scanner_ threads, the number is controlled by >> `NUMBER_OF_THREADS`, which repeatedly call >> `fileChooser.rescanCurrentDirectory()`. This results in calling >> `BasicDirectoryModel.validateFileCache` which starts a background thread — >> "Basic L File Loading Thread" — to enumerate the files. >> >> A timer is used to create new files in the directory that the file chooser >> is using. >> >> After enumerating the files, the File Loading Thread posts an event to EDT. >> The event updates `fileCache` and fires a `ListDataEvent`. >> >> If the File Loading Thread is iterating over `fileCache` using `Iterator` >> (when `fileCache.subList` or `fileCache.equals` is running; or a new >> `Vector` instance is created from a `fileCache` or its sublist) and >> `fileCache` is being updated on EDT, then `ConcurrentModificationException` >> is thrown. >> >> On Linux and on _headless_ macOS, `ShellFolder.invoke` is executed in the >> caller, which makes it easier to reproduce the issue. Because of >> [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179), there are >> several File Loading Threads, which also helps to reproduce the issue. >> >> On _headful_ macOS, the `BasicDirectoryModel` is not used, so the test does >> not reproduce the issue. >> >> On Windows, the test does not fail or fails with `OutOfMemoryError`. It is >> because all the File Loading Threads are serialised on the COM thread, >> `ShellFolder.invoke` submits the task to the COM thread and waits for i... > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Suppress throwing exceptions while deleting files Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18109#pullrequestreview-1970499628
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Wed, 27 Mar 2024 20:19:41 GMT, Alexey Ivanov wrote: > However, there are cases where instantiating and testing is safe on main > thread. That is my point, make it less safe - when the component is created on one thread:EDT and then for some reason accessed on a different thread, the rescanCurrentDirectory should still work. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1543954407
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Tue, 19 Mar 2024 17:03:25 GMT, Alexey Ivanov wrote: >Instantiating JFileChooser on EDT adds complexity for no benefit. The >JFileChooser is still accessed concurrently from several threads, including >Swing internal thread. But still, this is a common requirement to create UI components on EDT, and that will replicate more or less how it will work in the application. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1536222501
Integrated: 8328194: Add a test to check default rendering engine
On Thu, 14 Mar 2024 20:13:58 GMT, Sergey Bylokhov wrote: > This is a request to forward port the test added by the > [JDK-8241307](https://bugs.openjdk.org/browse/JDK-8241307) with inverted > condition - jdk jdk11 the marlin should be used by default. This pull request has now been integrated. Changeset: c013fa18 Author: Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/c013fa18119bbd2e355d5c0d13cd8c172892800a Stats: 42 lines in 1 file changed: 42 ins; 0 del; 0 mod 8328194: Add a test to check default rendering engine Reviewed-by: prr, tr - PR: https://git.openjdk.org/jdk/pull/18313
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 15 Mar 2024 12:02:43 GMT, Alexey Ivanov wrote: >> test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java >> line 99: >> >>> 97: createFiles(temp); >>> 98: >>> 99: final JFileChooser fc = new JFileChooser(temp.toFile()); >> >> it should be created on EDT, just in case. > > I don't think it matters here… The test does not rely on even processing on > EDT, no state of `JFileChooser` is modified except for calling > `rescanCurrentDirectory`. Technically, I am not allowed to do so either. But > if I call `rescanCurrentDirectory` from EDT only, the test becomes void. I think it will fire the property change events via firePropertyChange from the constructor and it is better to do that on EDT. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1529545568
Re: RFR: 8325179: Race in BasicDirectoryModel.validateFileCache [v4]
On Fri, 15 Mar 2024 14:50:12 GMT, Alexey Ivanov wrote: >> Ensure access to the `filesLoader` field of `BasicDirectoryModel` is >> synchronized. >> >> Without synchronization, a thread checks if `filesLoader` is not null and >> creates a new `FilesLoader` thread. If the thread is pre-empted between >> these two operations, another thread or even several threads can see the >> `null` value and create new `FilesLoader` threads. >> >> The same way, `BasicDirectoryModel.invalidateFileCache` needs to be >> synchornized to interrupt the current `filesLoader` and assign `null`. >> >> This bug allows reproducing `ConcurrentModificationException` seen in >> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and >> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) using the test in >> PR #18109. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Increment fetch ID to invalidate pending DoChangeContents Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18111#pullrequestreview-1944748397
Re: RFR: 8328194: Add a test to check default rendering engine
On Sat, 16 Mar 2024 21:32:02 GMT, Phil Race wrote: > We don't have any others in JDK 23, so I am not sure what this test does for > us. Right now it is not useful, but that is the same story we had in OpenJDK 8 - only the one engine for years, and then the engine "accidentally" changed to marlin. This test will help to catch such bugs. - PR Comment: https://git.openjdk.org/jdk/pull/18313#issuecomment-2002368655
RFR: 8328194: Add a test to check default rendering engine
This is a request to forward port the test added by the [JDK-8241307](https://bugs.openjdk.org/browse/JDK-8241307) with inverted condition - jdk jdk11 the marlin should be used by default. - Commit messages: - Create DefaultRenderingEngine.java Changes: https://git.openjdk.org/jdk/pull/18313/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18313=00 Issue: https://bugs.openjdk.org/browse/JDK-8328194 Stats: 42 lines in 1 file changed: 42 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18313.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18313/head:pull/18313 PR: https://git.openjdk.org/jdk/pull/18313
Re: RFR: 8325179: Race in BasicDirectoryModel.validateFileCache [v3]
On Wed, 6 Mar 2024 11:27:08 GMT, Alexey Ivanov wrote:
>> Ensure access to the `filesLoader` field of `BasicDirectoryModel` is
>> synchronized.
>>
>> Without synchronization, a thread checks if `filesLoader` is not null and
>> creates a new `FilesLoader` thread. If the thread is pre-empted between
>> these two operations, another thread or even several threads can see the
>> `null` value and create new `FilesLoader` threads.
>>
>> The same way, `BasicDirectoryModel.invalidateFileCache` needs to be
>> synchornized to interrupt the current `filesLoader` and assign `null`.
>>
>> This bug allows reproducing `ConcurrentModificationException` seen in
>> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and
>> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) using the test in
>> PR #18109.
>
> Alexey Ivanov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Store the size of fileCache inside synchronized block
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 534:
> 532: @Override
> 533: public void run() {
> 534: if (fetchID.get() != fid) {
How it will work if the "FilesLoader" was created already but before loading
the files the "invalidateFileCache" was called, before the patch we stop
execution due to "doFire=false".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18111#discussion_r1525420964
Re: RFR: 8325179: Race in BasicDirectoryModel.validateFileCache [v3]
On Tue, 5 Mar 2024 11:04:30 GMT, Alexey Ivanov wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
>> line 101:
>>
>>> 99: */
>>> 100: public synchronized void invalidateFileCache() {
>>> 101: if (filesLoader != null) {
>>
>> This is a public API method, can we minimize the change and use
>> "synchronized (this)" instead?
>
> The `synchronized` modifier is not part of public API specification, it's not
> shown in the generated Javadoc. Anyway, I updated the code to use
> `synchronized (this)` block inside the method.
I thought that is checked by the sigtests. at least previously the removing of
synchronized keyword requires the CSR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18111#discussion_r1525408186
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel
On Mon, 4 Mar 2024 15:52:45 GMT, Alexey Ivanov wrote:
> I'm adding a regression test for
> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and
> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091); it's also a
> regression test for
> [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690).
>
> I referenced this test in PR #17462 in [this
> comment](https://github.com/openjdk/jdk/pull/17462#issuecomment-1914844026).
> I fine-tuned the test since that time.
>
> The test doesn't fail all the time without the fix for JDK-8323670, however,
> it fails sometimes. If you run the test several times, it will likely fail
> _without the fix_.
>
> For me, the test fails about 10 times from 40 runs in the CI. It fails on
> macOS more frequently than on Linux.
>
> When the test passes, it usually completes in 5 minutes.
>
> **How the test works**
>
> The test creates a temporary directory in the current directory and creates a
> number of files in it. (The number of files is controlled by
> `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the
> temporary directory.
>
> The test starts several _scanner_ threads, the number is controlled by
> `NUMBER_OF_THREADS`, which repeatedly call
> `fileChooser.rescanCurrentDirectory()`. This results in calling
> `BasicDirectoryModel.validateFileCache` which starts a background thread —
> "Basic L File Loading Thread" — to enumerate the files.
>
> A timer is used to create new files in the directory that the file chooser is
> using.
>
> After enumerating the files, the File Loading Thread posts an event to EDT.
> The event updates `fileCache` and fires a `ListDataEvent`.
>
> If the File Loading Thread is iterating over `fileCache` using `Iterator`
> (when `fileCache.subList` or `fileCache.equals` is running; or a new `Vector`
> instance is created from a `fileCache` or its sublist) and `fileCache` is
> being updated on EDT, then `ConcurrentModificationException` is thrown.
>
> On Linux and on _headless_ macOS, `ShellFolder.invoke` is executed in the
> caller, which makes it easier to reproduce the issue. Because of
> [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179), there are several
> File Loading Threads, which also helps to reproduce the issue.
>
> On _headful_ macOS, the `BasicDirectoryModel` is not used, so the test does
> not reproduce the issue.
>
> On Windows, the test does not fail or fails with `OutOfMemoryError`. It is
> because all the File Loading Threads are serialised on the COM thread,
> `ShellFolder.invoke` submits the task to the COM thread and waits for it to
> complete. The chance of updating `fileCache` whil...
test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java
line 1:
> 1: import java.io.IOException;
the header is missing.
test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java
line 99:
> 97: createFiles(temp);
> 98:
> 99: final JFileChooser fc = new JFileChooser(temp.toFile());
it should be created on EDT, just in case.
test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java
line 211:
> 209: private static void deleteFile(final Path file) {
> 210: try {
> 211: Files.delete(file);
Should we try to delete all files first and then throw an exception if any?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1525401885
PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1525401638
PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1525403026
Re: RFR: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS [v2]
On Thu, 7 Mar 2024 12:36:05 GMT, Dmitry Markov wrote: >> Updated several tests to avoid potential failure with recent LCMS update and >> non-LCMS generated profile. > > Dmitry Markov has updated the pull request incrementally with one additional > commit since the last revision: > > Tolerance update Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18097#pullrequestreview-1925957557
Re: RFR: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS [v2]
On Thu, 7 Mar 2024 12:32:09 GMT, Dmitry Markov wrote: >> Actually the purpose of these particular tests is validation of the quality >> of the built-in color profiles. > > I agree, I reverted almost all changes except the ones in tolerance for > non-LCMS profiles. That values need to be updated to make the test pass on > JDKs (e.g. JDK 8u, etc) where non-LCMS profiles are used Could you please clarify what is the root cause of the problem. I assume you did not update the profiles itself, so what is the problem in the new lcms library? >It used to be OK until recent LCMS update where the CMM started to keep >original profile ID instead of writing ‘lcms’ to returned header. Do you mean that previously we always use lcms thresholds even for kcms related profiles? - PR Review Comment: https://git.openjdk.org/jdk/pull/18097#discussion_r1517039982
Re: RFR: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS
On Tue, 5 Mar 2024 00:58:42 GMT, Sergey Bylokhov wrote: >> Updated several tests to avoid potential failure with recent LCMS update and >> non-LCMS generated profile. > > test/jdk/sun/java2d/cmm/ColorConvertOp/ColConvCCMTest.java line 63: > >> 61: 10.5, // GRAY >> 62: 215.0, // PYCC >> 63: 56.0// CIEXYZ > > All this change will delete all information about "old and good" values and > start to use lcms thresholds for all profiles? I think it should be the > opposite - use the good data for all except lcms(or any other not that good > as jdk8 profiles), like we do it now. Actually the purpose of these particular tests is validation of the quality of the built-in color profiles. - PR Review Comment: https://git.openjdk.org/jdk/pull/18097#discussion_r1512099541
Re: RFR: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS
On Sun, 3 Mar 2024 09:01:49 GMT, Dmitry Markov wrote: > Updated several tests to avoid potential failure with recent LCMS update and > non-LCMS generated profile. test/jdk/sun/java2d/cmm/ColorConvertOp/ColConvCCMTest.java line 63: > 61: 10.5, // GRAY > 62: 215.0, // PYCC > 63: 56.0// CIEXYZ All this change will delete all information about "old and good" values and start to use lcms thresholds for all profiles? I think it should be the opposite - use the good data for all except lcms(or any other not that good as jdk8 profiles), like we do it now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18097#discussion_r1511992175
Re: RFR: 8325179: Race in BasicDirectoryModel.validateFileCache
On Mon, 4 Mar 2024 20:21:30 GMT, Alexey Ivanov wrote:
> Ensure access to the `filesLoader` field of `BasicDirectoryModel` is
> synchronized.
>
> Without synchronization, a thread checks if `filesLoader` is not null and
> creates a new `FilesLoader` thread. If the thread is pre-empted between these
> two operations, another thread or even several threads can see the `null`
> value and create new `FilesLoader` threads.
>
> The same way, `BasicDirectoryModel.invalidateFileCache` needs to be
> synchornized to interrupt the current `filesLoader` and assign `null`.
>
> This bug allows reproducing `ConcurrentModificationException` seen in
> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and
> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) using the test in
> PR #18109.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 101:
> 99: */
> 100: public synchronized void invalidateFileCache() {
> 101: if (filesLoader != null) {
This is a public API method, can we minimize the change and use "synchronized
(this)" instead?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18111#discussion_r1511986238
Re: RFR: 8187759: Background not refreshed when painting over a transparent JFrame [v4]
On Wed, 21 Feb 2024 04:27:13 GMT, Tejesh R wrote: >> That code was added by me to implement the shaped windows on macOS, that is >> the only platform we support the translucent backbuffer in RepaintManager. >> For the translucent backbuffer and even opaque component it is necessary to >> clear its content since the rendering inside of the components assumes the >> simple "fillrect" will clear the component which is not true if the >> composite is not Src. >> But probably at that place we always start rendering from the Window? in >> that case the usage of background is a right thing. I am not sure that the >> usage of the component background is a correct thing, and probably 0,0,0 >> should work better. >> >> I suggest to check that we do not fill the background twice, by the code >> added in the patch, and the code inside of the paintToOffscreen(). For >> example if there are the container and the component inside, if the >> component is not opaque and has no any content, will the container be fully >> visible? or we will see the background of the component(which should not be >> there). > > @mrserb Setting it to 0,0,0 is not working fine, the screen refresh is > happening. And also I don't see anything which might fill background twice. > It's better to move on with Component background only. sounds good then - PR Review Comment: https://git.openjdk.org/jdk/pull/17081#discussion_r1496941091
Re: RFR: 8325858: Replace Unsafe usage in XEmbeddingContainer with FFM API [v3]
On Wed, 21 Feb 2024 01:23:34 GMT, Phil Race wrote: > Embedded frames are for applets (dead) and JAWT and SWT. I don't see how any > of those could use this. yeah, I meant jawt, not eawt. SWT used some of the code directly from the native code, so they bypassed private access and modules. Might not be relevant anymore. - PR Comment: https://git.openjdk.org/jdk/pull/17846#issuecomment-1955727001
Re: RFR: 8325858: Replace Unsafe usage in XEmbeddingContainer with FFM API [v3]
On Tue, 20 Feb 2024 22:46:58 GMT, Phil Race wrote: >Anyone have ANY idea where or how this might be used ? I'd like to be sure >before I propose that. When the eawt was enhanced to support embedded frames all related non-public/internal code was not deleted - it was found that people used that and at that moment did not migrate to eawt(not sure that someone planned to do so since it worked). - PR Comment: https://git.openjdk.org/jdk/pull/17846#issuecomment-194558
Re: RFR: 8187759: Background not refreshed when painting over a transparent JFrame [v4]
On Tue, 20 Feb 2024 05:36:55 GMT, Tejesh R wrote: >> Since this is an intermediate buffer its content should not affect the >> rendering, so it should contain only the image rendered by the component >> itself. > > This how it is done in > [RepaintManager](https://github.com/openjdk/jdk/blob/69a11c7f7ea7c4195a8ee56391bdf04c75bd8156/src/java.desktop/share/classes/javax/swing/RepaintManager.java#L1711), > it uses background color of the component. That code was added by me to implement the shaped windows on macOS, that is the only platform we support the translucent backbuffer in RepaintManager. For the translucent backbuffer and even opaque component it is necessary to clear its content since the rendering iside of the components assumes the simple "fillrect" will clear the component which is not true if the composite is not Src. But probably at that place we always start rendering from the Window? in that case the usage of background is a right thing. I am not sure that the usage of the component background is a correct thing, and probably 0,0,0 should work better. I suggest to check that we do not fill the background twice, by the code added in the patch, and the code inside of the paintToOffscreen(). For example if there are the container and the component inside, if the component is not opaque and has no any content, will the container be fully visible? or we will see the background of the component(which should not be there). - PR Review Comment: https://git.openjdk.org/jdk/pull/17081#discussion_r1495342708
Re: RFR: 8187759: Background not refreshed when painting over a transparent JFrame [v4]
On Tue, 20 Feb 2024 05:17:04 GMT, Sergey Bylokhov wrote: >> Tejesh R has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Frame dispose moved into EDT > > src/java.desktop/share/classes/javax/swing/BufferStrategyPaintManager.java > line 252: > >> 250: >> g2d.setBackground(paintingComponent.getBackground()); >> 251: g2d.clearRect(x, y, w, h); >> 252: g2d.setBackground(oldBg); > > I wonder what color should we use to clear the background? Should it be > background color of the component, background color of the window, or > (0,0,0)? What the Swing components usually do in that case? Since this is an intermediate buffer its content should not affect the rendering, so it should contain only the image rendered by the component itself. - PR Review Comment: https://git.openjdk.org/jdk/pull/17081#discussion_r1495264220
Re: RFR: 8187759: Background not refreshed when painting over a transparent JFrame [v4]
On Mon, 12 Feb 2024 05:38:17 GMT, Tejesh R wrote: >> This is happening in linux where `BuffereStrategyPaintManager` is used to >> paint to offscreen. Here `bsg` bufferStrategy SunGraphics2D is used to paint >> to offscreen where the background is not refreshed, which does only clipping >> and then paints to offscreen. In order to handle the screen updated/clear >> the buffer, `setBackground` to component background color and `clearRect` >> against the clip area is used which solves the issue without causing any >> regression. >> CI is green for the fix and manual test is provided. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Frame dispose moved into EDT src/java.desktop/share/classes/javax/swing/BufferStrategyPaintManager.java line 252: > 250: g2d.setBackground(paintingComponent.getBackground()); > 251: g2d.clearRect(x, y, w, h); > 252: g2d.setBackground(oldBg); I wonder what color should we use to clear the background? Should it be background color of the component, background color of the window, or (0,0,0)? What the Swing components usually do in that case? - PR Review Comment: https://git.openjdk.org/jdk/pull/17081#discussion_r1495254466
Re: RFR: 8187759: Background not refreshed when painting over a transparent JFrame [v4]
On Mon, 12 Feb 2024 05:38:17 GMT, Tejesh R wrote: >> This is happening in linux where `BuffereStrategyPaintManager` is used to >> paint to offscreen. Here `bsg` bufferStrategy SunGraphics2D is used to paint >> to offscreen where the background is not refreshed, which does only clipping >> and then paints to offscreen. In order to handle the screen updated/clear >> the buffer, `setBackground` to component background color and `clearRect` >> against the clip area is used which solves the issue without causing any >> regression. >> CI is green for the fix and manual test is provided. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Frame dispose moved into EDT Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17081#pullrequestreview-1889509957
Re: RFR: 8325858: Replace Unsafe usage in XEmbeddingContainer with FFM API [v3]
On Wed, 14 Feb 2024 18:09:15 GMT, Per Minborg wrote: >> This PR proposes to remove the use of `Unsafe` in the class >> `XEmbeddingContainer ` and replace it with the supported FFM API. >> >> I tried to make this PR as small as possible while opening up for migration >> of other classes later on (such as `XEmbedServer` which can be modified >> similarly under a separate PR). >> >> There are also three drive-by fixes in this PR: >> * Moved JavaDocs for `XAtom` to its proper location and fixed two typos in >> the text >> * Declared a field in `XEmbeddingContainer` as `private final` >> * In `XAtom`, use a utility method `assertAtomInitialized()` instead of the >> current duplicated code >> >> Tested and passed tier1-5 > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Suppress restricted warning Did you check how this change affects the cold startup? I think until the problem described in [1] is not fixed we cannot use the FFM. [1] https://bugs.openjdk.org/browse/JDK-8313344 - PR Comment: https://git.openjdk.org/jdk/pull/17846#issuecomment-1953485948
Re: RFR: 8187759: Background not refreshed when painting over a transparent JFrame [v2]
On Tue, 6 Feb 2024 08:09:20 GMT, Tejesh R wrote: >> Since the fix was done for linux alone, I have restricted the test to it. >> Anyhow it works on windows and mac. > > Somehow the automated test is not working flawless in windows and mac. In > windows the image is getting stuck at center of white frame when its made > automatic and in mac CI machines are failing even though the white screen is > clear. Since the fix is specifically for linux I think we can leave the test > for linux alone. It is better to check why it fails there, it uses the public api only and it should work on all platforms. - PR Review Comment: https://git.openjdk.org/jdk/pull/17081#discussion_r1490328461
Re: RFR: 8323170 - j2dbench is using outdated javac source/target to be able to build by itself
On Sat, 3 Feb 2024 12:31:16 GMT, Jiří Vaněk wrote: > added workarounds so freshly built jdk can set its necessary source/taret > without touching ant/makefile. In addition it can set itself as > compiler/runner without putting itself on path. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17695#pullrequestreview-1881629477
Re: RFR: 8323170 - j2dbench is using outdated javac source/target to be able to build by itself
On Sat, 13 Jan 2024 20:17:06 GMT, Jiří Vaněk wrote: >Also, I'm not saying to bump source/target to something drastic like 20 or so, >I originally prosed 8 - which is the oldest live openjdk anyway. However >(unless something unexpected comes form >https://bugs.openjdk.org/browse/JDK-8323169) what in next jdks - from jdk23 >onwards I would suggest 11 as source target, which will again cover the period >of next LTS..and so on. Unfortunately, right now the most important thing to compare is the difference between JDK 7/8 and the JDK 11+, since there was a big code rework in 11 which caused a few-not-fixed regressions. - PR Comment: https://git.openjdk.org/jdk/pull/17303#issuecomment-1945252549
Re: RFR: 8322750: Test "api/java_awt/interactive/SystemTrayTests.html" failed because A blue ball icon is added outside of the system tray
On Thu, 15 Feb 2024 01:11:51 GMT, Alexander Zvegintsev
wrote:
> There is an issue displaying the xembed icons in the appIndicators area which
> are not displayed correctly with certain Gnome Shell versions.
> It was already fixed
> [externally](https://gitlab.gnome.org/3v1n0/gnome-shell/-/commit/20a81d786697f40880e81d867453b1bad9524ec1).
>
> However this is still a blocker on systems that have not received this fix,
> so this fix disables a SystemTray's support for Gnome Shell < 45 versions.
>
> Gnome Shell version detection has the following logic:
> * execute `/usr/bin/gnome-shell --version`
> * parse its output to extract the major version
> * disable the SystemTray support if the version < 45 or parsing failed for
> some reason
>
>
> The numbering convention changed with the introduction of Gnome Shell 40.
> The old numbering convention is also handled correctly(e.g. [3.38.1]
> (https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/d15f6c75b19be1e32ec24165f09b3e74afaf7395/NEWS#L1134)
> )
>
> This is a simplified fix to make it easier to backport it.
> The improved solution will be to [receive the ShellVersion property via the
> DBUS
> API](https://unix.stackexchange.com/questions/73212/how-to-get-the-gnome-version/73225#73225)
>
> Testing looks good:
> Oracle Linux 7.9, Gnome Shell 3.28.3
> Ubuntu 22.04, Gnome Shell 42.9
> Ubuntu 23.04, Gnome Shell 44.3
> Ubuntu 23.10, Gnome Shell 45.0
> Fedora 38, Gnome Shell 44.0
src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java line 286:
> 284: try {
> 285: Process process =
> 286: new ProcessBuilder("/usr/bin/gnome-shell", "--version")
How it will affect performance?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17860#discussion_r1490265908
Integrated: 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java
On Tue, 6 Feb 2024 03:10:36 GMT, Sergey Bylokhov wrote: > The ClipShapeTest test is split into 4 different tests which could be > executed in parallel. > > The execution time is changed from: > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java > 1 1 0 0 > > == > TEST SUCCESS > > Finished building target 'run-test' in configuration > 'macosx-aarch64-server-release' > > real 2m58.673s > user 3m8.847s > sys 0m11.845s > > > to: > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java > 4 4 0 0 > > == > TEST SUCCESS > > Finished building target 'run-test' in configuration > 'macosx-aarch64-server-release' > > real 1m17.752s > user 3m25.308s > sys 0m12.987s This pull request has now been integrated. Changeset: 6c7029ff Author:Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/6c7029ffd48186353fc1d2a03915386b5f386ae2 Stats: 38 lines in 1 file changed: 28 ins; 5 del; 5 mod 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java Reviewed-by: aivanov, shade - PR: https://git.openjdk.org/jdk/pull/17719
Re: RFR: 8309460: JScrollBar leaves behind clutter for non-integer UI scales [v2]
On Mon, 22 Jan 2024 18:10:27 GMT, Alexey Ivanov wrote: >Is this bug similar to >[JDK-8032219](https://bugs.openjdk.org/browse/JDK-8032219)? That bug was caused by mixing the vector-based API(lines) and rasters(fillRect). At that time we rendered the line outside of the component's bounds, and that part of the line was not cleared by the fillRect. So the fix changed how we render the lines(borders), to always render them inside the component. But in this case, it is unclear what is the actual problem. How it related to the "non-standard" fractional metrics(such as 2.3, 2.7) and why it works fine for 1.5 and 1.75. If we draw a border outside of the component that border should be fixed. If the clear/fillRect does not clear the component then we should fix the calculation of the bounds. Note the proposed patch just repaints the component using its bounds, so I assume the bounds are correct, the line is properly rendered inside of the component, and something different is broken. - PR Comment: https://git.openjdk.org/jdk/pull/17484#issuecomment-1935350083
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v11]
On Thu, 8 Feb 2024 20:28:06 GMT, Alisen Chung wrote: >> SunToolkit.java is trying to post an event on the TrayIcon appContext, but >> the TrayIcon was already removed by the test, causing an error. The fix is >> to make SunToolkit skip posting the event if appContext is null. The test is >> also updated to remove applet usage and use PassFailJFrame instead. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > added final to target Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17329#pullrequestreview-1871659985
Re: RFR: 8226990: GTK & Nimbus LAF: Tabbed pane's background color is not expected one when change the opaque checkbox.
On Tue, 6 Feb 2024 05:43:26 GMT, Abhishek Kumar wrote: > JTabbedPane's content area, tab area and tab background color are not as > expected when opaque is set to true or false. > The proposed fix is to handle the TabbedPane's background color in installed > LAFs. Manual test is added to support the fix and there is no regression > caused by the fix. > > Proposed fix is tested in Ubuntu 22.04 and Oracle linux. > > CI link is posted in JBS. Is this report the same as the bug reported in JDK-6462396? seems it is a duplicate? - PR Comment: https://git.openjdk.org/jdk/pull/17720#issuecomment-1935318362
Re: RFR: 8322239: [macos] a11y : java.lang.NullPointerException is thrown when focus is moved on the JTabbedPane
On Tue, 6 Feb 2024 20:13:01 GMT, Alexander Zuev wrote:
> Add null check for the Aqua LnF situation when tab is hidden die to the tabs
> overflow.
src/java.desktop/share/classes/javax/swing/JTabbedPane.java line 2340:
> 2338:
> 2339: public Point getLocationOnScreen() {
> 2340: Point parentLocation = parent.getLocationOnScreen();
possibly the "parent.getLocationOnScreen()" should be wrapped by the "try/catch
IllegalComponentStateException"? If some parent is not visible then that
exception will be thrown, but per the spec, the null should be returned.
I think this could be covered by the test since the change is in the public
shared code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17736#discussion_r1483846936
Re: RFR: 8325309: Amend "Listeners and Threads" in AWTThreadIssues.html
On Tue, 6 Feb 2024 08:39:01 GMT, Alexey Ivanov wrote: > Replace _“effect”_ with _“affect”_¹ because the meaning in the phrase “…the > changes only _effect_ subsequent notification” is to “have an effect on.” > > Add a comma to separate a conditional clause from the main one. > > Use `` instead of ``: the example should be either in-line or in a > separate paragraph. > > Amend grammar: “a key _listeners_ is added.” Here, the usage of the > indefinite article and the singular form of _“to be”_ implies the subject is > singular. > > ¹ For reference, see [‘Affect’ vs. > ‘Effect’](https://www.merriam-webster.com/grammar/affect-vs-effect-usage-difference). Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17725#pullrequestreview-1871636593
Re: RFR: 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java [v2]
On Thu, 8 Feb 2024 11:12:29 GMT, Alexey Ivanov wrote: > I ran the test in our CI and I didn't notice an improvement. On the other > hand, having separate test ids is more flexible. I have retested by the complete jdk_desktop group of tests: Base: real6m57.901s user123m45.204s sys 9m16.735s Fix: real4m23.600s user126m35.078s sys 8m50.104s So it might depend on the system and used concurrency. - PR Comment: https://git.openjdk.org/jdk/pull/17719#issuecomment-1935234631
Re: RFR: 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java [v3]
> The ClipShapeTest test is split into 4 different tests which could be > executed in parallel. > > The execution time is changed from: > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java > 1 1 0 0 > > == > TEST SUCCESS > > Finished building target 'run-test' in configuration > 'macosx-aarch64-server-release' > > real 2m58.673s > user 3m8.847s > sys 0m11.845s > > > to: > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java > 4 4 0 0 > > == > TEST SUCCESS > > Finished building target 'run-test' in configuration > 'macosx-aarch64-server-release' > > real 1m17.752s > user 3m25.308s > sys 0m12.987s Sergey Bylokhov has updated the pull request incrementally with one additional commit since the last revision: Update ClipShapeTest.java - Changes: - all: https://git.openjdk.org/jdk/pull/17719/files - new: https://git.openjdk.org/jdk/pull/17719/files/67e3d2c5..423a2e60 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17719=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17719=01-02 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17719/head:pull/17719 PR: https://git.openjdk.org/jdk/pull/17719
Re: RFR: 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java [v2]
On Thu, 8 Feb 2024 06:04:16 GMT, Sergey Bylokhov wrote: >> The ClipShapeTest test is split into 4 different tests which could be >> executed in parallel. >> >> The execution time is changed from: >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java >> 1 1 0 0 >> >> == >> TEST SUCCESS >> >> Finished building target 'run-test' in configuration >> 'macosx-aarch64-server-release' >> >> real 2m58.673s >> user 3m8.847s >> sys 0m11.845s >> >> >> to: >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java >> 4 4 0 0 >> >> == >> TEST SUCCESS >> >> Finished building target 'run-test' in configuration >> 'macosx-aarch64-server-release' >> >> real 1m17.752s >> user 3m25.308s >> sys 0m12.987s > > Sergey Bylokhov has updated the pull request incrementally with one > additional commit since the last revision: > > Update ClipShapeTest.java duplicated comments are reworked - PR Comment: https://git.openjdk.org/jdk/pull/17719#issuecomment-1933442088
Re: RFR: 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java [v2]
> The ClipShapeTest test is split into 4 different tests which could be > executed in parallel. > > The execution time is changed from: > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java > 1 1 0 0 > > == > TEST SUCCESS > > Finished building target 'run-test' in configuration > 'macosx-aarch64-server-release' > > real 2m58.673s > user 3m8.847s > sys 0m11.845s > > > to: > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java > 4 4 0 0 > > == > TEST SUCCESS > > Finished building target 'run-test' in configuration > 'macosx-aarch64-server-release' > > real 1m17.752s > user 3m25.308s > sys 0m12.987s Sergey Bylokhov has updated the pull request incrementally with one additional commit since the last revision: Update ClipShapeTest.java - Changes: - all: https://git.openjdk.org/jdk/pull/17719/files - new: https://git.openjdk.org/jdk/pull/17719/files/333096ac..67e3d2c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17719=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17719=00-01 Stats: 33 lines in 1 file changed: 8 ins; 20 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17719/head:pull/17719 PR: https://git.openjdk.org/jdk/pull/17719
RFR: 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java
The ClipShapeTest test is split into 4 different tests which could be executed in parallel. The execution time is changed from: == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java 1 1 0 0 == TEST SUCCESS Finished building target 'run-test' in configuration 'macosx-aarch64-server-release' real2m58.673s user3m8.847s sys 0m11.845s to: == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java 4 4 0 0 == TEST SUCCESS Finished building target 'run-test' in configuration 'macosx-aarch64-server-release' real1m17.752s user3m25.308s sys 0m12.987s - Commit messages: - 8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java Changes: https://git.openjdk.org/jdk/pull/17719/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17719=00 Issue: https://bugs.openjdk.org/browse/JDK-8318603 Stats: 36 lines in 1 file changed: 33 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17719/head:pull/17719 PR: https://git.openjdk.org/jdk/pull/17719
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v10]
On Thu, 1 Feb 2024 20:52:14 GMT, Alisen Chung wrote: >> SunToolkit.java is trying to post an event on the TrayIcon appContext, but >> the TrayIcon was already removed by the test, causing an error. The fix is >> to make SunToolkit skip posting the event if appContext is null. The test is >> also updated to remove applet usage and use PassFailJFrame instead. > > Alisen Chung has updated the pull request incrementally with two additional > commits since the last revision: > > - remove change in SunToolkit > - correct null change src/java.desktop/macosx/classes/sun/lwawt/macosx/CTrayIcon.java line 176: > 174: > 175: LWCToolkit.targetDisposedPeer(target, this); > 176: target = null; to make sure it is not changed in some other place you can merk the target field as final. - PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1479107367
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v9]
On Thu, 1 Feb 2024 16:46:18 GMT, Alisen Chung wrote: >> SunToolkit.java is trying to post an event on the TrayIcon appContext, but >> the TrayIcon was already removed by the test, causing an error. The fix is >> to make SunToolkit skip posting the event if appContext is null. The test is >> also updated to remove applet usage and use PassFailJFrame instead. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > remove comment, remove null check in updateNativeImage src/java.desktop/macosx/classes/sun/lwawt/macosx/CTrayIcon.java line 221: > 219: setNativeImage(ptr, imagePtr, imageAutoSize, > useTemplateImages); > 220: }); > 221: }); you deleted the wrong null check, the correct one is below see target == null src/java.desktop/share/classes/sun/awt/SunToolkit.java line 2: > 1: /* > 2: * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights > reserved. This can be discarded. - PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1475056981 PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1475057307
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v8]
On Thu, 1 Feb 2024 08:48:29 GMT, Sergey Bylokhov wrote: >> Alisen Chung has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - Merge branch 'master' of https://github.com/openjdk/jdk into 8316931 >> - remove null check in SunToolkit, remove setting target to null in >> CTrayIcon >> - Merge branch 'master' of https://github.com/openjdk/jdk into 8316931 >> - added suggested changes, moved test back into folder >> - added suggested changes, moved test back into folder >> - used jtreg.SkippedException, updated copyright years >> - removed extra newlines, moved test out of folder >> - spacing >> - updated test title, copyright year, removed redundant check >> - removed applet usage in test, fixed event posted on wrong app context bug > > src/java.desktop/macosx/classes/sun/lwawt/macosx/CTrayIcon.java line 176: > >> 174: >> 175: LWCToolkit.targetDisposedPeer(target, this); >> 176: //target = null; > > Why do you commented out this line? just delete it and mark the target as > final. you can also delete the null check(target == null) added by this fix: https://cr.openjdk.org/~mhalder/8207938/webrev.01/src/java.desktop/macosx/classes/sun/lwawt/macosx/CTrayIcon.java.sdiff.html BTW that bug looks like had the same root cause. - PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1474050567
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v8]
On Sat, 27 Jan 2024 05:57:51 GMT, Alisen Chung wrote: >> SunToolkit.java is trying to post an event on the TrayIcon appContext, but >> the TrayIcon was already removed by the test, causing an error. The fix is >> to make SunToolkit skip posting the event if appContext is null. The test is >> also updated to remove applet usage and use PassFailJFrame instead. > > Alisen Chung has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into 8316931 > - remove null check in SunToolkit, remove setting target to null in CTrayIcon > - Merge branch 'master' of https://github.com/openjdk/jdk into 8316931 > - added suggested changes, moved test back into folder > - added suggested changes, moved test back into folder > - used jtreg.SkippedException, updated copyright years > - removed extra newlines, moved test out of folder > - spacing > - updated test title, copyright year, removed redundant check > - removed applet usage in test, fixed event posted on wrong app context bug src/java.desktop/macosx/classes/sun/lwawt/macosx/CTrayIcon.java line 176: > 174: > 175: LWCToolkit.targetDisposedPeer(target, this); > 176: //target = null; Why do you commented out this line? just delete it and mark the target as final. - PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1474034815
Re: RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException [v3]
On Tue, 30 Jan 2024 13:47:05 GMT, Tejesh R wrote: >> Suggested fix [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) >> also created concurrent exception intermittently (monthly once/quarterly >> once) on CI system. The issue was not able to be reproduced yet, hence >> proposing an alternative fix which uses iterators to compare the List. >> CI testing shows green. > > Tejesh R has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Review updates - Indentatioins and unused imports > - Merge branch 'master' of https://git.openjdk.java.net/jdk into > branch_8323670 > - Revert fix 8307091 + Synchronised filecache > - Merge branch 'master' of https://git.openjdk.java.net/jdk into > branch_8323670 > - Fix Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17462#pullrequestreview-1855864492
Re: RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException
On Fri, 19 Jan 2024 07:24:14 GMT, Tejesh R wrote: > I don't think we are able to trace it out, since the issue intermittent and > previously I had made a copy of the vector list before checking for equality > of the list. There was again an issue in the code which I used to copy to a > temporary vector. So now instead of using` AbstractList.equals` I'm using > iterators and comparing every element within synchronized method. I think we should start investigating this one, probably by adding special delays/asserts into the JDK to track down on what threads the data is modified and used. - PR Comment: https://git.openjdk.org/jdk/pull/17462#issuecomment-1912802507
Integrated: 8324347: Enable "maybe-uninitialized" warning for FreeType 2.13.1
On Tue, 23 Jan 2024 00:58:27 GMT, Sergey Bylokhov wrote: > The next bug in freetype was fixed upstream and fix already merged to OpenJDK: > https://gitlab.freedesktop.org/freetype/freetype/-/issues/1245 > So now we can revert the workaround in the JDK: > https://bugs.openjdk.org/browse/JDK-8313576 > > Tested with "--with-freetype=bundled", "--with-freetype=system" and w/o > option on the system where the bug was reproduced. This pull request has now been integrated. Changeset: 781f368d Author:Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/781f368d421a94857929e4168974f43e890637d8 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8324347: Enable "maybe-uninitialized" warning for FreeType 2.13.1 Reviewed-by: erikj, azvegint, jwaters, aivanov - PR: https://git.openjdk.org/jdk/pull/17525
RFR: 8324347: Enable "maybe-uninitialized" warning for FreeType 2.13.1
The next bug in freetype was fixed upstream and fix already merged to OpeenJDK: https://gitlab.freedesktop.org/freetype/freetype/-/issues/1245 So now we can revert the workaround in the JDK: https://bugs.openjdk.org/browse/JDK-8313576 Tested with "--with-freetype=bundled", "--with-freetype=system" and w/o option on the system where the bug was reproduced. - Commit messages: - Revert "8313576: GCC 7 reports compiler warning in bundled freetype 2.13.0" Changes: https://git.openjdk.org/jdk/pull/17525/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17525=00 Issue: https://bugs.openjdk.org/browse/JDK-8324347 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17525/head:pull/17525 PR: https://git.openjdk.org/jdk/pull/17525
Re: RFR: 8233177: JTextField's size is computed incorrectly when it contains Indic or Thai characters
On Tue, 23 Jan 2024 08:18:33 GMT, Prasanta Sadhukhan
wrote:
> Existing regression test is failing because textfield height is not as per
> test's expectation..Seems like the indic character being tried to render is
> not being loaded (probably because of missing glyphs) leading to 0
> preferredSpan from
> [BoxView](https://github.com/openjdk/jdk/blame/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/java.desktop/share/classes/javax/swing/text/BoxView.java#L545-L552)
> which is superclass for `i18nFieldVIew`, the field view for Indic characters.
> Tried block character in the test which is now being loaded properly leading
> to correct height..
Can you please check that the updated test can verify the initial bug? When I
run the reproducer from the JBS for that bug on JDK8+macOS then I can reproduce
it. But if I update the characters as suggested in this PR the bug stops
reproducing.
test/jdk/javax/swing/plaf/basic/BasicTextUI/8001470/bug8001470.java line 58:
> 56:
> 57: textField1 = new JTextField("\u2588");
> 58: textField2 = new JTextField("\u2588");
You do not need to test the same character twice
-
PR Comment: https://git.openjdk.org/jdk/pull/17528#issuecomment-1907252071
PR Review Comment: https://git.openjdk.org/jdk/pull/17528#discussion_r1464225554
Re: RFR: 8320692: Null icon returned for .exe without custom icon [v2]
On Thu, 18 Jan 2024 19:40:40 GMT, Alexander Zuev wrote: >> Replaced asserts with NullPointerException calls because outside of testing >> that would be more informative - i do not think many people running their >> applications with assertions in system libraries enabled; >> Added a code that will analyze the result of the getIcon and will fall back >> to the default icon for the file type retrieved from the ShellFolder; >> Added a test case made by Aleksei Ivanov. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Revert NPE to asserts > Move null check inside the loop so we do not retrieve extra > icons if we encounter a null one test/jdk/javax/swing/JFileChooser/FileSystemView/NoIconExeNPE.java line 264: > 262: FileSystemView fsv = FileSystemView.getFileSystemView(); > 263: // No NullPointerException is expected > 264: fsv.getSystemIcon(hello.toFile()); Maybe we should check that something useful is returned? at least non-null VS null value? - PR Review Comment: https://git.openjdk.org/jdk/pull/17475#discussion_r1461241572
Re: RFR: 8309460: JScrollBar leaves behind clutter for non-integer UI scales
On Fri, 19 Jan 2024 05:52:34 GMT, Prasanta Sadhukhan wrote: > Also, it might be the case the scrollbar methods like getUnitIncrement, > scrollBar.getValue scrollbar.setValue setThumbBounds sets or returns or uses > integer value despite scale being non-integer, so I guess repainting dirty > region should be done to refresh the artifacts But in the fix and in the places you pointed out we also use the int values, so why is the bug reproduced only when the float scale is used? - PR Comment: https://git.openjdk.org/jdk/pull/17484#issuecomment-1902463567
Re: RFR: 8323108: BufferedImage.setData(Raster) should not cast float and double values to integers [v2]
On Sat, 20 Jan 2024 11:46:12 GMT, Martin Desruisseaux wrote: > Updated the copyright years to 2024. The pre-submit tests are failing, but I > believe that it is because the branch is 8 months old. Should I commit a > merge from master? (a test on my local machine suggests that it fixes the > issues). Yes, you can merge the master. - PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1902459971
Re: RFR: 8323108: BufferedImage.setData(Raster) should not cast float and double values to integers
On Thu, 4 May 2023 10:14:10 GMT, Martin Desruisseaux wrote:
> The `BufferedImage` Javadoc does not mention any constraint about the data
> type. In practice, `BufferedImage` with floating point values can be rendered
> by Java2D as well as integers, provided that a compatible `ColorModel` was
> supplied at construction time. However calls to `setData(Raster)`
> unexpectedly cast floating point values to integers. For example sample value
> 0.8 become 0. This is demonstrated by the `SetData` test case in this pull
> request.
>
> An easy fix, which is proposed in this pull request, is to replace the whole
> `BufferedImage.setData(Raster)` method body by a simple call to
> `WritableRaster.setRect(Raster)`, which handles all `DataBuffer` types
> correctly. The method contracts documented in their respective Javadoc are
> compatible. Furthermore an examination of their source code shows that they
> are equivalent, except for the differences noted in the _Behavioural changes_
> section below.
>
> # Source code comparison
> For demonstrating that delegating to `WritableRaster.setRect(Raster)` would
> be equivalent to the current code, one can copy the method body and apply the
> following changes:
>
> * Remove `dx` parameter, replaced by 0.
> * Remove `dy` parameter, replaced by 0.
> * Rename `srcRaster` parameter as `r` (like in `BufferedImage`).
> * Rename `startY` variable as `i` (like in `BufferedImage`).
> * Rename `srcOffX` variable as `startX` (like in `BufferedImage`).
> * Rename `srcOffY` variable as `startY` (like in `BufferedImage`).
> * Replace `this.minX` by 0 and simplify.
> * Replace `this.minY` by 0 and simplify.
> * Remove the `switch` statement, keeping only the `TYPE_INT` case.
>
> Then compare with `BufferedImage.setData(Raster)`. The modified block of code
> from `WritableRaster` is:
>
>
> int width = r.getWidth();
> int height = r.getHeight();
> int startX = r.getMinX();
> int startY = r.getMinY();
>
> We can see that above code is identical to `BufferedImage`. The next modified
> block of code from `WritableRaster`:
>
>
> int dstOffX = startX;
> int dstOffY = startY;
>
> // Clip to this raster
> if (dstOffX < 0) {
> width += dstOffX;
> startX -= dstOffX;// = 0 because dstOffX == startX
> dstOffX = 0;
> }
> if (dstOffY < 0) {
> height += dstOffY;
> startY -= dstOffY;// = 0 because dstOffY == startY
> dstOffY = 0;
> }
> if (dstOffX+width > this.width) {
> width = this.width - dstOffX;
> }
> if (dstOffY+height > this.height) {
> height = this.height - dstOffY;
> }
> if (width <= 0 || height <= 0) {
> return;
> }
>
>
> Above is equivalent to the following code from `BufferedImage`, ex...
Marked as reviewed by serb (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/13797#pullrequestreview-1833501028
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v4]
On Fri, 19 Jan 2024 01:43:48 GMT, Sergey Bylokhov wrote:
>> Somehow we have null passed in.
>> So something looked up appcontext and got null.
>> Likely because some other code removed the app context from a map or
>> similar.
>> First step is to find exactly where that happened, then we can start to
>> reason about how we managed to still post an event.
>
> Note that it does not fail with NPE but instead this code trigger the bug:
>
> AppContext eventContext = targetToAppContext(event.getSource());
> if (eventContext != null && !eventContext.equals(appContext)) {
> throw new RuntimeException("Event posted on wrong app context : "
> + event);
> }
>
> The source of the event is TrayIcon, and the appconteext for it is non-null
> as well.
>
> This is the code where we post an event:
>
> @Override
> public void dispose() {
> dummyFrame.dispose();
>
> if (popup != null) {
> popup.removeNotify();
> }
>
> LWCToolkit.targetDisposedPeer(target, this);
> target = null;
>
> super.dispose();
> }
>
> private void postEvent(final AWTEvent event) {
> SunToolkit.executeOnEventHandlerThread(target, new Runnable() {
> public void run() {
> SunToolkit.postEvent(SunToolkit.targetToAppContext(target),
> event);
> }
> });
> }
>
>
> Note that we also call SunToolkit.targetToAppContext(target), and the target
> should be a TrayIcon and have a non-null appcontext, because later we
> successfully fetch it.
>
>
> So the problem here is not in the null appcontext, but that we set target to
> null in the peer in dispose(), at the time we still processing the events.
> - So we should check how all this should be synchronized, to prevent such
> race
> - Or we should not set target to null as on Windows
> - Or call dispose on EDT like on Linux.
I think we should set the target once and use it forever.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1458186579
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v4]
On Thu, 18 Jan 2024 23:06:07 GMT, Phil Race wrote:
>> I think the context became null because the component doesn't exist anymore
>
> Somehow we have null passed in.
> So something looked up appcontext and got null.
> Likely because some other code removed the app context from a map or similar.
> First step is to find exactly where that happened, then we can start to
> reason about how we managed to still post an event.
Note that it does not fail with NPE but instead this code trigger the bug:
AppContext eventContext = targetToAppContext(event.getSource());
if (eventContext != null && !eventContext.equals(appContext)) {
throw new RuntimeException("Event posted on wrong app context : " +
event);
}
The source of the event is TrayIcon, and the appconteext for it is non-null as
well.
This is the code where we post an event:
@Override
public void dispose() {
dummyFrame.dispose();
if (popup != null) {
popup.removeNotify();
}
LWCToolkit.targetDisposedPeer(target, this);
target = null;
super.dispose();
}
private void postEvent(final AWTEvent event) {
SunToolkit.executeOnEventHandlerThread(target, new Runnable() {
public void run() {
SunToolkit.postEvent(SunToolkit.targetToAppContext(target),
event);
}
});
}
Note that we also call SunToolkit.targetToAppContext(target), and the target
should be a TrayIcon and have a non-null appcontext, because later we
successfully fetch it.
So the problem here is not in the null appcontext, but that we set target to
null in the peer in dispose(), at the time we still processing the events.
- So we should check how all this should be synchronized, to prevent such race
- Or we should not set target to null, this is why the bug is not reproduced
on other platforms.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1458182051
Re: RFR: 6507038: Memory Leak in JTree / BasicTreeUI
On Wed, 17 Jan 2024 07:19:21 GMT, Prasanta Sadhukhan wrote: > When using a TreeCellRenterer which creates new components in > getTreeCellRendererComponent() in a JTree that is not visible, changes to the > nodes cause a memory leak. > When a node is changed, the Method getNodeDimensions() is called to calculate > the new dimensions for the node. In this method, > getTreeCellRendererComponent() is called to obtain the renderer component > (what else...) and [this component is added to > rendererPane](https://github.com/openjdk/jdk/blob/36f4b34f1953af736706ec67192204727808bc6c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java#L3283-L3284). > It is not removed from the rendererPane afterwards. > Only when the tree is painted, the paint() method does a removeAll on the > rendererPane [in this > code](https://github.com/openjdk/jdk/blob/36f4b34f1953af736706ec67192204727808bc6c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java#L1500) > > FIx is added to remove the components from rendererPane when the JTree UI is > changed/uninstalled only when tree is not visible since they are already > removed when tree is painted in paint() method.. can we trigger this memory leak by some new or existed tests? - PR Comment: https://git.openjdk.org/jdk/pull/17458#issuecomment-1899457212
Re: RFR: 8295804: javax/swing/JFileChooser/JFileChooserSetLocationTest.java failed with "setLocation() is not working properly" [v4]
On Tue, 16 Jan 2024 05:58:45 GMT, Tejesh R wrote: >> The issue is that the doc area (in respect to the screen height which is >> 768px) which is at the bottom was causing the `JFileChooser `to be placed >> slightly above the set location. Was able to reproduce in local machine with >> reference to the failure image provided in the CI logs. The suggested fix is >> to place the main Frame slightly above the center of the screen than setting >> at the center of the screen. Several CI runs were made and no issue found. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review fix Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17364#pullrequestreview-1830878417
Re: RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException
On Wed, 17 Jan 2024 12:55:52 GMT, Tejesh R wrote: > Suggested fix [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) also > created concurrent exception intermittently (monthly once/quarterly once) on > CI system. The issue was not able to be reproduced yet, hence proposing an > alternative fix which uses iterators to compare the List. > CI testing shows green. What is the code path which modifies the vector when we iterate it? - PR Comment: https://git.openjdk.org/jdk/pull/17462#issuecomment-1899421133
Re: RFR: 8309460: JScrollBar leaves behind clutter for non-integer UI scales
On Thu, 18 Jan 2024 11:09:12 GMT, Prasanta Sadhukhan wrote: > Lines are left behind when moving the scrollbar in the positive direction. > but are cleaned up on mouse release. > Additonally, with right arrow clicks too, the lines are left behind. > Seems like for mouseDragged and scrollByUnit, the dirty region of the > scrollbar is not repainted leading to artifacts > which is being done in this fix.. Why the bug is reproduced only if non-integer UI is used? - PR Comment: https://git.openjdk.org/jdk/pull/17484#issuecomment-1899402031
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v4]
On Wed, 17 Jan 2024 21:56:25 GMT, Alisen Chung wrote: >> I'm checking to see if removing executeOnEventHandlerThread solves the >> issue. In this test the source of the appcontext (trayIcon) is deleted via >> an actionPerformed listener, so probably the deletion of the trayIcon comes >> first then the mouseEvent is trying to be posted onto the trayIcon and >> throwing an error > > if the SunToolkit.executeOnEventHandlerThread is removed, then the test > doesn't throw an exception, but the icon doesn't disappear (test fails) then it should be checked what is going on there. The context should not just become null for the existing component. - PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1457110113
Re: RFR: 8323108: BufferedImage.setData(Raster) should not cast float and double values to integers
On Mon, 13 Nov 2023 14:53:22 GMT, Martin Desruisseaux wrote:
>> The `BufferedImage` Javadoc does not mention any constraint about the data
>> type. In practice, `BufferedImage` with floating point values can be
>> rendered by Java2D as well as integers, provided that a compatible
>> `ColorModel` was supplied at construction time. However calls to
>> `setData(Raster)` unexpectedly cast floating point values to integers. For
>> example sample value 0.8 become 0. This is demonstrated by the `SetData`
>> test case in this pull request.
>>
>> An easy fix, which is proposed in this pull request, is to replace the whole
>> `BufferedImage.setData(Raster)` method body by a simple call to
>> `WritableRaster.setRect(Raster)`, which handles all `DataBuffer` types
>> correctly. The method contracts documented in their respective Javadoc are
>> compatible. Furthermore an examination of their source code shows that they
>> are equivalent, except for the differences noted in the _Behavioural
>> changes_ section below.
>>
>> # Source code comparison
>> For demonstrating that delegating to `WritableRaster.setRect(Raster)` would
>> be equivalent to the current code, one can copy the method body and apply
>> the following changes:
>>
>> * Remove `dx` parameter, replaced by 0.
>> * Remove `dy` parameter, replaced by 0.
>> * Rename `srcRaster` parameter as `r` (like in `BufferedImage`).
>> * Rename `startY` variable as `i` (like in `BufferedImage`).
>> * Rename `srcOffX` variable as `startX` (like in `BufferedImage`).
>> * Rename `srcOffY` variable as `startY` (like in `BufferedImage`).
>> * Replace `this.minX` by 0 and simplify.
>> * Replace `this.minY` by 0 and simplify.
>> * Remove the `switch` statement, keeping only the `TYPE_INT` case.
>>
>> Then compare with `BufferedImage.setData(Raster)`. The modified block of
>> code from `WritableRaster` is:
>>
>>
>> int width = r.getWidth();
>> int height = r.getHeight();
>> int startX = r.getMinX();
>> int startY = r.getMinY();
>>
>> We can see that above code is identical to `BufferedImage`. The next
>> modified block of code from `WritableRaster`:
>>
>>
>> int dstOffX = startX;
>> int dstOffY = startY;
>>
>> // Clip to this raster
>> if (dstOffX < 0) {
>> width += dstOffX;
>> startX -= dstOffX;// = 0 because dstOffX == startX
>> dstOffX = 0;
>> }
>> if (dstOffY < 0) {
>> height += dstOffY;
>> startY -= dstOffY;// = 0 because dstOffY == startY
>> dstOffY = 0;
>> }
>> if (dstOffX+width > this.width) {
>> width = this.width - dstOffX;
>> }
>> if (dstOffY+height > this.height) {
>> height = this.height - dstOffY;
>> }
>> if (width <= 0 ||...
>
> Added a commit doing the replacement of handcrafter intersection in
> `WritableRaster.setRect(…)` by a call to `Rectangle.intersection(…)`. I
> couldn't run the jtreg tests however. I tried for a few hours, but I presume
> that I didn't configured correctly.
>
> Note that `setRect(…)` is overridden in `sun.awt.image.ByteInterleavedRaster`
> and `sun.awt.image.BytePackedRaster` for optimization purposes. Those methods
> use the same handcrafter intersection code than the one replaced by this
> commit. However I didn't updated the code in those subclasses. They should
> not be subject to underflow / overflow because users should not extend these
> classes and thus access to the protected fields. We could update those
> methods anyway for consistency, but I do not know what is the OpenJDK policy
> in that case.
>
> Regarding `BufferedImage.getData()`, I remember the issue. That method does
> not have the lost of precision issue documented in this pull request. But it
> would nevertheless benefit from the same change (delegate to
> `WritableRaster.setRect(…)`) for performance reasons, because of
> optimizations in above-cited `ByteInterleavedRaster` and `BytePackedRaster`
> subclasses.
@desruisseaux please enable the github actions in your fork, it seems the
"Pre-submit test status" was skipped.
https://github.com/openjdk/jdk/pull/13797/checks?check_run_id=18627782594
-
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1897610914
Integrated: 8323554: The typos in Javadoc: "@return if "
On Wed, 10 Jan 2024 21:35:56 GMT, Sergey Bylokhov wrote:
> The docs for the boolean methods are updated from this style:
>
> @return if the event has been consumed
>
> to this one
>
> @return {@code true} if the event has been consumed, otherwise
> {@code false}
>
> plus small cleanups here and there.
This pull request has now been integrated.
Changeset: de237fb0
Author:Sergey Bylokhov
URL:
https://git.openjdk.org/jdk/commit/de237fb058c74b87ea65a6020939264a5dfe3796
Stats: 123 lines in 14 files changed: 23 ins; 16 del; 84 mod
8323554: The typos in Javadoc: "@return if "
Reviewed-by: prr
-
PR: https://git.openjdk.org/jdk/pull/17357
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v4]
On Mon, 15 Jan 2024 11:33:31 GMT, Sergey Bylokhov wrote:
>> src/java.desktop/share/classes/sun/awt/SunToolkit.java line 450:
>>
>>> 448: if (appContext == null) {
>>> 449: return;
>>> 450: }
>>
>> I think we should check why the appcontext is null here, the event comes
>> from the native peer, so when the event was created the peer was there, if
>> the peer exists the target should have an appconext as well.
>>
>> Maybe the problem is that we try to jump on EDT twice? First time from
>> CTrayIcon.postEvent-> SunToolkit.executeOnEventHandlerThread and then one
>> more time via SunToolkit.postEvent().
>>
>> Can deletion of the executeOnEventHandlerThread solve the problem?
>
> Or maybe it is a lack of synchronization?
So what is the root cause of null context here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1454251585
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v4]
On Mon, 15 Jan 2024 11:27:43 GMT, Sergey Bylokhov wrote:
>> Alisen Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> used jtreg.SkippedException, updated copyright years
>
> src/java.desktop/share/classes/sun/awt/SunToolkit.java line 450:
>
>> 448: if (appContext == null) {
>> 449: return;
>> 450: }
>
> I think we should check why the appcontext is null here, the event comes from
> the native peer, so when the event was created the peer was there, if the
> peer exists the target should have an appconext as well.
>
> Maybe the problem is that we try to jump on EDT twice? First time from
> CTrayIcon.postEvent-> SunToolkit.executeOnEventHandlerThread and then one
> more time via SunToolkit.postEvent().
>
> Can deletion of the executeOnEventHandlerThread solve the problem?
Or maybe it is a lack of synchronization?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1452264759
Re: RFR: 8316931: [macos14] Test "java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.html" throws and exception on Mac OS 14(x64, aarch64) [v4]
On Fri, 12 Jan 2024 22:18:04 GMT, Alisen Chung wrote:
>> SunToolkit.java is trying to post an event on the TrayIcon appContext, but
>> the TrayIcon was already removed by the test, causing an error. The fix is
>> to make SunToolkit skip posting the event if appContext is null. The test is
>> also updated to remove applet usage and use PassFailJFrame instead.
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> used jtreg.SkippedException, updated copyright years
src/java.desktop/share/classes/sun/awt/SunToolkit.java line 450:
> 448: if (appContext == null) {
> 449: return;
> 450: }
I think we should check why the appcontext is null here, the event comes from
the native peer, so when the event was created the peer was there, if the peer
exists the target should have an appconext as well.
Maybe the problem is that we try to jump on EDT twice? First time from
CTrayIcon.postEvent-> SunToolkit.executeOnEventHandlerThread and then one more
time via SunToolkit.postEvent().
Can deletion of the executeOnEventHandlerThread solve the problem?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17329#discussion_r1452259491
Integrated: 4760025: sRGB conversions to and from CIE XYZ incorrect
On Fri, 12 Jan 2024 07:37:13 GMT, Sergey Bylokhov wrote: > Since the time the bug was reported color conversion accuracy for the sRGB > profile and specific values from the report increased by x100. This is a > request to add the code from the report as a test case. This pull request has now been integrated. Changeset: bdee968e Author: Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/bdee968e3e969784df130c75a5cf6a1d2847bd29 Stats: 50 lines in 1 file changed: 50 ins; 0 del; 0 mod 4760025: sRGB conversions to and from CIE XYZ incorrect Reviewed-by: prr, aivanov - PR: https://git.openjdk.org/jdk/pull/17388
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
On Fri, 12 Jan 2024 21:05:19 GMT, Alexey Ivanov wrote: > Currently, we have test and gold which use the same ColorSpace which could be > plain or wrapper for both cases. It looks confusing to me. That could be fine. We can create a gold image first, then parametrize the test method by different src/mid/dst and compare the result vs gold, so we will cover all possible combinations. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1451693480
Re: RFR: 8323170 - j2dbench is using outdated javac source/target to be able to build by itself
On Tue, 9 Jan 2024 09:31:30 GMT, Jiří Vaněk wrote: >I think j2dbench should go in spirit of in-tree jtregs and shold be ok for jdk >it is cloned from. Anybody who wishes to run it on older jdk, can clone it >from older jdk. The code in the j2dbench is maintained in a way that every new feature/tests added to it should work on the old JDK version, or be skipped. That helps to compare the performance of say JDK21 vs JDK8 with the new tests. and the old source/target helps to check that the new java lang features are not added to it. - PR Comment: https://git.openjdk.org/jdk/pull/17303#issuecomment-1890692486
Re: RFR: 8295804: javax/swing/JFileChooser/JFileChooserSetLocationTest.java failed with "setLocation() is not working properly" [v3]
On Fri, 12 Jan 2024 05:12:40 GMT, Tejesh R wrote: >> The issue is that the doc area (in respect to the screen height which is >> 768px) which is at the bottom was causing the `JFileChooser `to be placed >> slightly above the set location. Was able to reproduce in local machine with >> reference to the failure image provided in the CI logs. The suggested fix is >> to place the main Frame slightly above the center of the screen than setting >> at the center of the screen. Several CI runs were made and no issue found. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review fix test/jdk/javax/swing/JFileChooser/JFileChooserSetLocationTest.java line 216: > 214: > 215: int width = (int) screenSize.getWidth() / 2; > 216: int height = (int) screenSize.getHeight() / 2; This is not the width and height, this is the kind of point(x,y) where the frame will be located. - PR Review Comment: https://git.openjdk.org/jdk/pull/17364#discussion_r1451242135
