Re: RFR: 8344892: beans/finder/MethodFinder.findMethod incorrectly returns null
On Fri, 28 Feb 2025 14:48:45 GMT, Alexander Zvegintsev
wrote:
>> During the [JDK-8344891 Remove uses of sun.misc.ReflectUtil in
>> java.desktop](https://bugs.openjdk.org/browse/JDK-8344891) it was discovered
>> that `beans/finder/MethodFinder.findMethod' incorrectly returned null if a
>> signature was not in the cache and added it to the cache if it was already
>> there:
>>
>>
>> Method method = CACHE.get(signature);
>> return (method == null) ? method : CACHE.create(signature);
>>
>> This resulted in a [significant drop in
>> performance](https://bugs.openjdk.org/browse/JDK-8350573).
>>
>>
>>
>> Before ReflectUtil was removed, it worked by coincidence:
>>
>>
>> Method method = CACHE.get(signature);
>> return (method == null) || isPackageAccessible(method.getDeclaringClass()) ?
>> method : CACHE.create(signature);
>>
>>
>>
>>
>> 1. `Cache#get` non-obviously creates a value in Cache, this in turn allowed
>> us to avoid the NPE in the `(method == null) ||
>> isPackageAccessible(method.getDeclaringClass())` condition
>>
>>
>> https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126
>>
>> 2. `isPackageAccessible(method.getDeclaringClass())` was evaluated as true
>>
>> This is how we previously returned the cached value.
>>
>> ---
>>
>> So the solution is obvious:
>>
>>
>> Method method = CACHE.get(signature);
>> return (method != null) ? method : CACHE.create(signature);
>>
>>
>> Testing is green.
>
> src/java.desktop/share/classes/com/sun/beans/finder/MethodFinder.java line 81:
>
>> 79: try {
>> 80: Method method = CACHE.get(signature);
>> 81: return (method != null) ? method : CACHE.create(signature);
>
> This can be simplified to the `return CACHE.get(signature)`, since we know
> that the implementation creates the value in the get method:
>
> https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126
>
> However, I am not a fan of this solution as it is implementation dependent
> and this behavior is not documented.
I think this is confusing… If `CACHE.get(signature)` always returns a non-null
value, there's no need for the condition `method != null` — it's always true.
The javadoc for the `Cache` class specifies a `null` value can be returned:
https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L98-L99
Is it a bug in `Cache` class that it creates the value and adds it to the cache
when the value isn't found?
https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L126-L127
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23845#discussion_r1975832067
Re: RFR: 8350924: javax/swing/JMenu/4213634/bug4213634.java fails
On Fri, 28 Feb 2025 08:50:31 GMT, Prasanta Sadhukhan
wrote:
> Test fails in ubuntu OCI system..Made it more robust my adding
> waitForIdle/delay before commencing test..
> OCI system is ok with the fix.
test/jdk/javax/swing/JMenu/4213634/bug4213634.java line 71:
> 69: frame.setJMenuBar(mb);
> 70: JTextArea ta = new JTextArea("This test dedicated to Nancy and
> Kathleen, testers and bowlers extraordinaire\n\n\nNo exception means pass.");
> 71: frame.getContentPane().add("Center", ta);
The text area seems redundant, especially the text inside which looks like an
[Easter egg](https://en.wikipedia.org/wiki/Easter_egg_(media)).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23837#discussion_r1975942628
Re: RFR: 8344892: beans/finder/MethodFinder.findMethod incorrectly returns null
On Fri, 28 Feb 2025 14:41:41 GMT, Alexander Zvegintsev wrote: > During the [JDK-8344891 Remove uses of sun.misc.ReflectUtil in > java.desktop](https://bugs.openjdk.org/browse/JDK-8344891) it was discovered > that `beans/finder/MethodFinder.findMethod' incorrectly returned null if a > signature was not in the cache and added it to the cache if it was already > there: > > > Method method = CACHE.get(signature); > return (method == null) ? method : CACHE.create(signature); > > This resulted in a [significant drop in > performance](https://bugs.openjdk.org/browse/JDK-8350573). > > > > Before ReflectUtil was removed, it worked by coincidence: > > > Method method = CACHE.get(signature); > return (method == null) || isPackageAccessible(method.getDeclaringClass()) ? > method : CACHE.create(signature); > > > > > 1. `Cache#get` non-obviously creates a value in Cache, this in turn allowed > us to avoid the NPE in the `(method == null) || > isPackageAccessible(method.getDeclaringClass())` condition > > > https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126 > > 2. `isPackageAccessible(method.getDeclaringClass())` was evaluated as true > > This is how we previously returned the cached value. > > --- > > So the solution is obvious: > > > Method method = CACHE.get(signature); > return (method != null) ? method : CACHE.create(signature); > > > Testing is green. It's somewhat off-topic, yet I find it *worrying*. The `Cache.get` method starts with *unsynchronised access first*. https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L112-L114 It doesn't make sense… First of all, if the method can be accessed concurrently, which seems to be implied, the `table` field could be in an inconsistent state. This could result in hard-to-reproduce bugs. Secondly, the `Cache.get` invokes `removeStaleEntries` which has a `synchronized` block. That is the `get` method still requires explicit synchronisation. Having this in mind, *the unsynchronised access to the data structures doesn't gain anything*. Performance-wise, it would be better to wrap the call to `removeStaleEntries` and the required logic into `synchronized (this.queue)`. Thirdly, there's another call to `removeStaleEntries` in the `get` method, this time it's inside the `synchronized (this.queue)` block. - PR Comment: https://git.openjdk.org/jdk/pull/23845#issuecomment-2691348842
Re: RFR: 8344892: beans/finder/MethodFinder.findMethod incorrectly returns null
On Fri, 28 Feb 2025 18:44:14 GMT, Chen Liang wrote: >> I think this is confusing… If `CACHE.get(signature)` always returns a >> non-null value, there's no need for the condition `method != null` — it's >> always true. >> >> The javadoc for the `Cache` class specifies a `null` value can be returned: >> >> https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L98-L99 >> >> Is it a bug in `Cache` class that it creates the value and adds it to the >> cache when the value isn't found? >> >> https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L126-L127 > > Sorry for my last premature comment. > > I have looked at `Cache`, and believe `create` should be made protected to > indicate it is not intended to be called, but only overridden. > > The preexisting code called `create` to reuse the creation mechanism without > going through the cache - such usage is dubious. Per my experience, similar > conditional caches are better implemented by enclosing the logic into the > cache structure and use a common endpoint to access, so we have one universal > site to determine the conditionality of caching. In this case, such a > condition is better included in the `Cache` class itself, and the `create` > method should not be publicly exposed. Yes, I'd prefer `CACHE.get(signature)`. At the same time, I've got doubts that Alexander has… Although, strictly speaking, it is not “implementation dependent” because the `Cache` class is part of OpenJDK, and I think it's reasonable to depend on its implementation. - PR Review Comment: https://git.openjdk.org/jdk/pull/23845#discussion_r1975876473
Re: RFR: 8350924: javax/swing/JMenu/4213634/bug4213634.java fails
On Fri, 28 Feb 2025 08:50:31 GMT, Prasanta Sadhukhan
wrote:
> Test fails in ubuntu OCI system..Made it more robust my adding
> waitForIdle/delay before commencing test..
> OCI system is ok with the fix.
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/JMenu/4213634/bug4213634.java line 65:
> 63:
> 64: public static void createAndShowGUI() {
> 65: frame = new JFrame("TEST");
Could you use a more specific title?
test/jdk/javax/swing/JMenu/4213634/bug4213634.java line 88:
> 86: SwingUtilities.invokeAndWait(() -> {
> 87: frame.dispose();
> 88: if (!menu.isSelected()) {
Previously, the condition `!menu.isSelected()` was verified before the frame
was disposed of.
I think you should verify whether menu is select before disposing of the frame;
once the frame is disposed of, the state of components is not well-defined.
-
PR Review: https://git.openjdk.org/jdk/pull/23837#pullrequestreview-2651432599
PR Review Comment: https://git.openjdk.org/jdk/pull/23837#discussion_r1975805598
PR Review Comment: https://git.openjdk.org/jdk/pull/23837#discussion_r1975816414
RFR: 8224968: javax/swing/JColorChooser/Test6827032.java times out in Windows 10
**Problem:** The test hangs intermittently when run on Windows. (In some cases, handling the timeout takes 2 hours.) Thread dump shows no AWT threads, yet jtreg harness still waits for the test thread to finish, in particular it waits for [`StreamCopier`](https://github.com/openjdk/jtreg/blob/759946dedbafa423552851ecb98bc3bb8dcf30ec/src/share/classes/com/sun/javatest/regtest/exec/ProcessCommand.java#L279-L281). See `threaddump.log` attached to the bug and [my comment](https://bugs.openjdk.org/browse/JDK-8224968?focusedId=14757188&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14757188) for more details. **Fix, or Workaround:** Drag mouse for a short while. In my testing on CI, the `javax/swing/JColorChooser/Test6827032.java` failed on Windows *3 times* out of 6 runs with 20 repeats (`JTREG=REPEAT_COUNT=20`) without the fix. There have been no failures after the fix in 10 runs with 20 repeats. - Commit messages: - 8224968: javax/swing/JColorChooser/Test6827032.java times out in Windows 10 Changes: https://git.openjdk.org/jdk/pull/23846/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23846&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8224968 Stats: 33 lines in 2 files changed: 13 ins; 11 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/23846.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23846/head:pull/23846 PR: https://git.openjdk.org/jdk/pull/23846
Re: RFR: 8224968: javax/swing/JColorChooser/Test6827032.java times out in Windows 10
On Fri, 28 Feb 2025 16:06:10 GMT, Alexander Zvegintsev wrote: > > Thread dump shows no AWT threads, yet jtreg harness still waits for the > > test thread to finish, in particular it waits for > > [StreamCopier](https://github.com/openjdk/jtreg/blob/759946dedbafa423552851ecb98bc3bb8dcf30ec/src/share/classes/com/sun/javatest/regtest/exec/ProcessCommand.java#L279-L281). > > See threaddump.log attached to the bug and [my > > comment](https://bugs.openjdk.org/browse/JDK-8224968?focusedId=14757188&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14757188) > > for more details. > > This looks like we should file an issue against jtreg, as I don't see > anything wrong with the test either. Yes. I'd like to start an internal discussion first and then submit a bug. - PR Comment: https://git.openjdk.org/jdk/pull/23846#issuecomment-2691029272
Re: RFR: 8224968: javax/swing/JColorChooser/Test6827032.java times out in Windows 10
On Fri, 28 Feb 2025 15:42:51 GMT, Alexey Ivanov wrote: > **Problem:** > > The test hangs intermittently when run on Windows. (In some cases, handling > the timeout takes 2 hours.) > > Thread dump shows no AWT threads, yet jtreg harness still waits for the test > thread to finish, in particular it waits for > [`StreamCopier`](https://github.com/openjdk/jtreg/blob/759946dedbafa423552851ecb98bc3bb8dcf30ec/src/share/classes/com/sun/javatest/regtest/exec/ProcessCommand.java#L279-L281). > See `threaddump.log` attached to the bug and [my > comment](https://bugs.openjdk.org/browse/JDK-8224968?focusedId=14757188&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14757188) > for more details. > > **Fix, or Workaround:** > > Drag mouse for a short while. > > In my testing on CI, the `javax/swing/JColorChooser/Test6827032.java` failed > on Windows *3 times* out of 6 runs with 20 repeats (`JTREG=REPEAT_COUNT=20`) > without the fix. > > There have been no failures after the fix in 10 runs with 20 repeats. test/jdk/javax/swing/JColorChooser/Test6827032.java line 27: > 25: * @test > 26: * @key headful > 27: * @bug 6827032 [JDK-8197825](https://bugs.openjdk.org/browse/JDK-8197825) was a test stabilisation fix, we don't add such bugs into the `@bug` tag. - PR Review Comment: https://git.openjdk.org/jdk/pull/23846#discussion_r1975632755
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v6]
On Mon, 24 Feb 2025 16:46:05 GMT, Alexey Ivanov wrote:
>> Harshitha Onkar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> javadoc update
>
> test/jdk/java/awt/color/ICC_ProfileSetNullDataTest.java line 33:
>
>> 31: */
>> 32: public final class ICC_ProfileSetNullDataTest {
>> 33: private static final int[] colorSpace = new int [] {
>
> Suggestion:
>
> private static final int[] colorSpace = new int[] {
In fact, the syntax can be simplified further:
private static final int[] colorSpace = {
There's no need for explicit `new int[]`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1969639009
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v8]
On Tue, 25 Feb 2025 02:16:11 GMT, Harshitha Onkar wrote:
>> Built-in Profiles are singleton objects and if the user happens to modify
>> this shared profile object via setData() then the modified version of the
>> profile is returned each time the same built-in profile is requested via
>> getInstance().
>>
>> It is good to protect Built-in profiles from such direct modification by
>> adding BuiltIn profile check in `setData()` such that **only copies** of
>> Built-In profiles are allowed to be updated.
>>
>> With the proposed fix, if Built-In profile is updated using `.setData()` it
>> throws _**IAE - "BuiltIn profile cannot be modified"**_
>>
>> Fix consists of:
>> * `private boolean isBuiltIn = false` in ICC_Profile which is set to true
>> when BuiltIn profiles are created and false for non-builtIn profile.
>> * Converted BuiltInProfile from private interface to private static class
>> (with all the profile instances as static final)
>> * `isBuiltIn` flag is set to true when BuiltInProfile are loaded.
>> * JavaDoc update for setData() (CSR required)
>>
>> There are no restrictions on creating copies of BuiltIn profile and then
>> modifying it, but what is being restricted with this fix is - the direct
>> modification of the shared BuiltIn profile instance.
>>
>> Applications which need a modified version of the ICC Profile should instead
>> do the following:
>>
>>
>> byte[] profileData = ICC_Profile.getData() // get the byte array
>> represtation of BuiltIn- profile
>> ICCProfile newProfile = ICC_Profile.getInstance(profileData) // create a new
>> profile
>> newProfile.setData() // to modify and customize the profile
>>
>>
>> Following existing tests are modified to update a copy of Built-In profile.
>>
>> - java/awt/color/ICC_Profile/SetHeaderInfo.java
>> - java/awt/color/ICC_ProfileSetNullDataTest.java
>> - sun/java2d/cmm/ProfileOp/SetDataTest.java
>
> Harshitha Onkar has updated the pull request incrementally with one
> additional commit since the last revision:
>
> renamed flag to builtIn
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 115:
> 113: * This check is used in {@link #setData(int, byte[])} to prevent
> modifying
> 114: * Built-in profiles.
> 115: */
/**
* Set to {@code true} for {@code BuiltInProfile},
* remains {@code false} otherwise.
* This flag is used in {@link #setData(int, byte[])} to prevent modifying
* built-in profiles.
*/
There's no need to capitalise “Built-in”.
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 70:
> 68: throw new RuntimeException("Test Failed! IAE NOT
> thrown.");
> 69: } catch (IllegalArgumentException iae) {
> 70: if (!iae.getMessage().equalsIgnoreCase(EXCEPTION_MSG)) {
I think we can compare with regular `equals`. It is unlikely the message will
change by letter case only. The test uses exactly the same message as the one
that's thrown in `ICC_Profile.setData`, therefore the messages should match.
If the message is changed in the code for whatever reason, the message in the
test will need updating, which is expected, isn't it?
-
PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2640520613
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1969536537
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1969481870
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Mon, 24 Feb 2025 19:05:12 GMT, Harshitha Onkar wrote: > There are other way to create a profile - directly loading it form a file > (serialization) `ICC_Profile.getInstance( profiles>); `or using the byte array representation of the profile. So the > main intention here was not to tie ProfileDeferralInfo with isBuiltIn. Yes, there are. Does any other way create a **built-in profile**? No, it doesn't as far as I can see. Is this flexibility needed? I'd say, it's not needed… unless there's a very high chance there'll soon be introduced a new build-in ICC profile which is created in another way but `ICC_Profile(ProfileDeferralInfo)` constructor. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968271385
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 20:59:37 GMT, Alexey Ivanov wrote: > At this time, all the built-in profiles are created with > `ProfileDeferralInfo`. > > If `ProfileDeferralInfo` is used to create only built-in profiles, then > modifying `ICC_Profile(ProfileDeferralInfo pdi)` is enough. I've updated my branch with this change: `ICC_Profile(ProfileDeferralInfo pdi)` sets the `isBuiltIn` flag to `true`; `ICC_Profile(Profile p)` sets `isBuiltIn` to `false`. The diff to ‘master’: https://github.com/openjdk/jdk/compare/2a5d1da3355a4df3109ec42646b5b0cf088b4c2a..a34f16860c2f7d393f4a1fb57f46fba1a68a8412 Only `ICC_Profile.java` is modified, which is *minimal*. Only several lines are added. If there's another way to create a built-in profile in the future, this approach will need to updated to take into account the new way. We'll deal with it at that time in the future. The diff to your branch: https://github.com/openjdk/jdk/compare/8da82c10b5be3b92655abef95fd5be0c8b6eaa00..a34f16860c2f7d393f4a1fb57f46fba1a68a8412 - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968244204
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v6]
On Fri, 21 Feb 2025 00:50:45 GMT, Harshitha Onkar wrote: >> Built-in Profiles are singleton objects and if the user happens to modify >> this shared profile object via setData() then the modified version of the >> profile is returned each time the same built-in profile is requested via >> getInstance(). >> >> It is good to protect Built-in profiles from such direct modification by >> adding BuiltIn profile check in `setData()` such that **only copies** of >> Built-In profiles are allowed to be updated. >> >> With the proposed fix, if Built-In profile is updated using `.setData()` it >> throws _**IAE - "BuiltIn profile cannot be modified"**_ >> >> Fix consists of: >> * `private boolean isBuiltIn = false` in ICC_Profile which is set to true >> when BuiltIn profiles are created and false for non-builtIn profile. >> * Converted BuiltInProfile from private interface to private static class >> (with all the profile instances as static final) >> * `isBuiltIn` flag is set to true when BuiltInProfile are loaded. >> * JavaDoc update for setData() (CSR required) >> >> There are no restrictions on creating copies of BuiltIn profile and then >> modifying it, but what is being restricted with this fix is - the direct >> modification of the shared BuiltIn profile instance. >> >> Applications which need a modified version of the ICC Profile should instead >> do the following: >> >> >> byte[] profileData = ICC_Profile.getData() // get the byte array >> represtation of BuiltIn- profile >> ICCProfile newProfile = ICC_Profile.getInstance(profileData) // create a new >> profile >> newProfile.setData() // to modify and customize the profile >> >> >> Following existing tests are modified to update a copy of Built-In profile. >> >> - java/awt/color/ICC_Profile/SetHeaderInfo.java >> - java/awt/color/ICC_ProfileSetNullDataTest.java >> - sun/java2d/cmm/ProfileOp/SetDataTest.java > > Harshitha Onkar has updated the pull request incrementally with one > additional commit since the last revision: > > javadoc update src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 116: > 114: * BuiltInProfile. > 115: */ > 116: private boolean isBuiltIn = false; Should the field be named `builtIn` instead? `isBuiltIn` starts with a verb which is used for naming methods. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968245796
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Mon, 24 Feb 2025 17:01:56 GMT, Alexey Ivanov wrote: >>> And two @throws with the same type aren't allowed in javadoc, which means >>> you have to add the new reason to throw IllegalArgumentException into the >>> existing one. >> >> That's demonstrably untrue. This method already has two. >> >> Regarding the scenario above, this apparent and has all been considered >> already and the IAE is the preferred solution. >> >> And FWIW I don't think ISE is really any better. > >> > And two @throws with the same type aren't allowed in javadoc, which means >> > you have to add the new reason to throw IllegalArgumentException into the >> > existing one. >> >> That's demonstrably untrue. This method already has two. > > I was wrong here, the specification for > [`ICC_Profile.html.setData`](https://docs.oracle.com/en/java/javase/23/docs/api/java.desktop/java/awt/color/ICC_Profile.html#setData(int,byte[])) > correctly displays the two entries for `IllegalArgumentException`. The > generated javadoc for the proposed changeset contains three entries for IAE. > > I misremembered it, or it was a limitation in an IDE javadoc parser. > Regarding the scenario above, this apparent and has all been considered > already and the IAE is the preferred solution. > > And FWIW I don't think ISE is really any better. IllegalArgumentException implies the *argument* is invalid, but it is now thrown for a *valid* argument if the object *state* doesn't allow modifying the data. IllegalStateException conveys the object *state* is inappropriate to call this method. There's a semantic difference between the two, and I think IllegalStateException is better in this case. We can also discuss it with Joe in the CSR. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968073992
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 21:55:43 GMT, Phil Race wrote: > > And two @throws with the same type aren't allowed in javadoc, which means > > you have to add the new reason to throw IllegalArgumentException into the > > existing one. > > That's demonstrably untrue. This method already has two. I was wrong here, the specification for [`ICC_Profile.html.setData`](https://docs.oracle.com/en/java/javase/23/docs/api/java.desktop/java/awt/color/ICC_Profile.html#setData(int,byte[])) correctly displays the two entries for `IllegalArgumentException`. The generated javadoc for the proposed changeset contains three entries for IAE. I misremembered it, or it was a limitation in an IDE javadoc parser. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968059734
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v6]
On Fri, 21 Feb 2025 00:50:45 GMT, Harshitha Onkar wrote:
>> Built-in Profiles are singleton objects and if the user happens to modify
>> this shared profile object via setData() then the modified version of the
>> profile is returned each time the same built-in profile is requested via
>> getInstance().
>>
>> It is good to protect Built-in profiles from such direct modification by
>> adding BuiltIn profile check in `setData()` such that **only copies** of
>> Built-In profiles are allowed to be updated.
>>
>> With the proposed fix, if Built-In profile is updated using `.setData()` it
>> throws _**IAE - "BuiltIn profile cannot be modified"**_
>>
>> Fix consists of:
>> * `private boolean isBuiltIn = false` in ICC_Profile which is set to true
>> when BuiltIn profiles are created and false for non-builtIn profile.
>> * Converted BuiltInProfile from private interface to private static class
>> (with all the profile instances as static final)
>> * `isBuiltIn` flag is set to true when BuiltInProfile are loaded.
>> * JavaDoc update for setData() (CSR required)
>>
>> There are no restrictions on creating copies of BuiltIn profile and then
>> modifying it, but what is being restricted with this fix is - the direct
>> modification of the shared BuiltIn profile instance.
>>
>> Applications which need a modified version of the ICC Profile should instead
>> do the following:
>>
>>
>> byte[] profileData = ICC_Profile.getData() // get the byte array
>> represtation of BuiltIn- profile
>> ICCProfile newProfile = ICC_Profile.getInstance(profileData) // create a new
>> profile
>> newProfile.setData() // to modify and customize the profile
>>
>>
>> Following existing tests are modified to update a copy of Built-In profile.
>>
>> - java/awt/color/ICC_Profile/SetHeaderInfo.java
>> - java/awt/color/ICC_ProfileSetNullDataTest.java
>> - sun/java2d/cmm/ProfileOp/SetDataTest.java
>
> Harshitha Onkar has updated the pull request incrementally with one
> additional commit since the last revision:
>
> javadoc update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1163:
> 1161: * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
> 1162: * {@link ColorSpace#CS_CIEXYZ}.
> 1163: *
Suggestion:
*
* Note: JDK built-in ICC Profiles cannot be updated using this method
* as it will result in {@code IllegalArgumentException}. JDK built-in
* profiles are those obtained by
* {@code ICC_Profile.getInstance(int colorSpaceID)}
* where {@code colorSpaceID} is one of the following:
* {@link ColorSpace#CS_sRGB}, {@link ColorSpace#CS_LINEAR_RGB},
* {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
* {@link ColorSpace#CS_CIEXYZ}.
*
You should spell `IllegalArgumentException` fully, it explicitly lists the
exception thrown. And `colorSpaceID` should also be marked up with `{@code}`.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1170:
> 1168: * @throws IllegalArgumentException if {@code tagSignature} is not a
> 1169: * signature as defined in the ICC specification.
> 1170: * @throws IllegalArgumentException if a content of the {@code
> tagData}
This is an existing issue, if it's an issue: should the text say *“**the**
content”* instead of *“**a** content”*? @prrace
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1175:
> 1173: * @throws IllegalArgumentException if this is a profile for one of
> the
> 1174: * built-in pre-defined ColorSpaces, i.e. those which can
> be obtained
> 1175: * by calling {@code ICC_Profile.getInstance(int
> colorSpaceID)}
Suggestion:
* @throws IllegalArgumentException if this is a built-in profile for one
of the
* pre-defined color spaces, i.e. those which can be obtained
* by calling {@code ICC_Profile.getInstance(int colorSpaceID)}
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 42:
> 40: System.out.println("CASE 1: Testing BuiltIn Profile");
> 41: updateProfile(true);
> 42: System.out.println("Passed \n");
Suggestion:
System.out.println("Passed\n");
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 51:
> 49: private static void updateProfile(boolean isBuiltIn) {
> 50: ICC_Profile builtInProfile =
> ICC_Profile.getInstance(ColorSpace.CS_sRGB);
> 51: //create a copy of the BuiltIn profile
Suggestion:
// Create a copy of the built-in profile
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 58:
> 56: byte[] headerData = iccProfile.getData(HEADER_TAG);
> 57: int index = PROFILE_CLASS_START_INDEX;
> 58: //set profile class to valid icSigInputClass = 0x73636E72
Suggestion:
// Set profile class to valid icSigInputClass = 0x73636E72
test/jdk/java/awt/color/ICC_Profile/Bui
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon [v5]
On Fri, 21 Feb 2025 16:29:08 GMT, Rajat Mahajan wrote: >> **Issue:** >> The JNI method `Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon `calls >> `CreateIconFromRaster `that can throw a C++ exception. >> >> The C++ exception must be caught and must not be allowed to escape the JNI >> method. The call to `CreateIconFromRaster `has to wrapped into a try-catch >> block. >> >> **Solution:** >> >> Added exception handling to make sure any exception from >> `CreateIconFromRaster `is handled properly. >> >> Testing done. > > Rajat Mahajan has updated the pull request incrementally with one additional > commit since the last revision: > > use Macros TRY CATCH_BAD_ALLOC Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23470#pullrequestreview-2637059197
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
On Sat, 8 Feb 2025 04:02:08 GMT, Sergey Bylokhov wrote: >> **Issue:** >> The JNI method `Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon `calls >> `CreateIconFromRaster `that can throw a C++ exception. >> >> The C++ exception must be caught and must not be allowed to escape the JNI >> method. The call to `CreateIconFromRaster `has to wrapped into a try-catch >> block. >> >> **Solution:** >> >> Added exception handling to make sure any exception from >> `CreateIconFromRaster `is handled properly. >> >> Testing done. > > It would be good to check what type of exception the methods chain can throw, > is it only std::bad_alloc()? if yes, then we can use TRY + CATCH_BAD_ALLOC. > If we can get another exception, we should figure out how we can report this > error to the user. @mrserb Could you take another look? - PR Comment: https://git.openjdk.org/jdk/pull/23470#issuecomment-2678395367
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 21:29:34 GMT, Alexey Ivanov wrote: >> Although it is not a checked exception, the fact that >> IllegalArgumentException is already documented on this method is considered >> to outweigh any benefit of a new Exception type such as >> IllegalStateException. And IllegalArgumentException is not that far off from >> existing usage. >> Existing usage includes the case that "the specified data is not appropriate >> to be set in this profile". > >> > Although, there's no time where such a modification will be allowed >> >> Exactly, it will throw exception every time a built-In profile is passed >> unlike IllegalStateException which is thrown only during an inappropriate >> time for instance when you are trying to add an element to the queue when it >> is already full. Plus setData() already throws IAE for other invalid >> argument cases. > > I see no contradiction here. Inappropriate time is a vague definition. For > built-in object, it is always inappropriate time to call `setData`. > > `IllegalArgumentException` implies that the argument is invalid, and if I > change the argument, the call will succeed — but the call will never succeed > for a built-in profile *because **the state of the object** doesn't allow it*. > > This is why `IllegalStateException` is more appropriate. > >> Although it is not a checked exception, the fact that >> IllegalArgumentException is already documented on this method is considered >> to outweigh any benefit of a new Exception type such as >> IllegalStateException. > > I do not agree here. The exception type should convey the reason it's thrown, > otherwise there wouldn't be so many types of exceptions. > > And two `@throws` with the same type aren't allowed in javadoc, which means > you have to add the new reason to throw `IllegalArgumentException` into the > existing one. Let's consider a scenario. If I call `setData` with `tagSignature` and `tagData` which cannot be interpreted correctly, I get `IllegalArgumentException`, which is reasonable — I passed invalid arguments. Then if I call `setData` with valid values for `tagSignature` and `tagData`, the call succeeds even with a built-in color profile. After this fix, this call would fail with `IllegalArgumentException` but I know that the arguments are **valid** because the call succeeded previously. And however I change the arguments, the call will never succeed. Throwing `IllegalStateException` will convey the updated behaviour in a cleaner way: it is *the state* of the object that makes the call to fail, not the arguments. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964372049
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 20:34:19 GMT, Phil Race wrote: >>> Although, there's no time where such a modification will be allowed >> >> Exactly, it will throw exception every time a built-In profile is passed >> unlike IllegalStateException which is thrown only during an inappropriate >> time for instance when you are trying to add an element to the queue when it >> is already full. Plus setData() already throws IAE for other invalid >> argument cases. > > Although it is not a checked exception, the fact that > IllegalArgumentException is already documented on this method is considered > to outweigh any benefit of a new Exception type such as > IllegalStateException. And IllegalArgumentException is not that far off from > existing usage. > Existing usage includes the case that "the specified data is not appropriate > to be set in this profile". > > Although, there's no time where such a modification will be allowed > > Exactly, it will throw exception every time a built-In profile is passed > unlike IllegalStateException which is thrown only during an inappropriate > time for instance when you are trying to add an element to the queue when it > is already full. Plus setData() already throws IAE for other invalid argument > cases. I see no contradiction here. Inappropriate time is a vague definition. For built-in object, it is always inappropriate time to call `setData`. `IllegalArgumentException` implies that the argument is invalid, and if I change the argument, the call will succeed — but the call will never succeed for a built-in profile *because **the state of the object** doesn't allow it*. This is why `IllegalStateException` is more appropriate. > Although it is not a checked exception, the fact that > IllegalArgumentException is already documented on this method is considered > to outweigh any benefit of a new Exception type such as IllegalStateException. I do not agree here. The exception type should convey the reason it's thrown, otherwise there wouldn't be so many types of exceptions. And two `@throws` with the same type aren't allowed in javadoc, which means you have to add the new reason to throw `IllegalArgumentException` into the existing one. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964365376
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 19:38:29 GMT, Harshitha Onkar wrote:
>>> Can't this field be final and be set in constructor of the class?
>>> > Indeed, it _can be_, and I prefer this design. This way, the `isBuiltIn`
>>> > field can't be changed after the object is constructed. I think it's a
>>> > cleaner design.
>>
>> A cleaner diff between your latest commit and my commit on top:
>> https://github.com/openjdk/jdk/compare/8da82c10b5be3b92655abef95fd5be0c8b6eaa00..6729625f39c0dc47cd2588906b1793611996ca10
>>
>> This link shouldn't become invalid after either of us removes the
>> corresponding branches from our forks.
>
>> Indeed, it can be, and I prefer this design. This way, the isBuiltIn field
>> can't be changed after the object is constructed. I think it's a cleaner
>> design.
>
> I agree, the approach you mentioned does have merits. After evaluating both
> approaches setting in static block was preferred over the constructor for the
> following reason.
> Setting it via constructor meant that we could mark the profile as Built-In
> only when it was constructed using ProfileDeferralInfo and modifying all the
> constructors - ICC_Profile, ICC_ProfileRGB, ICC_ProfileGray whereas setting
> it in static block meant minimal changes to existing code.
I don't get the reason why it's bad to modify the constructors.
At this time, all the built-in profiles are created with `ProfileDeferralInfo`.
If `ProfileDeferralInfo` is used to create only built-in profiles, then
modifying `ICC_Profile(ProfileDeferralInfo pdi)` is enough:
ICC_Profile(ProfileDeferralInfo pdi) {
deferralInfo = pdi;
isBuiltIn = true;
}
And this is a *minimal* change.
When the `isBuiltIn` is final, it's much easier to find where the value gets
set.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964330041
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 15:14:42 GMT, Alexey Ivanov wrote: >> Indeed, it *can be*, and I prefer this design. This way, the `isBuiltIn` >> field can't be changed after the object is constructed. I think it's a >> cleaner design. >> >> To achieve this, you have to add a constructor which accepts a boolean flag: >> `ICC_Profile(ProfileDeferralInfo pdi, boolean builtIn)`, and >> `ICC_ProfileRGB` and `ICC_ProfileGray` need to provide an additional >> constructor as well. >> >> Here's [a diff to your branch which achieves >> this](https://github.com/honkar-jdk/jdk/compare/BuiltInCheck...aivanov-jdk:jdk:harshitha/8346465-built-inprofiles). >> >> When [you compare the changeset that I propose to >> `jdk/master`](https://github.com/openjdk/jdk/compare/master...aivanov-jdk:jdk:harshitha/8346465-built-inprofiles), >> there are less differences, and `BuiltInProfile` remains an interface. Yet >> two more files `ICC_ProfileRGB` and `ICC_ProfileGray` are modified. > > After adding a constructor which accepts a boolean parameter, the > constructors `ICC_ProfileRGB(ProfileDeferralInfo pdi)` and > `ICC_ProfileGray(ProfileDeferralInfo pdi)` — these two can be removed then. > Can't this field be final and be set in constructor of the class? > > Indeed, it _can be_, and I prefer this design. This way, the `isBuiltIn` > > field can't be changed after the object is constructed. I think it's a > > cleaner design. A cleaner diff between your latest commit and my commit on top: https://github.com/openjdk/jdk/compare/8da82c10b5be3b92655abef95fd5be0c8b6eaa00..6729625f39c0dc47cd2588906b1793611996ca10 This link shouldn't become invalid after either of us removes the corresponding branches from our forks. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963780521
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 18:20:52 GMT, Harshitha Onkar wrote:
>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1154:
>>
>>> 1152: * This method is useful for advanced applications which need to
>>> access
>>> 1153: * profile data directly. Only non-built-in, application provided
>>> profiles
>>> 1154: * should be updated using this method.
>>
>> What you mean is that only non-built-in profiles *can* be updated. The fix
>> makes it impossible to update built-in profiles.
>
> I'll be inverting this statement to state what is NOT allowed rather than
> what is allowed by .setData() as below. Does the following sound better?
>
>
> * Note: JDK built-in ICC Profiles cannot be updated using this method
> * as it will result in IAE. JDK built-in profiles are those obtained by
> * {@code ICC_Profile.getInstance(int colorSpaceID)} where colorSpaceID
> * is one of the following:
> * {@link ColorSpace#CS_sRGB}, {@link ColorSpace#CS_LINEAR_RGB},
> * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
> * {@link ColorSpace#CS_CIEXYZ}
This is clearer.
>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1164:
>>
>>> 1162: * array can not be interpreted as valid tag data,
>>> corresponding to
>>> 1163: * the {@code tagSignature}
>>> 1164: * @throws IllegalArgumentException if this is a profile for one
>>> of the
>>
>> `IllegalStateException` better describes the reason: the argument to the
>> method can be perfectly valid, but the internal state of the object doesn't
>> allow modifications.
>
> @aivanov-jdk
>
> _IllegalStateException - Signals that a method has been invoked at an
> **illegal or inappropriate time.**_
> Since IllegalStateException is thrown to indicate more of an unstable state
> of object, it may not be what we want here. The exception is to be thrown
> when the ICC_Profile object invoking .setData() is JDK Built-in profile and
> IIlegalArgumentException more closely match this case.
The javadoc for `IllegalArgumentException` says, https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/IllegalArgumentException.html";>Thrown
to indicate that a method has been passed an illegal or inappropriate
argument. (Emphasis mine.)
Getting an `IllegalArgumentException` for a valid argument would be confusing.
`IllegalStateException`, as you said, “signals that a method has been invoked
at an illegal or inappropriate time”. It's exactly the state of the
object: a *built-in* color profile cannot be modified.
Although, there's no time where such a modification will be allowed, I still
think `IllegalStateException` better conveys the meaning: changes to built-in
profiles are *never* allowed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964136111
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964135080
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 14:43:57 GMT, Alexey Ivanov wrote: >> Harshitha Onkar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> javadoc change > > src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 116: > >> 114: * BuiltInProfile. >> 115: */ >> 116: private boolean isBuiltIn = false; > > Can't this field be `final` and be set in constructor of the class? Indeed, it *can be*, and I prefer this design. This way, the `isBuiltIn` field can't be changed after the object is constructed. I think it's a cleaner design. To achieve this, you have to add a constructor which accepts a boolean flag: `ICC_Profile(ProfileDeferralInfo pdi, boolean builtIn)`, and `ICC_ProfileRGB` and `ICC_ProfileGray` need to provide an additional constructor as well. Here's [a diff to your branch which achieves this](https://github.com/honkar-jdk/jdk/compare/BuiltInCheck...aivanov-jdk:jdk:harshitha/8346465-built-inprofiles). When [you compare the changeset that I propose to `jdk/master`](https://github.com/openjdk/jdk/compare/master...aivanov-jdk:jdk:harshitha/8346465-built-inprofiles), there are less differences, and `BuiltInProfile` remains an interface. Yet two more files `ICC_ProfileRGB` and `ICC_ProfileGray` are modified. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963761853
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Thu, 20 Feb 2025 15:11:47 GMT, Alexey Ivanov wrote: >> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 116: >> >>> 114: * BuiltInProfile. >>> 115: */ >>> 116: private boolean isBuiltIn = false; >> >> Can't this field be `final` and be set in constructor of the class? > > Indeed, it *can be*, and I prefer this design. This way, the `isBuiltIn` > field can't be changed after the object is constructed. I think it's a > cleaner design. > > To achieve this, you have to add a constructor which accepts a boolean flag: > `ICC_Profile(ProfileDeferralInfo pdi, boolean builtIn)`, and `ICC_ProfileRGB` > and `ICC_ProfileGray` need to provide an additional constructor as well. > > Here's [a diff to your branch which achieves > this](https://github.com/honkar-jdk/jdk/compare/BuiltInCheck...aivanov-jdk:jdk:harshitha/8346465-built-inprofiles). > > When [you compare the changeset that I propose to > `jdk/master`](https://github.com/openjdk/jdk/compare/master...aivanov-jdk:jdk:harshitha/8346465-built-inprofiles), > there are less differences, and `BuiltInProfile` remains an interface. Yet > two more files `ICC_ProfileRGB` and `ICC_ProfileGray` are modified. After adding a constructor which accepts a boolean parameter, the constructors `ICC_ProfileRGB(ProfileDeferralInfo pdi)` and `ICC_ProfileGray(ProfileDeferralInfo pdi)` — these two can be removed then. - PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963768913
Re: RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]
On Wed, 19 Feb 2025 22:54:15 GMT, Harshitha Onkar wrote:
>> Built-in Profiles are singleton objects and if the user happens to modify
>> this shared profile object via setData() then the modified version of the
>> profile is returned each time the same built-in profile is requested via
>> getInstance().
>>
>> It is good to protect Built-in profiles from such direct modification by
>> adding BuiltIn profile check in `setData()` such that **only copies** of
>> Built-In profiles are allowed to be updated.
>>
>> With the proposed fix, if Built-In profile is updated using `.setData()` it
>> throws _**IAE - "BuiltIn profile cannot be modified"**_
>>
>> Fix consists of:
>> * `private boolean isBuiltIn = false` in ICC_Profile which is set to true
>> when BuiltIn profiles are created and false for non-builtIn profile.
>> * Converted BuiltInProfile from private interface to private static class
>> (with all the profile instances as static final)
>> * `isBuiltIn` flag is set to true when BuiltInProfile are loaded.
>> * JavaDoc update for setData() (CSR required)
>>
>> There are no restrictions on creating copies of BuiltIn profile and then
>> modifying it, but what is being restricted with this fix is - the direct
>> modification of the shared BuiltIn profile instance.
>>
>> Applications which need a modified version of the ICC Profile should instead
>> do the following:
>>
>>
>> byte[] profileData = ICC_Profile.getData() // get the byte array
>> represtation of BuiltIn- profile
>> ICCProfile newProfile = ICC_Profile.getInstance(profileData) // create a new
>> profile
>> newProfile.setData() // to modify and customize the profile
>>
>>
>> Following existing tests are modified to update a copy of Built-In profile.
>>
>> - java/awt/color/ICC_Profile/SetHeaderInfo.java
>> - java/awt/color/ICC_ProfileSetNullDataTest.java
>> - sun/java2d/cmm/ProfileOp/SetDataTest.java
>
> Harshitha Onkar has updated the pull request incrementally with one
> additional commit since the last revision:
>
> javadoc change
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 116:
> 114: * BuiltInProfile.
> 115: */
> 116: private boolean isBuiltIn = false;
Can't this field be `final` and be set in constructor of the class?
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1154:
> 1152: * This method is useful for advanced applications which need to
> access
> 1153: * profile data directly. Only non-built-in, application provided
> profiles
> 1154: * should be updated using this method.
What you mean is that only non-built-in profiles *can* be updated. The fix
makes it impossible to update built-in profiles.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1164:
> 1162: * array can not be interpreted as valid tag data,
> corresponding to
> 1163: * the {@code tagSignature}
> 1164: * @throws IllegalArgumentException if this is a profile for one of
> the
`IllegalStateException` better describes the reason: the argument to the method
can be perfectly valid, but the internal state of the object doesn't allow
modifications.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1172:
> 1170: public void setData(int tagSignature, byte[] tagData) {
> 1171: if (isBuiltIn) {
> 1172: throw new IllegalArgumentException("BuiltIn profile"
Suggestion:
throw new IllegalArgumentException("Built-in profile"
-
PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2630072442
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963699390
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963690614
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963694920
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963695795
Re: RFR: 8350224: Test javax/swing/JComboBox/TestComboBoxComponentRendering.java fails in ubuntu 23.x and later
On Wed, 19 Feb 2025 02:18:13 GMT, Prasanta Sadhukhan wrote: > I have changed the bug description to be symptomatic of the issue we are > seeing.. Thank you. - PR Comment: https://git.openjdk.org/jdk/pull/23670#issuecomment-2668601106
Integrated: 8350260: Improve HTML instruction formatting in PassFailJFrame
On Tue, 18 Feb 2025 11:48:09 GMT, Alexey Ivanov wrote: > **Problem:** > > When instructions are long, the formatting in `PassFailJFrame` looks off: > > 1. When the instructions are displayed on the screen for the first time, the > HTML is scrolled to the bottom, which isn't convenient; > 2. Numbers above 10 in the list are clipped on the left; > 3. No border around the HTML text. > > These problems were found while converting the instructions for > `test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java` in > https://github.com/openjdk/jdk/pull/23436. > > > Screenshots of the instructions > > The instructions are scrolled to the bottom when the test starts: >  > > The number 10 in the list is clipped on the left: >  > > The headings _“Testing with…”_ are flushed to the left, there's no additional > space between the scroll pane border and the text. > > > > **Fix:** > > 1. Adjust the list margins to accommodate for longer text; > 2. Install the text border to either text instruction component; > 3. Scroll the text to the top. > > > Screenshot of the instructions with the fix > > The stated issues are resolved: >  > > This pull request has now been integrated. Changeset: 014701a0 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/014701a09b23d21f57edb5b085820532804475bd Stats: 7 lines in 1 file changed: 2 ins; 1 del; 4 mod 8350260: Improve HTML instruction formatting in PassFailJFrame Reviewed-by: kizune, azvegint, abhiscxk - PR: https://git.openjdk.org/jdk/pull/23674
Re: RFR: 8350260: Improve HTML instruction formatting in PassFailJFrame
On Tue, 18 Feb 2025 12:03:43 GMT, Alexander Zuev wrote: > Especially scrolling to the beginning. That was the most disturbing thing. I didn't notice it before, because HTML instructions were never so long to have the vertical scroll bar. - PR Comment: https://git.openjdk.org/jdk/pull/23674#issuecomment-2665566653
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v3]
On Tue, 18 Feb 2025 11:12:14 GMT, Abhishek Kumar wrote: > > By the way, I updated the code in `PassFailJFrame` for HTML handling to > > make the long instructions look better. I'll submit a PR soon. > > That's a nice idea. Looking forward for the PR. Submitted PR https://github.com/openjdk/jdk/pull/23674. - PR Comment: https://git.openjdk.org/jdk/pull/23436#issuecomment-2665442437
RFR: 8350260: Improve HTML instruction formatting in PassFailJFrame
**Problem:** When instructions are long, the formatting in `PassFailJFrame` looks off: 1. When the instructions are displayed on the screen for the first time, the HTML is scrolled to the bottom, which isn't convenient; 2. Numbers above 10 in the list are clipped on the left; 3. No border around the HTML text. These problems were found while converting the instructions for `test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java` in https://github.com/openjdk/jdk/pull/23436. Screenshots of the instructions The instructions are scrolled to the bottom when the test starts:  The number 10 in the list is clipped on the left:  The headings _“Testing with…”_ are flushed to the left, there's no additional space between the scroll pane border and the text. **Fix:** 1. Adjust the list margins to accommodate for longer text; 2. Install the text border to either text instruction component; 3. Scroll the text to the top. Screenshot of the instructions with the fix The stated issues are resolved:  - Commit messages: - 8350260: Improve HTML instruction formatting in PassFailJFrame Changes: https://git.openjdk.org/jdk/pull/23674/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23674&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8350260 Stats: 7 lines in 1 file changed: 2 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/23674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23674/head:pull/23674 PR: https://git.openjdk.org/jdk/pull/23674
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v4]
On Tue, 18 Feb 2025 11:15:57 GMT, Abhishek Kumar wrote: >> VoiceOver doesn't announce the _untick state_ when Checkbox is `deselected` >> using **space** key. When CheckBox is deselected, the state change is not >> notified to a11y client (VoiceOver) and so the state is not announced by VO. >> >> Screen Magnifier is also unable to magnify the unchecked state of JCheckBox >> due to same reason and is captured as separate bug >> [JDK-8345728](https://bugs.openjdk.org/browse/JDK-8345728). >> >> Proposed fix is to send the state change notification to a11y client when >> checkbox is deselected, this resolves the problem for VoiceOver and Screen >> Magnifier. >> >> Similar issue observed for JToggleButton. So, I extended the fix for >> JToggleButton as well. >> >> The proposed change can be verified the manual test in the PR. >> >> CI pipeline testing is `OK`, link posted in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > Minor test instruction formatting Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23436#pullrequestreview-2623293534
Re: RFR: 8350224: Deproblemlist javax/swing/JComboBox/TestComboBoxComponentRendering.java
On Tue, 18 Feb 2025 07:45:06 GMT, Prasanta Sadhukhan wrote: > Test is failing in ubuntu 23.x and beyond with expected red pixel not being > picked up > Increase in fontsize increase the possibility of font red pixel being picked > up. > Test is now passing in ubuntu 24.04 and in CI.. The fix looks good to me. However, the JBS bug should be more specific: the PR doesn't only remove the `javax/swing/JComboBox/TestComboBoxComponentRendering.java` test from the problem list, it modifies the test so that it can be removed from the problem-list. Therefore, the subject of the bug has to reflect this additional change. (*“Increase font size in javax/swing/JComboBox/TestComboBoxComponentRendering.java”* isn't a bad candidate in my opinion.) The current subject would be fine if you only removed the test from PL without modifying it. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23670#pullrequestreview-2623177171
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v3]
On Fri, 14 Feb 2025 04:32:53 GMT, Abhishek Kumar wrote: >> VoiceOver doesn't announce the _untick state_ when Checkbox is `deselected` >> using **space** key. When CheckBox is deselected, the state change is not >> notified to a11y client (VoiceOver) and so the state is not announced by VO. >> >> Screen Magnifier is also unable to magnify the unchecked state of JCheckBox >> due to same reason and is captured as separate bug >> [JDK-8345728](https://bugs.openjdk.org/browse/JDK-8345728). >> >> Proposed fix is to send the state change notification to a11y client when >> checkbox is deselected, this resolves the problem for VoiceOver and Screen >> Magnifier. >> >> Similar issue observed for JToggleButton. So, I extended the fix for >> JToggleButton as well. >> >> The proposed change can be verified the manual test in the PR. >> >> CI pipeline testing is `OK`, link posted in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > HTML instructions formatting Looks good, just a couple of minor tweaks to the instructions. By the way, I updated the code in `PassFailJFrame` for HTML handling to make the long instructions look better. I'll submit a PR soon. test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 70: > 68: Enable Screen magnifier on the Mac: > 69:System Settings -> Accessibility -> > 70:Hover Text -> Enable Hover Text Suggestion: Hover Text -> Enable Hover Text Only the UI element should be in bold. (Yes, I know that I provided this snippet, yet I didn't notice it back then.) test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 71: > 69:System Settings -> Accessibility -> > 70:Hover Text -> Enable Hover Text > 71:Default Hover Text Activation Modifier is > Command key. Suggestion: Default Hover Text Activation Modifier is Command key The period at the end of the only sentence looks weird. test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 72: > 70:Hover Text -> Enable Hover Text > 71:Default Hover Text Activation Modifier is > Command key. > 72: Move focus back to test application Suggestion: Move focus back to the test application and perform the following tests test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 100: > 98: > 99: > 100: Suggestion: Disable Hover Text (optionally) in the Settings To complete the instructions. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23436#pullrequestreview-2623029837 PR Comment: https://git.openjdk.org/jdk/pull/23436#issuecomment-2665180871 PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1959441483 PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1959442820 PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1959429037 PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1959438566
Integrated: 8294155: Exception thrown before awaitAndCheck hangs PassFailJFrame
On Tue, 11 Feb 2025 16:17:13 GMT, Alexey Ivanov wrote: > If a manual test throws an exception during construction of `PassFailJFrame`, > the test execution hangs. When the builder pattern is used, no UI appears on > the screens, but the Java process does not terminate automatically because > there are windows which prevent AWT from shutting down. > > **Fix:** > > Catch exceptions in `PassFailJFrame.Builder.build()` and dispose of all the > windows if an exception occurs. > > This ensures all the created windows are disposed of, which lets AWT shut > down cleanly. > > _Note:_ the above problem doesn't occur when the test is run with `jtreg` > because jtreg shuts down the JVM as soon as the main thread exits. This pull request has now been integrated. Changeset: 906358d3 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/906358d3a14ce755fec771f0a6bb856b3a8f3297 Stats: 14 lines in 1 file changed: 12 ins; 0 del; 2 mod 8294155: Exception thrown before awaitAndCheck hangs PassFailJFrame Reviewed-by: serb, azvegint, kizune - PR: https://git.openjdk.org/jdk/pull/23564
Integrated: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable
On Tue, 28 Jan 2025 14:51:57 GMT, Alexey Ivanov wrote: > The `javax/swing/JButton/4796987/bug4796987.java` test strictly verifies if > it's run on Windows XP, for this reason it never runs on modern versions of > Windows. > > Since Windows XP is obsolete and visual styles cannot be disabled since > Windows 8, I removed the OS version check completely. > > I captured an image of the panel and analyse colors in the image instead of > using `Robot.getPixelColor`; I save the image for reference if the test fails. > > The updated test passes in the CI. This pull request has now been integrated. Changeset: 650d0d95 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/650d0d954ea8e20e31f17d459993d5edecf08a4c Stats: 82 lines in 1 file changed: 45 ins; 15 del; 22 mod 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable Reviewed-by: tr, abhiscxk, serb - PR: https://git.openjdk.org/jdk/pull/23336
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon [v4]
On Sun, 16 Feb 2025 03:25:50 GMT, Rajat Mahajan wrote: >> **Issue:** >> The JNI method `Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon `calls >> `CreateIconFromRaster `that can throw a C++ exception. >> >> The C++ exception must be caught and must not be allowed to escape the JNI >> method. The call to `CreateIconFromRaster `has to wrapped into a try-catch >> block. >> >> **Solution:** >> >> Added exception handling to make sure any exception from >> `CreateIconFromRaster `is handled properly. >> >> Testing done. > > Rajat Mahajan has updated the pull request incrementally with one additional > commit since the last revision: > > CATCH_BAD_ALLOC The suggestion was to use the macros: [`TRY`](https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/alloc.h#L131-L134) and [`CATCH_BAD_ALLOC`](https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/alloc.h#L154-L160). - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23470#pullrequestreview-2621002791
Integrated: 8342524: Use latch in AbstractButton/bug6298940.java instead of delay
On Fri, 24 Jan 2025 16:47:42 GMT, Alexey Ivanov wrote: > Use a `CountDownLatch` in `javax/swing/AbstractButton/bug6298940.java` > instead of delay. > Use `Util.hitMnemonics` instead of custom code to handle macOS. > > I ran the updated test a few times with `JTREG=REPEAT_COUNT=20`, the test > always passed in the CI. This pull request has now been integrated. Changeset: 2bd8f026 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/2bd8f026dbd449e810dc6ce96cd9235e5cb51a9b Stats: 93 lines in 1 file changed: 93 ins; 0 del; 0 mod 8342524: Use latch in AbstractButton/bug6298940.java instead of delay Reviewed-by: azvegint, kizune, dnguyen, achung - PR: https://git.openjdk.org/jdk/pull/23301
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon [v4]
On Mon, 17 Feb 2025 13:03:59 GMT, Alexey Ivanov wrote:
> The suggestion was to use the macros:
> [`TRY`](https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/alloc.h#L131-L134)
> and
> [`CATCH_BAD_ALLOC`](https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/alloc.h#L154-L160).
The code would look like this:
JNIEXPORT void JNICALL Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
(JNIEnv *env, jobject, jlong window, jintArray buf, jint w, jint h)
{
TRY;
HICON icon = CreateIconFromRaster(env, buf, w, h);
m_Taskbar->SetOverlayIcon((HWND)window, icon, NULL);
::DestroyIcon(icon);
CATCH_BAD_ALLOC;
}
-
PR Comment: https://git.openjdk.org/jdk/pull/23470#issuecomment-2663076031
Re: RFR: 8349351 : Combine Screen Inset Tests into a Single File [v2]
On Sun, 9 Feb 2025 19:56:48 GMT, anass baya wrote: >> While working on [JDK-6899304](https://bugs.openjdk.org/browse/JDK-6899304), >> we discovered that there are two tests meant to perform the same task. >> >> The first test is located at >> test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java >> and was originally designed for multi-screen configurations on Linux >> platforms. >> >> The second test, located at >> test/jdk/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java, is >> intended for single-screen configurations but lacks accuracy due to some >> workarounds to support Windows. >> >> Now, the test at >> test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java >> has been updated to work across all platforms, including Windows, which was >> previously failing. As a result, it has been agreed to rename this test to >> ScreenInsetsTest, remove the condition that restricted its execution to >> multi-screen configurations, and remove the redundant test at >> test/jdk/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java. > > anass baya has updated the pull request incrementally with one additional > commit since the last revision: > > Add proposed enhancments Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23449#pullrequestreview-2618077111
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v11]
On Thu, 13 Feb 2025 03:23:32 GMT, Prasanta Sadhukhan
wrote:
> Is it mentioned somewhere that Windows 12 will fallback to Windows 10
> rendering?
I was mentioned in our, JDK, code.
https://github.com/openjdk/jdk/blob/46a1cb5a4f2eb922e56b221a5420d2ac1204ade5/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java#L663
The condition `System.getProperty("os.name").equals("Windows 11")` would
evaluate to `false` in Windows 12, and the rendering would've fell back to
Windows 10 way.
You've taken care of this in your latest changeset.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1954441223
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 5 Feb 2025 07:13:33 GMT, Abhishek Kumar wrote: >> test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 87: >> >>> 85: >>> 86: Press Pass if you are able to hear correct VoiceOver >>> announcements and >>> 87: able to see the correct screen magnifier behaviour. """; >> >> These long instructions may benefit from using HTML for formatting >> instructions. > > Are you suggesting to change entire instruction set with HTML formatting ? > > I would appreciate if you add some sample changes. you are referring Here's the sample that I used: HTML code for instructions String INSTRUCTIONS = """ Testing with VoiceOver Start the VoiceOver application (Press Command + F5) Click on the Frame with CheckBox and ToggleButton window to move focus Press Spacebar VO should announce the checked state Press Spacebar again VO should announce the unchecked state Press Tab to move focus to ToggleButton Repeat steps 3 to 6 and listen the announcement If announcements are incorrect, press Fail Stop the VoiceOver application (Press Command + F5 again) Testing with Screen Magnifier Enable Screen magnifier on the Mac: System Settings -> Accessibility -> Hover Text -> Enable Hover Text Default Hover Text Activation Modifier is Command key. Move focus back to test application Test CheckBox states with Screen Magnifier Click on CheckBox to select it Press the Command key and hover mouse over CheckBox CheckBox ticked state along with its label should be magnified Keep the Command key pressed and click CheckBox to deselect it CheckBox unticked state along with its label should be magnified Release the Command key If Screen Magnifier behaviour is incorrect, press Fail Test ToggleButton states with Screen Magnifier Click on ToggleButton to select it Press the Command key and hover mouse over ToggleButton Ticked state along with label should be magnified Keep the Command button pressed and click ToggleButton to deselect it Unticked state along with its label should be magnified Release the Command key If Screen Magnifier behaviour is incorrect, press Fail Press Pass if you are able to hear correct VoiceOver announcements and able to see the correct screen magnifier behaviour."""; This gives the following look: https://github.com/user-attachments/assets/38146f37-c86c-4e14-bc9f-98ae384c17ac"; /> - PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1953131477
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 5 Feb 2025 07:04:21 GMT, Abhishek Kumar wrote: >> VoiceOver doesn't announce the _untick state_ when Checkbox is `deselected` >> using **space** key. When CheckBox is deselected, the state change is not >> notified to a11y client (VoiceOver) and so the state is not announced by VO. >> >> Screen Magnifier is also unable to magnify the unchecked state of JCheckBox >> due to same reason and is captured as separate bug >> [JDK-8345728](https://bugs.openjdk.org/browse/JDK-8345728). >> >> Proposed fix is to send the state change notification to a11y client when >> checkbox is deselected, this resolves the problem for VoiceOver and Screen >> Magnifier. >> >> Similar issue observed for JToggleButton. So, I extended the fix for >> JToggleButton as well. >> >> The proposed change can be verified the manual test in the PR. >> >> CI pipeline testing is `OK`, link posted in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > Condition evaluation simplified Looks good to me. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23436#pullrequestreview-2612711918
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 12 Feb 2025 15:49:37 GMT, Alexey Ivanov wrote: >> @aivanov-jdk I think we should not change RadioButton's implementation as >> the announcement is not consistent. >> Do you have any other suggestion ? > > @kumarabhi006 Sorry the long delay. > > I tested the behaviour of radio buttons and I can't see or hear any > difference in announcements when the same condition, namely > `!Objects.equals(newValue, oldValue)`, is used before calling > `valueChanged(ptr)`. > > I added a panel with three radio buttons to your test case. Initially all the > buttons aren't selected. VoiceOver announces when I move to the first radio > button. It also announces when I move between radio buttons with left and > right arrows. Then when I press the Space key, the current radio > button gets selected and this is announced by VoiceOver: selected, caption of the button>. When I move to another button with arrow keys, it > doesn't get selected right away when VoiceOver is active, and VoiceOver > announces that I moved to another button and that it's not selected. Pressing > the Space key selects the currently active button. > > What do you hear when testing such a scenario? Tested with SwingSet2, and I can hear the difference in announcements. It looks the difference comes from the focus events: when the old button loses focus, `valueChanged` isn't called currently, but if `Objects.equals` is used, `valueChanged` gets called twice. Let's keep the current behaviour, it's more reliable even though it looks inconsistent. - PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1953019780
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Fri, 7 Feb 2025 08:14:51 GMT, Abhishek Kumar wrote: >>> For consistency, it can be done. I need to check if it impacts on VO >>> announcement. >> >> It shouldn't impact the announcements. One button gets deselected, another >> one gets selected… >> >> Yet now that I think about it more, we may want to skip announcing >> deselecting a button in the group when selection moves to another button. >> >> What about the case where currently focused or unfocused selected button >> gets deselected programmatically? Is there an interactive way to deselect a >> radio button when it's the only button in its own group? >> >> This *needs more testing*. > > @aivanov-jdk I think we should not change RadioButton's implementation as the > announcement is not consistent. > Do you have any other suggestion ? @kumarabhi006 Sorry the long delay. I tested the behaviour of radio buttons and I can't see or hear any difference in announcements when the same condition, namely `!Objects.equals(newValue, oldValue)`, is used before calling `valueChanged(ptr)`. I added a panel with three radio buttons to your test case. Initially all the buttons aren't selected. VoiceOver announces when I move to the first radio button. It also announces when I move between radio buttons with left and right arrows. Then when I press the Space key, the current radio button gets selected and this is announced by VoiceOver: selected, . When I move to another button with arrow keys, it doesn't get selected right away when VoiceOver is active, and VoiceOver announces that I moved to another button and that it's not selected. Pressing the Space key selects the currently active button. What do you hear when testing such a scenario? - PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1952921829
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Wed, 12 Feb 2025 03:07:25 GMT, Prasanta Sadhukhan wrote: >> It just feels wrong… A superclass doesn't need to know, shouldn't know, >> about its subclasses. >> >> Yet this solution could be the simplest one… if achieving the same effect >> different makes the code too complicated. > > Where it is checking for subclasses? It is just checking for if the current > lookandfeel is Windows L&F (as I told it is not checking for `instanceof > WindowsLookAndFeel` in which case one could have argued that it is trying to > know about windows subclass.. > > Also, having helper method will have effect in only Windows subclass and noop > in others and that is unnecessary in my opinion and on top of that, it will > need a CSR for that method and that method would be additional maintenance > headache and it will prevent backporting this fix if one wants to and > considering this is related to JCK issue, people would like it to be > backported.. Windows Look-and-Feel is based on Basic L&F implementation — from this point of view, Basic L&F changes its layout to accommodate for customisations that are needed for another L&F that's descends from it. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1952779196
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v11]
On Wed, 12 Feb 2025 03:06:09 GMT, Prasanta Sadhukhan
wrote:
> Ending support shouldn't mean we should make Windows 10 design look like
> Windows 11, that would be so wrong, in my opinion..because the user would
> like to get the same look and feel as in native Windows 10 as much as
> possible..
The thing is neither of us has found any app which displays both radio bullets
or check-marks and an icon in its menus.
The problem with `.equals("Windows 11")` is that it'll need updating if and
when Microsoft releases a newer version of Windows because Windows 12 will
fallback to rendering like in Windows 10 and earlier versions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1952771095
Re: RFR: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable [v3]
On Tue, 11 Feb 2025 02:51:01 GMT, Sergey Bylokhov wrote: > Even for classic windows? I guess the test is applicable to the classic Windows theme, yet [JDK-4796987](https://bugs.openjdk.org/browse/JDK-4796987) states, https://bugs.openjdk.org/browse/JDK-4796987#descriptionmodule-label";>This problem does not occur if desktop themes descending from the classic (Windows 2000) look are used. I'm quite reluctant about adding another case to the test. Let me know if you think it's necessary. - PR Review Comment: https://git.openjdk.org/jdk/pull/23336#discussion_r1951321428
Re: RFR: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable [v3]
> The `javax/swing/JButton/4796987/bug4796987.java` test strictly verifies if > it's run on Windows XP, for this reason it never runs on modern versions of > Windows. > > Since Windows XP is obsolete and visual styles cannot be disabled since > Windows 8, I removed the OS version check completely. > > I captured an image of the panel and analyse colors in the image instead of > using `Robot.getPixelColor`; I save the image for reference if the test fails. > > The updated test passes in the CI. Alexey Ivanov 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 master - Update the test summary - Remove the redundant button - 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable - Changes: - all: https://git.openjdk.org/jdk/pull/23336/files - new: https://git.openjdk.org/jdk/pull/23336/files/0349f37a..a03f66df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23336&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23336&range=01-02 Stats: 31639 lines in 929 files changed: 14138 ins; 10937 del; 6564 mod Patch: https://git.openjdk.org/jdk/pull/23336.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23336/head:pull/23336 PR: https://git.openjdk.org/jdk/pull/23336
Re: RFR: 8347019: Test javax/swing/JRadioButton/8033699/bug8033699.java still fails: Focus is not on Radio Button Single as Expected
On Tue, 11 Feb 2025 04:15:49 GMT, Prasanta Sadhukhan wrote: > Test seems to fail in ubuntu 22.04 system..It seems removing setAutoDelay is > making the test more stable in such systems. > Tested in all platforms where it is still ok without auto delay. CI job link > in JBS.. Looks good. I assume the test remains stable other platforms. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23554#pullrequestreview-2609456798
RFR: 8294155: Exception thrown before awaitAndCheck hangs PassFailJFrame
If a manual test throws an exception during construction of `PassFailJFrame`, the test execution hangs. When the builder pattern is used, no UI appears on the screens, but the Java process does not terminate automatically because there are windows which prevent AWT from shutting down. **Fix:** Catch exceptions in `PassFailJFrame.Builder.build()` and dispose of all the windows if an exception occurs. This ensures all the created windows are disposed of, which lets AWT shut down cleanly. _Note:_ the above problem doesn't occur when the test is run with `jtreg` because jtreg shuts down the JVM as soon as the main thread exits. - Commit messages: - 8294155: Exception thrown before awaitAndCheck hangs PassFailJFrame Changes: https://git.openjdk.org/jdk/pull/23564/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23564&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294155 Stats: 14 lines in 1 file changed: 12 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/23564.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23564/head:pull/23564 PR: https://git.openjdk.org/jdk/pull/23564
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon [v2]
On Mon, 10 Feb 2025 21:50:26 GMT, Rajat Mahajan wrote: >> **Issue:** >> The JNI method `Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon `calls >> `CreateIconFromRaster `that can throw a C++ exception. >> >> The C++ exception must be caught and must not be allowed to escape the JNI >> method. The call to `CreateIconFromRaster `has to wrapped into a try-catch >> block. >> >> **Solution:** >> >> Added exception handling to make sure any exception from >> `CreateIconFromRaster `is handled properly. >> >> Testing done. > > Rajat Mahajan has updated the pull request incrementally with two additional > commits since the last revision: > > - Update src/java.desktop/windows/native/libawt/windows/awt_Taskbar.cpp > >Co-authored-by: Alexey Ivanov > - Update copyright year. Changes requested by aivanov (Reviewer). src/java.desktop/windows/native/libawt/windows/awt_Taskbar.cpp line 2: > 1: /* > 2: * Copyright (c) 2016, 2025 Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved. A comma after 2025 is required. - PR Review: https://git.openjdk.org/jdk/pull/23470#pullrequestreview-2609078478 PR Review Comment: https://git.openjdk.org/jdk/pull/23470#discussion_r1951043779
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v11]
On Tue, 11 Feb 2025 03:00:29 GMT, Prasanta Sadhukhan
wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java
>> line 663:
>>
>>> 661: paintIcon(g, lh, lr, holdc);
>>> 662: if (UIManager.getLookAndFeel().getName().equals("Windows")
>>> 663: && System.getProperty("os.name").equals("Windows 11")
>>
>> It's not what I meant.
>>
>> I suggested painting the menu items *the same way* for Windows 10 and 11.
>> Yet the layout for Windows 11 should be tweaked.
>>
>> To be clear, the bullet / check-mark is painted at the same location where
>> it would be painted if there were no custom icon for both Windows 10 and 11.
>> The custom icon is painted to the right of the bullet / check-mark, and the
>> menu text moves farther to the right.
>
> This will preserve existing windows 10 behavior and at the same time, it will
> show Windows 11 File explorer behavior.
> What is the need to show the same way for WIndows 10 and 11 when it will not
> be same as what native does for windows 10.
>
> I am sorry but I dont agree to this...
Because, as you say, support for Windows 10 ends in October this year.
For this reason, I'd rather avoid the complexity of supporting different look
for Windows 10 and 11.
I won't object to this if you think it's important to support Windows 10 look.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1951010812
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Tue, 11 Feb 2025 03:11:18 GMT, Prasanta Sadhukhan wrote: >>> I am checking for WIndows L&F so Metal will not be affected. >> >> This is the reason why I don't like this approach: the base class needs to >> know about Windows L&F and do some customisation based on the current L&F. >> This code will be included on Linux and macOS where Windows L&F isn't >> available. >> >> A better solution would be a helper method or a helper class which handles >> this difference. >> >> As far as I can see, Metal L&F paints both the bullet / check-mark and the >> custom icon — this is exactly what you're implementing here. The difference >> is in margins around these elements. >> >> The layout of the menu is handled by `MenuItemLayoutHelper`. It's customised >> for Synth L&F in `SynthMenuItemLayoutHelper`. Perhaps, you need to create a >> customised class for Windows L&F. >> >> Yet there's no hook in `BasicMenuItemUI` to create or pass another >> `MenuItemLayoutHelper` object which customises the layout. A new method may >> be added. >> >> --- >> >> I haven't looked deeply into how popup menus are laid out and therefore I >> can't advice on how to customise menu item layout. > > It is just checking for current L&F is Windows or not..I am not even checking > for "instanceof WIndowsLookAndFeel" which will not be present in linux and > mac so I dont think it will create any problem.. > Helper method will create unnecessary changes in all L&F..I dont think I will > like to do that.. It just feels wrong… A superclass doesn't need to know, shouldn't know, about its subclasses. Yet this solution could be the simplest one… if achieving the same effect different makes the code too complicated. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1951015048
Re: RFR: JDK-8348302 : [Test bug] Update FileDialogIconTest.java [v3]
On Fri, 7 Feb 2025 22:20:26 GMT, Harshitha Onkar wrote:
>> FileDialogIconTest.java has been updated.
>>
>> Following changes were made.
>>
>> - Test instructions updated
>> - BugID associated with the test is updated to the correct one
>> - setIconBufferedImagesToFrame and setIconBufferedImagesToDialog btns added
>> to the frame.
>> - other minor cleanups
>
> Harshitha Onkar has updated the pull request incrementally with one
> additional commit since the last revision:
>
> minor
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 59:
> 57: public static void main(String[] args) throws Exception {
> 58: String INSTRUCTIONS = """
> 59: The 1st row of buttons in testUI are to open Load,
It may not be apparent what `testUI` is… on the other hand, it should still be
obvious that you referring to a window on the right.
It could better to refer to it as a ‘window’… or a least ‘test UI’ (with a
space).
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 76:
> 74:
> 75: PassFailJFrame.builder()
> 76: .title("Test Instructions Frame")
I believe the ‘frame‘ is redundant.
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 152:
> 150: image =
> Toolkit.getDefaultToolkit().getImage(fileName);
> 151: PassFailJFrame.log("Loaded image " + "T" + i +
> ".gif."
> 152:+ " Setting to the list for
> frame");
Why can't `fileName` be used in the logging?
Suggestion:
PassFailJFrame.log("Loaded image " + fileName + "."
+ " Setting to the list for frame");
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 172:
> 170: image =
> Toolkit.getDefaultToolkit().getImage(fileName);
> 171: PassFailJFrame.log("Loaded image " + "T" + i +
> ".gif."
> 172:+ " Setting to the list for
> dialog");
This piece of code looks the save as the one above.
Does it make to isolate the loading of images from setting them (to avoid code
duplication)?
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 185:
> 183: setImageButton5.addActionListener(new ActionListener() {
> 184: public void actionPerformed(ActionEvent event) {
> 185: List list = new ArrayList<>();
Suggestion:
List list = new ArrayList<>(4);
Micro-optimization: we know the number of images, the array in `ArrayList`
could be sized accordingly.
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 217:
> 215: list.add(image);
> 216: }
> 217: setImagesToFD(list);
Here, again the code is the same as in `setImageButton5.addActionListener`
except for the final call `setImagesToFD` vs `setImagesToFrame`. I suggest
moving capture code out of either methods and re-use it.
test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 256:
> 254: static class MyActionListener implements ActionListener {
> 255: int id;
> 256: String name;
Suggestion:
final int id;
final String name;
If the value of the fields is unchanged, they should be `final`. The may even
be `private`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949668666
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949652576
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949653808
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949657086
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949658652
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949661300
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949663909
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
On Sat, 8 Feb 2025 04:02:08 GMT, Sergey Bylokhov wrote: > It would be good to check what type of exception the methods chain can throw, > is it only std::bad_alloc()? if yes, then we can use TRY + CATCH_BAD_ALLOC. > If we can get another exception, we should figure out how we can report this > error to the user. In `CreateIconFromRaster` a catch-all construct is used, and then the exception is re-thrown: https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2082-L2087 At the same time, `BitmapUtil::CreateTransparencyMaskFromARGB` may throw only `std::bad_alloc` at https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/awt_BitmapUtil.cpp#L43 where `SAFE_SIZE_NEW_ARRAY2` is defined as https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/share/native/common/awt/utility/sizecalc.h#L93-L95 Thus, after looking at the chain of calls, `std::bad_alloc` is the only C++ exception that may ever be thrown. Then, the pair of [`TRY`](https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/alloc.h#L131-L134) and [`CATCH_BAD_ALLOC`](https://github.com/openjdk/jdk/blob/ab66c82ce9fdb5ee3fd7690f42b8ad4d78bf5e40/src/java.desktop/windows/native/libawt/windows/alloc.h#L154-L160) could be a better alternative. - PR Comment: https://git.openjdk.org/jdk/pull/23470#issuecomment-2648827456
Re: RFR: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
On Wed, 5 Feb 2025 18:40:07 GMT, Rajat Mahajan wrote:
> **Issue:**
> The JNI method `Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon `calls
> `CreateIconFromRaster `that can throw a C++ exception.
>
> The C++ exception must be caught and must not be allowed to escape the JNI
> method. The call to `CreateIconFromRaster `has to wrapped into a try-catch
> block.
>
> **Solution:**
>
> Added exception handling to make sure any exception from
> `CreateIconFromRaster `is handled properly.
>
> Testing done.
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/native/libawt/windows/awt_Taskbar.cpp line 1:
> 1: /*
Please update the copyright year.
src/java.desktop/windows/native/libawt/windows/awt_Taskbar.cpp line 130:
> 128: {
> 129: try
> 130: {
Suggestion:
try {
I think it makes sense to use Java style and put the opening brace on the same
line with `try` as this style is followed by `if`-`else` statements in the file
as well as you follow Java style for the `catch` block below.
-
PR Review: https://git.openjdk.org/jdk/pull/23470#pullrequestreview-2606715517
PR Review Comment: https://git.openjdk.org/jdk/pull/23470#discussion_r1949617073
PR Review Comment: https://git.openjdk.org/jdk/pull/23470#discussion_r1949616538
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v11]
On Mon, 10 Feb 2025 16:23:23 GMT, Prasanta Sadhukhan
wrote:
>> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is
>> shown without radiobutton in WIndowsLookAndFeel as there was no provision of
>> drawing the radiobutton alongside icon.
>> If icon is not there, the radiobutton is drawn. Added provision of drawing
>> the radiobutton windows Skin even when imageIcon is present.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Restore Windows 10 behavior
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java line
663:
> 661: paintIcon(g, lh, lr, holdc);
> 662: if (UIManager.getLookAndFeel().getName().equals("Windows")
> 663: && System.getProperty("os.name").equals("Windows 11")
It's not what I meant.
I suggested painting the menu items *the same way* for Windows 10 and 11. Yet
the layout for Windows 11 should be tweaked.
To be clear, the bullet / check-mark is painted at the same location where it
would be painted if there were no custom icon for both Windows 10 and 11. The
custom icon is painted to the right of the bullet / check-mark, and the menu
text moves farther to the right.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 886:
> 884: skin.paintSkin(g, x - OFFSET, y +
> OFFSET, state);
> 885: }
> 886: } else {
Again, it's not what I meant.
Both Windows 10 and 11 should follow the same layout — the new style that you
found in File Explorer in Windows 11.
-
PR Review: https://git.openjdk.org/jdk/pull/23324#pullrequestreview-2606681172
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1949596849
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1949599874
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Mon, 10 Feb 2025 05:24:32 GMT, Prasanta Sadhukhan wrote: > I am checking for WIndows L&F so Metal will not be affected. This is the reason why I don't like this approach: the base class needs to know about Windows L&F and do some customisation based on the current L&F. This code will be included on Linux and macOS where Windows L&F isn't available. A better solution would be a helper method or a helper class which handles this difference. As far as I can see, Metal L&F paints both the bullet / check-mark and the custom icon — this is exactly what you're implementing here. The difference is in margins around these elements. The layout of the menu is handled by `MenuItemLayoutHelper`. It's customised for Synth L&F in `SynthMenuItemLayoutHelper`. Perhaps, you need to create a customised class for Windows L&F. Yet there's no hook in `BasicMenuItemUI` to create or pass another `MenuItemLayoutHelper` object which customises the layout. A new method may be added. --- I haven't looked deeply into how popup menus are laid out and therefore I can't advice on how to customise menu item layout. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1949525180
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Mon, 10 Feb 2025 05:28:50 GMT, Prasanta Sadhukhan wrote: > > It's close, yet the spacing is wrong. > > > > The bullet / check-mark should be where they were before the fix. There was > > 8-pixel margin to the left of selected bullet background before the fix, > > now there are only 2 pixels. > > > > The margin between the bullet background and the text was again 8 pixels, I > > think we should maintain the same margin between the bullet and the icon as > > well as between the icon and the menu text. > > Does bullet/checkmark use to appear before the fix in windows 10? I thought > you told the icon was getting highlighted/dehighlighted depending on item is > selected or not That's right! The `ImageIcon` was painted instead of bullet or check-mark. Now that you moved the `ImageIcon` to the right, the bullet or check-mark has to be painted where the bullets and check-marks are painted when there's no custom icon. That's what I meant — the previous location of the bullet / check-mark should be preserved. > I am not sure if we can make 8 pixel margin as it will cause MenuItem text or > bullet to be outgrow popupmenu width ( I faced this issue while experimenting > with spacing), which I am not sure if and where we can increase. This is why I say that the layout and therefore the width of the menu item has to be updated. Previously, the custom icon was painted over, or instead of, the bullet or check-mark. This PR proposes painting the custom icon separately, which means the width of the menu item has increase to accommodate the margins around the custom icon as well as the icon itself. - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2648601371
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Fri, 7 Feb 2025 05:26:10 GMT, Prasanta Sadhukhan wrote: >> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is >> shown without radiobutton in WIndowsLookAndFeel as there was no provision of >> drawing the radiobutton alongside icon. >> If icon is not there, the radiobutton is drawn. Added provision of drawing >> the radiobutton windows Skin even when imageIcon is present. > > Prasanta Sadhukhan has updated the pull request incrementally with two > additional commits since the last revision: > > - remove test file > - Move text position w.r.t menuItem icon This is how it looks on Windows 10:  It's close, yet the spacing is wrong. The bullet / check-mark should be where they were before the fix. There was 8-pixel margin to the left of selected bullet background before the fix, now there are only 2 pixels. The margin between the bullet background and the text was again 8 pixels, I think we should maintain the same margin between the bullet and the icon as well as between the icon and the menu text. - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2643787270
Re: RFR: 8349351 : Combine Screen Inset Tests into a Single File
On Fri, 7 Feb 2025 17:29:58 GMT, Harshitha Onkar wrote: >> Is it needed? If there are no failures, I see no reason to increase the >> tolerance. > > If it test runs fine on CI then it should be okay. Previously, I came across > few tests which required increasing tolerance value (although not in this > context but particularly for pixel color case). Tolerance when comparing colours is different from comparing sizes. We know that fractional pixels after scaling up and down may be lost, that's where the tolerance comes from. - PR Review Comment: https://git.openjdk.org/jdk/pull/23449#discussion_r1947080032
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Fri, 7 Feb 2025 05:26:10 GMT, Prasanta Sadhukhan
wrote:
>> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is
>> shown without radiobutton in WIndowsLookAndFeel as there was no provision of
>> drawing the radiobutton alongside icon.
>> If icon is not there, the radiobutton is drawn. Added provision of drawing
>> the radiobutton windows Skin even when imageIcon is present.
>
> Prasanta Sadhukhan has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - remove test file
> - Move text position w.r.t menuItem icon
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java line
662:
> 660: paintCheckIcon(g, lh, lr, holdc, foreground);
> 661: paintIcon(g, lh, lr, holdc);
> 662: if (UIManager.getLookAndFeel().getName().equals("Windows")
Can't this be handled in `WindowsMenuItemUI` instead? After all, Metal and
Windows L&F looked differently.
test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItemAndJCheckBoxMenuItem.java
line 1:
> 1: /*
Does it have to be that long? `TestRadioAndCheckMenuItemWithIcon.java` looks
descriptive enough.
test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItemAndJCheckBoxMenuItem.java
line 118:
> 116: buttonGroup1.add(check1);
> 117: buttonGroup1.add(check2);
> 118: buttonGroup1.add(check3);
I expect that each check-box can be selected independently from others.
test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItemAndJCheckBoxMenuItem.java
line 137:
> 135: frame.setJMenuBar(menuBar);
> 136: frame.setSize(300, 300);
> 137: frame.setLocationRelativeTo(null);
`setLocationRelativeTo` is redundant, the location of the frame is controlled
by `PassFailJFrame`.
-
PR Review: https://git.openjdk.org/jdk/pull/23324#pullrequestreview-2602598579
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1947041075
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1947049448
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1947056340
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1947057458
Re: RFR: 8349351 : Combine Screen Inset Tests into a Single File
On Thu, 6 Feb 2025 23:45:43 GMT, Harshitha Onkar wrote: >> While working on [JDK-6899304](https://bugs.openjdk.org/browse/JDK-6899304), >> we discovered that there are two tests meant to perform the same task. >> >> The first test is located at >> test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java >> and was originally designed for multi-screen configurations on Linux >> platforms. >> >> The second test, located at >> test/jdk/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java, is >> intended for single-screen configurations but lacks accuracy due to some >> workarounds to support Windows. >> >> Now, the test at >> test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java >> has been updated to work across all platforms, including Windows, which was >> previously failing. As a result, it has been agreed to rename this test to >> ScreenInsetsTest, remove the condition that restricted its execution to >> multi-screen configurations, and remove the redundant test at >> test/jdk/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java. > > test/jdk/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java line 47: > >> 45: private static final int SIZE = 100; >> 46: // Allow a margin tolerance of 1 pixel due to scaling >> 47: private static final int MARGIN_TOLERANCE = 1; > > Since this test is extended to both single and multi monitor, I think > tolerance can be increased to 2-3 (considering this test runs on CI now). Is it needed? If there are no failures, I see no reason to increase the tolerance. - PR Review Comment: https://git.openjdk.org/jdk/pull/23449#discussion_r1946827397
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v10]
On Fri, 7 Feb 2025 05:28:28 GMT, Prasanta Sadhukhan wrote: > It seems in windows 11, the menuitem text location is dependant on presence > of menuitem icon. As per this screenshot, the bullet/checkmark seems to be in > its fixed location but text is shifted depending on menuitem icon. That's what [I said](https://github.com/openjdk/jdk/pull/23324#issuecomment-2624442850): > If we add support for painting both an ImageIcon and the bullet / check-mark, > the layout of the menu item has to change. --- > Modified the PR to look the same. Thank you. I'll test it. However, I think that all text in a popup menu should move to the right… that is all text in menu items should remain aligned if there bullets/check-marks and icons in a particular popup menu. That is the text for the third menu items for radio button and for check mark should also move. Whether all text is aligned [could be controlled by a property](https://github.com/openjdk/jdk/pull/23324#discussion_r1941070014): > We may introduce a property that controls this behaviour: whether the text > aligns in all the menu items or not. There's a sample in [the linked message](https://github.com/openjdk/jdk/pull/23324#discussion_r1941070014). - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2642667698
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 5 Feb 2025 11:50:36 GMT, Alexey Ivanov wrote: >>> Nothing stops you from doing so… >> >> I agree but I have never seen any application which is having a single >> RadioButton for selection. >> >>> I still think the condition in the radio button case should be the same: >>> Object.equals handles all the cases, and the code becomes consistent. >>> Otherwise, the radio button stands out… for no apparent reason. >> >> For consistency, it can be done. I need to check if it impacts on VO >> announcement. > >> For consistency, it can be done. I need to check if it impacts on VO >> announcement. > > It shouldn't impact the announcements. One button gets deselected, another > one gets selected… > > Yet now that I think about it more, we may want to skip announcing > deselecting a button in the group when selection moves to another button. > > What about the case where currently focused or unfocused selected button gets > deselected programmatically? Is there an interactive way to deselect a radio > button when it's the only button in its own group? > > This *needs more testing*. > @aivanov-jdk I think we should not change RadioButton's implementation as the > announcement is not consistent. Do you have any other suggestion ? Yes, I agree. I just wanted to test it myself too, and I haven't been able to yet. - PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1946381166
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v9]
On Fri, 7 Feb 2025 03:10:03 GMT, Prasanta Sadhukhan wrote: > I am not sure if we can satisfy both windows 10 and windows 11 simultaneously > as both design is different for this logic... We absolutely can… I'm not proposing using different rendering, instead I'm for following Windows 11 rendering, but it requires updating menu layout code. > Without the fix, it looks good as Windows 10 as it doesn't support separate > bullet/checkmark skin whereas windows 11 does... I'm unsure Windows 11 does support it natively, likely File Explorer implements custom drawing code. I'm pretty sure the situation is the same with Windows 10, yet none of use was able to find an app which displays both icons and bullets/check-marks. The commonly available feature in Windows is to display an icon, which you can see in window menu activated by Alt+Space or right-clicking the window title bar: Restore, Minimise, Maximise, and Close items display an icon — this icon is rendered centred exactly at the position where radio bullet or check mark are. I may be able to play around in a Win32 app with it. > Also, its difficult to test for me as I do not have windows 10 so any change > for windows 11 whether it works for windows10 is difficult to make out, also > considering the fact mainline jdk is not supporting windows10 so not sure why > we should introduce additional logic to satisfy windows10 My point here is that the current approach doesn't look similar to how File Explorer renders menu with bullets and icons. If you do it, this style would work just fine for Windows 10 too: the bullet will have its own place, the icon will have its own place. Even though Windows 10 is not going to be supported by JDK 25, it doesn't make sense to break anything that works there. After all, we had to restore theming support for Windows 7 support for which ended long-long time ago. - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2642621656
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v6]
On Wed, 5 Feb 2025 13:59:00 GMT, Prasanta Sadhukhan wrote: >> I have modified the PR to move the icon w.r.t bullet skin width and it looks >> like this in windows 11..I believe this should work for windows 10 also but >> I dont have windows10 to check >> >>  >> >> >> I dont think we can tamper with text location as it is done in >> WindowsMenuItemUI#paintText which does not have any knowledge of bullet/icon >> presence. >> Also, ideally I dont think this should be applicable for windows10 and we >> should do this only for windows11 but I am not sure if there is version >> check in our code and anyway, windows10 would be EOL-ed and unsupported >> platform in few months > > It looks ok in windows 11 with both icon selected/unselected > >  > >  > > I would like to know how it looks in windows 10? Is there any issue? > > Although, as per https://bugs.openjdk.org/browse/JDK-8349268 windows 10 is > not supported in JDK25 and this fix is going in mainline jdk25 > I dont think we can tamper with text location as it is done in > WindowsMenuItemUI#paintText which does not have any knowledge of bullet/icon > presence. Why can't we? `paintText` paints the text into a rectangle that's passed to it. It's up to menu layout code to determine at what location the text should be painted. If we want to mimic Windows 11 File Explorer menu, which we should because it's the platform app that renders both radio-bullets and icons in its menu, then we need to update the menu layout code — otherwise the spacing between icons and text is way off. The currently suggested fix can be used as a workaround, however, I'd rather we do it in a consistent way with what Windows 11 File Explorer does. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1945188262
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v9]
On Tue, 4 Feb 2025 14:16:16 GMT, Prasanta Sadhukhan wrote: >> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is >> shown without radiobutton in WIndowsLookAndFeel as there was no provision of >> drawing the radiobutton alongside icon. >> If icon is not there, the radiobutton is drawn. Added provision of drawing >> the radiobutton windows Skin even when imageIcon is present. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > retain diff of OFFSET between skin background start coord and skin > coordinate On that note, Swing's menu rendering doesn't blend with the way Windows 11 renders menus: 1. Menu has rounded corners, 2. The highlight color is just slightly darked than the menu background, 3. The highlight rectangle also has rounded.  This is out of scope of this issue, I just wanted to bring it up. I guess I should submit a bug against this, it would be more relevant as support for Windows 10 is phased out. Swing uses the theming API to paint menus, yet it differs now. - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2640637956
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v9]
On Tue, 4 Feb 2025 14:16:16 GMT, Prasanta Sadhukhan wrote: >> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is >> shown without radiobutton in WIndowsLookAndFeel as there was no provision of >> drawing the radiobutton alongside icon. >> If icon is not there, the radiobutton is drawn. Added provision of drawing >> the radiobutton windows Skin even when imageIcon is present. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > retain diff of OFFSET between skin background start coord and skin > coordinate I tested it on Windows 11 and 10. **Windows 11**  It looks reasonably well… But it doesn't look like the menu in Windows 11 File Explorer, does it?  Windows L&F should look like native apps do, and we're not there yet with the menu rendering. **Windows 10**  It looks bad because neither icon is where it should be. Without the fix, it looks good:  And one can see if an item is selected or not. Yet they changed the skin in Windows 11, now it's impossible to tell distinguish them. --- The updated test is available at [commit 7aa4de1](https://github.com/openjdk/jdk/blob/7aa4de1b5446126a7732f11a6c94839a396ff8bf/test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java). - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2640591767
Re: RFR: 8328977 : JEditorPane.setPage not thread-safe, pageLoader not cancelled [v3]
On Fri, 19 Apr 2024 14:53:09 GMT, Renjith Kannath Pariyangad
wrote:
>> Hi Reviewers,
>>
>> Added pageloader cancel before new page creation along with code
>> restructuring. Moved all page loading calls inside synchronize to make it
>> thread safe.
>>
>> Regards,
>> Renjith.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with
> one additional commit since the last revision:
>
> Rearranged if based on suggesion
There are lots of failures with the message: _java.lang.NullPointerException:
Cannot invoke "java.net.URL.getProtocol()" because "u2" is null_
For example, the `javax/swing/JEditorPane/4492274/bug4492274.java` test fails
with the following stack traces:
--System.err:(32/2233)--
java.lang.reflect.InvocationTargetException
at java.desktop/java.awt.EventQueue.invokeAndWait(EventQueue.java:1312)
at java.desktop/java.awt.EventQueue.invokeAndWait(EventQueue.java:1287)
at
java.desktop/javax.swing.SwingUtilities.invokeAndWait(SwingUtilities.java:1474)
at bug4492274.main(bug4492274.java:50)
at
java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at
com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
at java.base/java.lang.Thread.run(Thread.java:1447)
Caused by: java.lang.RuntimeException: java.lang.NullPointerException: Cannot
invoke "java.net.URL.getProtocol()" because "u2" is null
at bug4492274.createAndShowGUI(bug4492274.java:113)
at bug4492274$1.run(bug4492274.java:53)
at
java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:313)
at …
at
java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)
Caused by: java.lang.NullPointerException: Cannot invoke
"java.net.URL.getProtocol()" because "u2" is null
at
java.base/java.net.URLStreamHandler.sameFile(URLStreamHandler.java:413)
at java.base/java.net.URL.sameFile(URL.java:1123)
at java.desktop/javax.swing.JEditorPane.setPage(JEditorPane.java:478)
at bug4492274.createAndShowGUI(bug4492274.java:104)
... 10 more
src/java.desktop/share/classes/javax/swing/JEditorPane.java line 480:
> 478: final String reference = page.getRef();
> 479:
> 480: if ((postData == null) && page.sameFile(loaded)) {
This line of code causes lots of test failures because `page.sameFile(loaded)`
doesn't accept a `null` parameter, which results in `NullPointerException`.
-
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18670#pullrequestreview-2595528235
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1942776674
Re: RFR: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable [v2]
On Fri, 31 Jan 2025 20:40:27 GMT, Alexey Ivanov wrote: >> The `javax/swing/JButton/4796987/bug4796987.java` test strictly verifies if >> it's run on Windows XP, for this reason it never runs on modern versions of >> Windows. >> >> Since Windows XP is obsolete and visual styles cannot be disabled since >> Windows 8, I removed the OS version check completely. >> >> I captured an image of the panel and analyse colors in the image instead of >> using `Robot.getPixelColor`; I save the image for reference if the test >> fails. >> >> The updated test passes in the CI. > > Alexey Ivanov has updated the pull request incrementally with two additional > commits since the last revision: > > - Update the test summary > - Remove the redundant button @mrserb Any other comments? - PR Comment: https://git.openjdk.org/jdk/pull/23336#issuecomment-2636516796
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 5 Feb 2025 11:43:18 GMT, Abhishek Kumar wrote: > For consistency, it can be done. I need to check if it impacts on VO > announcement. It shouldn't impact the announcements. One button gets deselected, another one gets selected… Yet now that I think about it more, we may want to skip announcing deselecting a button in the group when selection moves to another button. What about the case where currently focused or unfocused selected button gets deselected programmatically? Is there an interactive way to deselect a radio button when it's the only button in its own group? This *needs more testing*. - PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1942729376
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 5 Feb 2025 06:44:45 GMT, Abhishek Kumar wrote: > Don't think RadioButton can be used stand-alone. Nothing stops you from doing so… I still think the condition in the radio button case should be the same: `Object.equals` handles all the cases, and the code becomes *consistent*. Otherwise, the radio button stands out… for no apparent reason. - PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1942712616
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Wed, 5 Feb 2025 07:04:21 GMT, Abhishek Kumar wrote: >> VoiceOver doesn't announce the _untick state_ when Checkbox is `deselected` >> using **space** key. When CheckBox is deselected, the state change is not >> notified to a11y client (VoiceOver) and so the state is not announced by VO. >> >> Screen Magnifier is also unable to magnify the unchecked state of JCheckBox >> due to same reason and is captured as separate bug >> [JDK-8345728](https://bugs.openjdk.org/browse/JDK-8345728). >> >> Proposed fix is to send the state change notification to a11y client when >> checkbox is deselected, this resolves the problem for VoiceOver and Screen >> Magnifier. >> >> Similar issue observed for JToggleButton. So, I extended the fix for >> JToggleButton as well. >> >> The proposed change can be verified the manual test in the PR. >> >> CI pipeline testing is `OK`, link posted in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > Condition evaluation simplified I [insist](https://github.com/openjdk/jdk/pull/23436#discussion_r1942712616) on changing the condition for radio buttons, too. Other than that, it looks good to me. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23436#pullrequestreview-2595421026
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS [v2]
On Tue, 4 Feb 2025 13:24:29 GMT, Artem Semenov wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Condition evaluation simplified
>
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CAccessible.java line 186:
>
>> 184: if (thisRole == AccessibleRole.CHECK_BOX) {
>> 185: if ((newValue != null &&
>> !newValue.equals(oldValue)) ||
>> 186: oldValue != null &&
>> !oldValue.equals(newValue)) {
>
> At first glance, these conditions look like the same thing. equals() above
> will return false if oldValue is null. oldValue.equals(newValue) and
> newValue.equals(oldValue) are also the same thing. Try to rewrite this
> condition more clearly.
I think @savoptik meant that `!oldValue.equals(newValue)` was redundant.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1942708541
Re: RFR: 8328977 : JEditorPane.setPage not thread-safe, pageLoader not cancelled [v3]
On Tue, 4 Feb 2025 18:19:22 GMT, Alexey Ivanov wrote: > Both of us ran the client tests. I might misremember things… Yet I see test failures with the proposed changeset. **The PR should not be *sponsored***. - PR Comment: https://git.openjdk.org/jdk/pull/18670#issuecomment-2635202834
Re: RFR: 8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering
On Tue, 4 Feb 2025 00:07:48 GMT, anass baya wrote: > The PR includes two fix : > 1 - The first fix targets proper rendering of the transparent image. > 2 - The second fix ensures the image is painted at the correct resolution to > avoid pixelation. @mickleness I wonder what is your preferred email address and name. You were referenced by another email in [your recent contribution](https://bugs.openjdk.org/browse/JDK-8342782?focusedId=14733170&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14733170). - PR Comment: https://git.openjdk.org/jdk/pull/23430#issuecomment-2635153763
Re: RFR: 8328977 : JEditorPane.setPage not thread-safe, pageLoader not cancelled [v3]
On Thu, 9 May 2024 19:27:47 GMT, Phil Race wrote: > Changes seem fine, but I would like to hear how this was tested ? @prrace Renjith and I went over all the cases. We tried to move the cases where no action is needed to the top of the method. > And if there is a specific test that exists for this, or why one could not be > created. Both of us ran the client tests. I'm unaware of any test in the OpenJDK which tests this functionality… It's hard to test reliably, especially the asynchronous cases… However, it may still be possible to test some scenarios… - PR Comment: https://git.openjdk.org/jdk/pull/18670#issuecomment-2634734034
Re: RFR: 8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering
On Tue, 4 Feb 2025 00:07:48 GMT, anass baya wrote: > The PR includes two fix : > 1 - The first fix targets proper rendering of the transparent image. > 2 - The second fix ensures the image is painted at the correct resolution to > avoid pixelation. @anass-baya If your fix is based on @mickleness's previous PR, you should him as co-author using the [`/contributor`](https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/contributor) command. - PR Comment: https://git.openjdk.org/jdk/pull/23430#issuecomment-2634683856
Re: RFR: 8348936: [Accessibility,macOS,VoiceOver] VoiceOver doesn't announce untick on toggling the checkbox with "space" key on macOS
On Tue, 4 Feb 2025 10:58:20 GMT, Abhishek Kumar wrote:
> VoiceOver doesn't announce the _untick state_ when Checkbox is `deselected`
> using **space** key. When CheckBox is deselected, the state change is not
> notified to a11y client (VoiceOver) and so the state is not announced by VO.
>
> Screen Magnifier is also unable to magnify the unchecked state of JCheckBox
> due to same reason and is captured as separate bug
> [JDK-8345728](https://bugs.openjdk.org/browse/JDK-8345728).
>
> Proposed fix is to send the state change notification to a11y client when
> checkbox is deselected, this resolves the problem for VoiceOver and Screen
> Magnifier.
>
> Similar issue observed for JToggleButton. So, I extended the fix for
> JToggleButton as well.
>
> The proposed change can be verified the manual test in the PR.
>
> CI pipeline testing is `OK`, link posted in JBS.
Changes requested by aivanov (Reviewer).
src/java.desktop/macosx/classes/sun/lwawt/macosx/CAccessible.java line 186:
> 184: if (thisRole == AccessibleRole.CHECK_BOX) {
> 185: if ((newValue != null &&
> !newValue.equals(oldValue)) ||
> 186: oldValue != null &&
> !oldValue.equals(newValue)) {
Suggestion:
if (!Objects.equals(newValue, oldValue)) {
This handles all the cases, including `null` values.
src/java.desktop/macosx/classes/sun/lwawt/macosx/CAccessible.java line 212:
> 210: // Do send toggle button state changes to native side
> 211: if (thisRole == AccessibleRole.TOGGLE_BUTTON) {
> 212: if ((newValue != null &&
> !newValue.equals(oldValue)) ||
I believe the new condition should be used for `RADIO_BUTTON` too:
https://github.com/openjdk/jdk/blob/b985347c2383a7a637ffa9a4a8687f7f7cde1369/src/java.desktop/macosx/classes/sun/lwawt/macosx/CAccessible.java#L197-L200
test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 34:
> 32: /*
> 33: * @test
> 34: * @bug 8348936
Suggestion:
* @bug 8348936 8345728
Two bugs are fixed by one changeset, and your summary implies either issue is
tested by this single test.
test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 58:
> 56: 7. Press Tab to move focus to ToggleButton
> 57: 8. Repeat steps 3 to 6 and listen the announcement
> 58: 9. If announcements are incorrect, Press Fail
Suggestion:
9. If announcements are incorrect, press Fail
test/jdk/javax/accessibility/TestJCheckBoxToggleAccessibility.java line 87:
> 85:
> 86: Press Pass if you are able to hear correct VoiceOver
> announcements and
> 87: able to see the correct screen magnifier behaviour. """;
These long instructions may benefit from using HTML for formatting instructions.
-
PR Review: https://git.openjdk.org/jdk/pull/23436#pullrequestreview-2593380133
PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1941484674
PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1941487923
PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1941492200
PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1941493443
PR Review Comment: https://git.openjdk.org/jdk/pull/23436#discussion_r1941495810
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v6]
On Tue, 4 Feb 2025 11:39:48 GMT, Prasanta Sadhukhan wrote: > But that's what I believe is been done now in the PR test, corresponding to > your 1st image and corresponds to windows11 No, it doesn't. On Windows 10 which has a selected background around the bullet, it looks completely off.   [The original behaviour](https://github.com/openjdk/jdk/pull/23324#issuecomment-2624726843) was consistent at least. Yet what the currently proposed fix produces doesn't look right at all. The bullet or check-mark should remain centered in their own place as if there's no icon. > Are you suggesting to keep more gap in between checkmark and button? Yes, but not only. If there's an icon, it should move the text to the right to accommodate the additional icon, if we're going this route. In addition to that, the text in all other menu items should move to the right for consistency so that all the menu items are aligned and items without an icon render an empty icon in this case. We may introduce a property that controls this behaviour: whether the text aligns in all the menu items or not. The first option corresponds to this layout:  where all the text moves right to add space for menu icons. This corresponds to menu layout in File Explorer in Windows 11. The second option corresponds to this layout:  where the second menu item remains at the position where it's rendered now. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1941070014
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v6]
On Tue, 4 Feb 2025 11:13:48 GMT, Prasanta Sadhukhan wrote: >>> Why offset is multiplied by 3? >> >> I believe this is likely how layout is calculated. >> >>> Buffered Image in test code is of size 16x16. Is it possible that the icon >>> can be of different size? >> >> It's definitely possible… Yet it's pretty common to use 16×16 (small) icons >> in menus. >> >> This question is applicable to other L&Fs, yet the problem could be more >> prominent in Windows L&F. >> >>> does it depend on the icon size? >>> If Icon size is more then I guess it may overlap with radio button skin. >> >> This is why we should re-work the layout of menus with icons if there's any >> radio- or check-menu item as I [suggested >> above](https://github.com/openjdk/jdk/pull/23324#discussion_r1935784688). > >> This is why we should re-work the layout of menus with icons if there's any >> radio- or check-menu item as I [suggested >> above](https://github.com/openjdk/jdk/pull/23324#discussion_r1935784688). > > Not sure I understand...As in windows11 Explorer, radiobutton checkmark and > icon are at the same position as in jdk.. > what more you are suggesting to change? Radio- or checks are in the same position but ***the icon itself** is moved to the right* so that both the bullet or check and the icon are displayed. Currently, only the icon is rendered at the place where the bullet or checkmark are if there's no icon. The result should like this  instead of  It is the former image that corresponds to File Explorer in Windows 11: https://github.com/openjdk/jdk/pull/23324#discussion_r1935744523 - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1940990299
Re: RFR: 8328977 : JEditorPane.setPage not thread-safe, pageLoader not cancelled [v3]
On Fri, 19 Apr 2024 14:53:09 GMT, Renjith Kannath Pariyangad wrote: >> Hi Reviewers, >> >> Added pageloader cancel before new page creation along with code >> restructuring. Moved all page loading calls inside synchronize to make it >> thread safe. >> >> Regards, >> Renjith. > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > Rearranged if based on suggesion I believe it's premature to integrate it right away. At the very least, you should merge master and re-run all the client tests before integrating. Even though the PR is approved, there's [an unresolved discussion](https://github.com/openjdk/jdk/pull/18670#pullrequestreview-2048663406). - PR Comment: https://git.openjdk.org/jdk/pull/18670#issuecomment-2633596869
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v10]
On Tue, 4 Feb 2025 06:16:29 GMT, ScientificWare wrote: > Awaiting review. I've been unable to take a look at the updated code yet. I'll look at the changes as soon as I can. - PR Comment: https://git.openjdk.org/jdk/pull/15319#issuecomment-2633589047
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v2]
On Thu, 30 Jan 2025 07:41:33 GMT, Abhishek Kumar wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> formatting
>
> test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java
> line 45:
>
>> 43: public class TestImageIconWithJRadioButtonMenuItem {
>> 44:
>> 45: private static final String instructionsText = """
>
> Suggestion:
>
> private static final String INSTRUCTIONSTEXT = """
For what it's worth, it should be `INSTRUCTIONS_TEXT` with the underscore
separating words.
It's not required to capitalise private constants, only public constants use
all capitals with underscores between words.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1940969316
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v6]
On Tue, 4 Feb 2025 10:02:22 GMT, Abhishek Kumar wrote: > Why offset is multiplied by 3? I believe this is likely how layout is calculated. > Buffered Image in test code is of size 16x16. Is it possible that the icon > can be of different size? It's definitely possible… Yet it's pretty common to use 16×16 (small) icons in menus. This question is applicable to other L&Fs, yet the problem could be more prominent in Windows L&F. > does it depend on the icon size? > If Icon size is more then I guess it may overlap with radio button skin. This is why we should re-work the layout of menus with icons if there's any radio- or check-menu item as I [suggested above](https://github.com/openjdk/jdk/pull/23324#discussion_r1935784688). - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1940965640
Re: RFR: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable [v2]
On Mon, 3 Feb 2025 18:07:38 GMT, Abhishek Kumar wrote: >> Why not? Visual styles were introduced in Windows XP. >> >> Even though the test was written when both Windows Vista and Windows 7 had >> already been released, it was limited to Windows XP only for some reason. > > Since it will never be tested on Windows XP machine, I thought it's not worth > to mention. It just references the point where the feature was introduced. Currently supported versions of Java likely don't start on Windows XP any more, but it doesn't change the fact that visual styles were introduced in Windows XP. - PR Review Comment: https://git.openjdk.org/jdk/pull/23336#discussion_r1939863738
Re: RFR: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable [v2]
On Mon, 3 Feb 2025 06:38:28 GMT, Abhishek Kumar wrote: >> Alexey Ivanov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Update the test summary >> - Remove the redundant button > > test/jdk/javax/swing/JButton/4796987/bug4796987.java line 30: > >> 28: * @requires (os.family == "windows") >> 29: * @summary Verify JButton.setBorderPainted(false) removes border >> 30: * for Windows visual styles (Windows XP and later) > > Is it necessary to mention about Windows XP ? Why not? Visual styles were introduced in Windows XP. Even though the test was written when both Windows Vista and Windows 7 had already been released, it was limited to Windows XP only for some reason. - PR Review Comment: https://git.openjdk.org/jdk/pull/23336#discussion_r1939789226
Re: RFR: 8348675: TrayIcon tests fail in Ubuntu 24.10 Wayland [v2]
On Tue, 28 Jan 2025 18:40:00 GMT, Alexander Zvegintsev wrote: >> Several TrayIcon tests are trying to click on the system tray icon with >> Robot using XTest API. >> >> Basically we have the same kind of failures as before, e.g. >> [JDK-8280990](https://bugs.openjdk.org/browse/JDK-8280990) >>> the reason is this is using XTEST, an X11 protocol which will not work >>> outside of X11. >>> >>> In other words, the emulated input event reaches the X11 clients, but not >>> the Wayland compositor which is the actual display server but also the X11 >>> window manager in Wayland, the component which is in charge of >>> moving/resizing/stacking the windows. >> >> I also tested the same tests by clicking manually instead of using the >> robot, and it works as expected. >> So for now, skip those tests on Wayland (and do some minor cleanup). >> >> Testing after modifications is also green. > > Alexander Zvegintsev has updated the pull request incrementally with two > additional commits since the last revision: > > - minor > - review comments Marked as reviewed by aivanov (Reviewer). test/jdk/java/awt/TrayIcon/TrayIconPopup/TrayIconPopupClickTest.java line 79: > 77: "\"Always show all icons and notifications on the > taskbar\" true " + > 78: "to avoid this problem. Or change behavior only for > Java SE " + > 79: "tray icon."); These instructions are outdated, at the same time, they're visible only on the test log… *I'd rather leave them as is* — it's out of scope to amend the instructions to be accurate for Windows 10 and 11. - PR Review: https://git.openjdk.org/jdk/pull/23329#pullrequestreview-2587845730 PR Review Comment: https://git.openjdk.org/jdk/pull/23329#discussion_r1937964332
Re: RFR: 8348865: JButton/bug4796987.java never runs because Windows XP is unavailable [v2]
> The `javax/swing/JButton/4796987/bug4796987.java` test strictly verifies if > it's run on Windows XP, for this reason it never runs on modern versions of > Windows. > > Since Windows XP is obsolete and visual styles cannot be disabled since > Windows 8, I removed the OS version check completely. > > I captured an image of the panel and analyse colors in the image instead of > using `Robot.getPixelColor`; I save the image for reference if the test fails. > > The updated test passes in the CI. Alexey Ivanov has updated the pull request incrementally with two additional commits since the last revision: - Update the test summary - Remove the redundant button - Changes: - all: https://git.openjdk.org/jdk/pull/23336/files - new: https://git.openjdk.org/jdk/pull/23336/files/62833cbc..0349f37a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23336&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23336&range=00-01 Stats: 5 lines in 1 file changed: 1 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23336.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23336/head:pull/23336 PR: https://git.openjdk.org/jdk/pull/23336
Re: RFR: 8345538: Robot.mouseMove doesn't clamp bounds on macOS when trying to move mouse off screen [v6]
On Thu, 30 Jan 2025 20:21:16 GMT, Alisen Chung wrote: > > I would like to clarify one point: if the robot moves the mouse off the > > screen while the actual mouse pointer is on the screen and immediately > > presses the mouse button, where will the click occur? on or off the screen? > > In macOS code there is similar method that checks for mouse offscreen > locations called checkMousePos() which updates and clamps the mousePosition > stored in CRobot and is called after mousePress and mouseRelease In macOS? Do you mean in macOS-specific code in JDK? ~~If the code to clamp mouse coordinates has already been written, why are you writing new code instead of re-using the existing code?~~ However, the code in [`CRobot.checkMousePos`](https://github.com/openjdk/jdk/blob/651ac3cc0f2a8b3edf5cddb42df1d38d4aa0e1a6/src/java.desktop/macosx/classes/sun/lwawt/macosx/CRobot.java#L98) doesn't really clamp mouse coordinates. The value of `MOUSE_LOCATION_UNKNOWN` is -1: https://github.com/openjdk/jdk/blob/651ac3cc0f2a8b3edf5cddb42df1d38d4aa0e1a6/src/java.desktop/macosx/classes/sun/lwawt/macosx/CRobot.java#L36 This means it may be impossible to click at (-1, -1) which could be perfectly valid coordinates. It looks like _a bug_, actually. > > nonexistent coordinates. > > But why this coordinates nonexistent, you at least can move undecorated > windows there, that coordinates just is not visible on teh screen. This is the question we have to answer first: Are the coordinates off the virtual screen invalid, non-existent? What we've seen so far is `Robot.mouseMove` succeeds in a way that mouse appears to be moved to the specified location. That location gets returned by the OS, at least on macOS, when the OS-specific native code in `MouseInfo.getPointerInfo()` reads the coordinates — it is the code in `MouseInfo.getPointerInfo()` that clamps the coordinates to known screen devices and returns `null`, if the coordinates are outside of these bounds: https://github.com/openjdk/jdk/blob/651ac3cc0f2a8b3edf5cddb42df1d38d4aa0e1a6/src/java.desktop/share/classes/java/awt/MouseInfo.java#L84-L86 This is because `PointerInfo` requires both the device and the coordinates. Then, when you use a physical mouse, the OS doesn't allow moving the mouse cursor outside of the virtual screen bounds. Does this imply Java should prevent moving the mouse cursor off the virtual screen, too? - PR Comment: https://git.openjdk.org/jdk/pull/22781#issuecomment-2628256432
Re: RFR: 8336382: Fix error reporting in loading AWT [v10]
On Fri, 31 Jan 2025 13:33:13 GMT, Magnus Ihse Bursie wrote: > See https://bugs.openjdk.org/browse/JDK-8349099 > > @Karm I was wondering why this did not show up on GHA (it's probably not > executed there). But I noticed when researching this that your last merge > from mainline was in September. Even if there are no "physical" merge > conflicts that git can detect, there are likely to be "logical" conflicts > like this, where a library was removed that your test depended on. I thought I'd run the client tests with changes from this PR, and I usually merge master into my local branch before submitting a job. Yet I can't find the job, I might not have run the tests. - PR Comment: https://git.openjdk.org/jdk/pull/20169#issuecomment-2628103573
Re: RFR: 6899304 : java.awt.Toolkit.getScreenInsets(GraphicsConfiguration) returns incorrect values [v8]
On Fri, 31 Jan 2025 02:36:50 GMT, Harshitha Onkar wrote: > @anass-baya Fix looks good to me. > > Initially the test passed without the fix, I realized that I had the option > **"Show taskbar on all displays"** checked in Settings, after unchecking it > the test works as expected with the fix. I have the option **Show taskbar on all displays** selected, yet the test didn't work for me initially. It depends on whether the main monitor happens to be the first screen in graphics environment. If not, you get wrong results. I cannot reproduce the problem at this moment, my main monitor is the first one in the list. I [reproduced the problem](https://bugs.openjdk.org/browse/JDK-6899304?focusedId=14594269&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14594269) when three monitors were connected, I used a different laptop at that time. > Suggestion: Based on other reviewers opinion you can choose to combine > MultiScreenInsetsTest & ScreenInsetsTest. [#23183 > (comment)](https://github.com/openjdk/jdk/pull/23183#discussion_r1936568297) Yes, we should combine the two tests. However, I'd rather do it separately. - PR Comment: https://git.openjdk.org/jdk/pull/23183#issuecomment-2626920314
Re: RFR: 8344581: [TESTBUG] java/awt/Robot/ScreenCaptureRobotTest.java failing on macOS [v4]
On Thu, 30 Jan 2025 13:26:08 GMT, Naveen Narayanan wrote: >> This contains test fixes for the following issue in >> ScreenCaptureRobotTest.java >> >> •Failures in Mac OS because of mouse pointer visible on top of the image >> that is screen captured by robot. >> >> Testing: >> Tested using Mach5(100 times per platform) in MacOS, Linux, Windows & MacOS >> AArch64. Seen all tests passing. > > Naveen Narayanan has updated the pull request incrementally with one > additional commit since the last revision: > > 8344581: java/awt/Robot/ScreenCaptureRobotTest.java failing on macOS Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23264#pullrequestreview-2584260664
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v5]
On Thu, 30 Jan 2025 14:55:59 GMT, Prasanta Sadhukhan wrote: > I found one in windows11 in Explorer which seems to use both icon and bullet > checkmark and the present PR do the same Yeah, you're right, it looks similar. Yet the currently proposed fix in this PR is still off. The bullet or check-mark should be displayed where it is now. What I mean by this is that this `if` statement needs to be removed: https://github.com/openjdk/jdk/blob/fac63d4383c931ea515dcdf7a89e4285f753f41b/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java#L878-L881 If you remove it, then the bullet or check-mark will be rendered at its own place. Yet if there's an icon associated with a menu item, all the text in other menus should shift to make room for the added icon. (Note that this menu in File Explorer in Windows 11 has icons on each and every menu item.)  - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935784688
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v5]
On Thu, 30 Jan 2025 12:49:14 GMT, Prasanta Sadhukhan wrote: >> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is >> shown without radiobutton in WIndowsLookAndFeel as there was no provision of >> drawing the radiobutton alongside icon. >> If icon is not there, the radiobutton is drawn. Added provision of drawing >> the radiobutton windows Skin even when imageIcon is present. > > Prasanta Sadhukhan has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove duke > - RadioButton not drawn. ImageIcon on the fly Based on [the discussion above](https://github.com/openjdk/jdk/pull/23324#discussion_r1935589364) in relation to [JDK-6432667](https://bugs.openjdk.org/browse/JDK-6432667) where the current behaviour/rendering was implemented, this quirk may be *a feature* of Windows L&F rather than a bug.  Windows 10 has a highlight around the selected icon. Yet there's no such highlight in Windows 11, therefore it's hard to distinguish whether a menu item has a selected state of not. - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2624726843
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v5]
On Thu, 30 Jan 2025 13:28:00 GMT, Prasanta Sadhukhan wrote: > The bug says > > > in case icon is defined for JCheckBoxMenuItem or > > JRadioButtonMenuItem this icon is used as a check/radio mark. themed > > background is used to show the selection. > > so I guess it was done intentionally to not draw checkmark if icon is there, > but I am not getting any menuitem with icon and radiobutton selection > natively in windows11 to compare..anything in windows10? I think so. Vista and Windows 7 had a kind of separator in the menu between text and icons.  This screenshot shows how menu looked like in Windows 7, the *Word Wrap* item is selected. I don't think Windows has ever supported both icons and bullets / check marks… unless you drew menu items yourself. The icons in menus were always displayed in that area. The right-click menu on the desktop in Windows 10 is a good example; Windows 11 uses a completely different style of menus on the desktop and in Explorer. I can't find any native Windows app which displayed both states. The closest I found are Paint.NET and VirtualBox. Paint.NET  Here *Pixel Grid* is selected. It uses a selected background and a highlight square around the icon. The *Pixels* item is also selected, but it has no icon, it displays a check-mark (even though it should've been a bullet: only one of _Pixels_, _Inches_, _Centimeters_ can be selected). VirtualBox  In this case, the *Pause* item is selected. VirtualBox adds both a selected background as well as a check-mark overlay to indicate the item is currently selected. Neither Paint.NET nor VirtualBox fully rely on Windows to render their menus, either app uses owner-drawn menus or its own controls. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935724542
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v5]
On Thu, 30 Jan 2025 13:18:25 GMT, Prasanta Sadhukhan wrote: > > You shouldn't render an unselected radio button in menu. > > Please check updated PR… I saw your update. I wrote this comment before you removed rendering the unselected radio button. > …share your testcase Here's the latest version of [modified `TestImageIconWithJRadioButtonMenuItem.java`](https://github.com/openjdk/jdk/blob/de48cbdf37fd812d652f6dc7113e700b0d26f99d/test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java). The diff [between yours and mine](https://github.com/openjdk/jdk/compare/4a5a7790e00650cbdd87d533429e3b47a537c3e8..de48cbdf37fd812d652f6dc7113e700b0d26f99d). - PR Comment: https://git.openjdk.org/jdk/pull/23324#issuecomment-2624548270
Re: RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v5]
On Thu, 30 Jan 2025 11:25:20 GMT, Alexey Ivanov wrote: >> I guess if we can use normal resolution image, then its best to use it than >> bitmap image which can be flaky..Probably, in earlier days, image icons used >> to come in bitmap format.. > >> Do you happen to know why State.BITMAP was originally here? > > Perhaps, because of default Windows behaviour: a bitmap menu item fully > replaces the menu item text. Yet I can't be sure… This state was added by [JDK-6432667](https://bugs.openjdk.org/browse/JDK-6432667): _Vista: Menu dropdown differs while compare with native in vista laf_. No more background is available… It may be due to how Windows renders icons in menus: if a menu item has an icon, it's rendered at the location where bullets / check-marks are rendered; if you need to display selected / unselected state of such a menu item, you have to use different icons. - PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935589364
