Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v17]
On Mon, 8 Jul 2024 09:39:11 GMT, Abhishek Kumar wrote: >> In GTK LAF, the menu mnemonics are always displayed which is different from >> the native behavior. In native application **(tested with gedit for normal >> buttons and tested with libreoffice for menu**), the menu mnemonics toggle >> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles >> between show/hide on `ALT` press. >> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the >> native behavior. Fix is similar to the `ALT` key processing in Windows LAF. >> Automated test case is added to verify the fix and tested in Ubuntu and >> Oracle linux. >> >> CI testing is green and link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > wild import expand. SynthGraphicsUtils revert back The updated fix does not introduce new public API, so the CSR is no longer needed. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2214459963
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v18]
On Mon, 8 Jul 2024 15:37:04 GMT, Abhishek Kumar wrote: >> In GTK LAF, the menu mnemonics are always displayed which is different from >> the native behavior. In native application **(tested with gedit for normal >> buttons and tested with libreoffice for menu**), the menu mnemonics toggle >> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles >> between show/hide on `ALT` press. >> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the >> native behavior. Fix is similar to the `ALT` key processing in Windows LAF. >> Automated test case is added to verify the fix and tested in Ubuntu and >> Oracle linux. >> >> CI testing is green and link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > Typo in copyright header Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2163688858
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v17]
On Mon, 8 Jul 2024 09:39:11 GMT, Abhishek Kumar wrote: >> In GTK LAF, the menu mnemonics are always displayed which is different from >> the native behavior. In native application **(tested with gedit for normal >> buttons and tested with libreoffice for menu**), the menu mnemonics toggle >> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles >> between show/hide on `ALT` press. >> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the >> native behavior. Fix is similar to the `ALT` key processing in Windows LAF. >> Automated test case is added to verify the fix and tested in Ubuntu and >> Oracle linux. >> >> CI testing is green and link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > wild import expand. SynthGraphicsUtils revert back Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 2: > 1: /* > 2: * Copyright (c) (c) 2011, 2024, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved. Too many `(c)` now. - PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2163659222 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1668838420
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v17]
On Mon, 8 Jul 2024 09:39:11 GMT, Abhishek Kumar wrote: >> In GTK LAF, the menu mnemonics are always displayed which is different from >> the native behavior. In native application **(tested with gedit for normal >> buttons and tested with libreoffice for menu**), the menu mnemonics toggle >> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles >> between show/hide on `ALT` press. >> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the >> native behavior. Fix is similar to the `ALT` key processing in Windows LAF. >> Automated test case is added to verify the fix and tested in Ubuntu and >> Oracle linux. >> >> CI testing is green and link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > wild import expand. SynthGraphicsUtils revert back > /summary Hides mnemonics on menus, buttons, and labels for GTK L > > Moved shared code for hiding mnemonics into `sun/swing/MnemonicHandler` and > `AltProcessor` to avoid code duplication. The second line is too long, is it possible to break the second line as [I initially suggested](https://github.com/openjdk/jdk/pull/18992#issuecomment-2214143609)? - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2214256600
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v15]
On Mon, 8 Jul 2024 09:17:55 GMT, Abhishek Kumar wrote: >> Abhishek Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Javadoc style comment, unused import removed > >> The diff modifies the generic `SynthGraphicsUtils.paintText` so that no >> mnemonic is passed to `SwingUtilities2.drawStringUnderlineCharAt` if >> `isMnemonicHidden` returns `true`. >> >> This approach has _a performance impact_ on all UI text painting. The >> condition can be moved into `GTKGraphicsUtils` so that only GTK L will >> call `isMnemonicHidden`. > > Updated the code to handle the mnemonic for buttons as well as labels. > Mnemonic hide condition check moved to `GTKGraphicsUtils` to avoid the > `performance issue`. > @kumarabhi006 Setting summary to: It's not what I meant. This summary will be included in the commit message, its purpose is to provide additional details of the changeset. Something like this should be enough: Hides mnemonics on menus, buttons, and labels for GTK L Moved shared code for hiding mnemonics into sun/swing/MnemonicHandler and AltProcessor to avoid code duplication. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2214143609
Re: RFR: 8333403: Write a test to check various components events are triggered properly [v4]
On Thu, 4 Jul 2024 10:22:15 GMT, Ravi Gupta wrote:
>> test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 171:
>>
>>> 169: EventQueue.invokeAndWait(() -> {
>>> 170: frame.dispose();
>>> 171: frame.setVisible(true);
>>
>> I'm not really understanding this part. Why do you dispose the frame then
>> setVisible here?
>
> Thanks to providing the review comments here I want to revalidate the screen
> resources by dispose and setVisible the frame before the frame ICONIFIED test.
>
> These lines are not required , After removing these lines test also works
> fine.
You should not call any methods of the frame after you called `dispose`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667118343
Re: RFR: 8333403: Write a test to check various components events are triggered properly [v5]
On Thu, 4 Jul 2024 10:25:38 GMT, Ravi Gupta wrote:
>> This testcase checks for the following assertions for Component events:
>>
>> 1. When components are resized, moved, hidden and shown the respective
>> events are triggered.
>> 2. When the components are hidden/disabled also,the component events like
>> resized/moved are triggered.
>> 3. When a hidden component is hidden again, or a visible component is shown
>> again, the events should not be fired.
>> 4. When a window is minimized/restored then hidden and shown component
>> events should be triggered.
>>
>> Testing:
>> Tested using Mach5(20 times per platform) in macos,linux and windows and got
>> all pass.
>
> Ravi Gupta has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8333403: Review comments fixed
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 56:
> 54:
> 55: private static Frame frame;
> 56: private static int DELAY = 500;
Suggestion:
private static final int DELAY = 500;
It's a constant, isn't it?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 58:
> 56: private static int DELAY = 500;
> 57: private static Robot robot;
> 58: private volatile static Component[] components;
Why does the `components` array need to be `volatile`?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 62:
> 60: private volatile static boolean componentShown = false;
> 61: private volatile static boolean componentMoved = false;
> 62: private volatile static boolean componentResized = false;
Assigning the default value is redundant…
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 66:
> 64: private volatile static Dimension compSize;
> 65: private volatile static ArrayList events =
> 66: new ArrayList<>();
Marking a list as `volatile` is not enough to access the contents of the list
from different threads, you have to use a synchronized collection and iterate
over `events` while holding the lock.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 141:
> 139: EventQueue.invokeAndWait(ComponentEventTest::initializeGUI);
> 140: robot.delay(DELAY);
> 141: robot.waitForIdle();
Suggestion:
robot.waitForIdle();
robot.delay(DELAY);
Delaying before `waitForIdle` doesn't make much sense… The tests use
`waitForIdle` followed by `delay` to ensure all the events are handled and then
to add additional time… just in case.
If you call `waitForIdle` after `delay`, the chances are high that
`waitForIdle` returns immediately because there are no pending events already
(they're handled when the thread has been sleeping).
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 164:
> 162: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 163:
> 164: // Hide all components and check if the ComponentEvent is
> triggered
Is it really what's going on here? Where do you hide all the components?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 181:
> 179: System.err.print("Events triggered are: ");
> 180: for (int j = 0; j < events.size();
> 181: System.err.print(events.get(j) + "; "), j++);
This is very confusing. Move printing into the body of the `for`-loop:
Suggestion:
for (int j = 0; j < events.size(); j++) {
System.err.print(events.get(j) + "; ");
}
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 184:
> 182: System.err.println();
> 183: throw new RuntimeException(
> 184: "FAIL: ComponentEvent triggered when frame is
> iconified");
You're repeating these lines of code over and over again — create a helper
method for printing the events.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 282:
> 280: throw new RuntimeException(
> 281: "FAIL: ComponentResized not triggered for "
> 282: + components[i].getClass());
Choose one style… At line 267, you pass the message on the same line; here you
wrap the message to next line immediately.
It's more common to keep the first part of the string on the same line and wrap
the string as necessary.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 291:
> 289: resetFrame();
> 290: });
> 291: robot.delay(DELAY);
So, you're repeating the same tests as above? Why can't you use a loop?
for (boolean state : new boolean[] {true, false}) {
currentComponent.setEnabled(state);
doTests();
}
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 377:
> 375: }
> 376:
> 377: private static void resetFrame() {
Suggestion:
private static void revalidateFrame() {
`revalidateFrame` is
Re: RFR: 8335789: [TESTBUG] XparColor.java test fails with Error. Parse Exception: Invalid or unrecognized bugid: @ [v2]
On Fri, 5 Jul 2024 19:02:06 GMT, lawrence.andrews wrote: >> 1) After the fix , I could see the test was launched. > > lawrence.andrews has updated the pull request incrementally with one > additional commit since the last revision: > > updated the copyright Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20057#pullrequestreview-2161169039
Re: RFR: 8335789: [TESTBUG] XparColor.java test fails with Error. Parse Exception: Invalid or unrecognized bugid: @ [v2]
On Fri, 5 Jul 2024 18:57:49 GMT, lawrence.andrews wrote: >> 1) After the fix , I could see the test was launched. > > lawrence.andrews has updated the pull request incrementally with one > additional commit since the last revision: > > updated the copyright Looks good to me. I'd still update the copyright year. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20057#pullrequestreview-2161141139
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v14]
On Fri, 5 Jul 2024 10:54:19 GMT, Alexey Ivanov wrote: > > > This changeset enables hiding / showing mnemonics on JMenuBar only. Do > > > you plan to update ButtonUI and LabelUI for GTK Look-and-Feel too? > > > > > > Not as a part of this PR. It can be taken up as other bug. > > Does it make sense to handle mnemonics in buttons and labels as a separate > bug? I doesn't seem worth it. > > You've done 98% of the job in this PR. Refactoring touched all the classes > which hide mnemonics. > > The only thing left is to add handling into `SynthButtonUI` and > `SynthLabelUI` where you need to replace `b.getDisplayedMnemonicIndex()` and > `label.getDisplayedMnemonicIndex()` with a conditional display based on > `MnemonicHandler.isMnemonicHidden()`. I still think it is better to include mnemonics for buttons and labels in this PR. It could be done easily by the following diff: diff --git a/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java b/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java index befa65d095e..514bfe400a5 100644 --- a/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java +++ b/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java @@ -361,7 +361,9 @@ public void paintText(SynthContext ss, Graphics g, String text, FontMetrics fm = SwingUtilities2.getFontMetrics(c, g); y += fm.getAscent(); SwingUtilities2.drawStringUnderlineCharAt(c, g, text, - mnemonicIndex, x, y); + MnemonicHandler.isMnemonicHidden() + ? -1 : mnemonicIndex, + x, y); } } I [committed it as 7c70b20](https://github.com/aivanov-jdk/jdk/commit/7c70b20b7928fe21d262e7b229d37a6373876cee) on top of the latest version in this PR. The diff modifies the generic `SynthGraphicsUtils.paintText` so that no mnemonic is passed to `SwingUtilities2.drawStringUnderlineCharAt` if `isMnemonicHidden` returns `true`. This approach has *a performance impact* on all UI text painting. The condition can be moved into `GTKGraphicsUtils` so that only GTK L will call `isMnemonicHidden`. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2211244902
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v15]
On Fri, 5 Jul 2024 11:12:50 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Javadoc style comment, unused import removed
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 1:
> 1: /*
Git identifies this class as rename from `AquaMnemonicHandler` to
`MnemonicHandler`.
Indeed, the files are very similar. Shall we preserve the original copyright
year: `(c) 2011, 2024`?
test/jdk/javax/swing/JMenuBar/TestMenuMnemonicLinuxAndMac.java line 59:
> 57: UIManager.getInstalledLookAndFeels()) {
> 58: if (laf.getName().contains("GTK") ||
> laf.getName().contains("Aqua")) {
> 59: System.out.println("Testing: "+laf.getName());
Suggestion:
System.out.println("Testing: " + laf.getName());
Spaces around the binary operator `+`.
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2160915917
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1666923069
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1666925212
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v14]
On Thu, 4 Jul 2024 18:44:52 GMT, Abhishek Kumar wrote: >> In GTK LAF, the menu mnemonics are always displayed which is different from >> the native behavior. In native application **(tested with gedit for normal >> buttons and tested with libreoffice for menu**), the menu mnemonics toggle >> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles >> between show/hide on `ALT` press. >> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the >> native behavior. Fix is similar to the `ALT` key processing in Windows LAF. >> Automated test case is added to verify the fix and tested in Ubuntu and >> Oracle linux. >> >> CI testing is green and link attached in JBS. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > wild imports expand, imports sorting > Imports are expanded and sorted as per recent review comments. I guess > everything is fine now. Nearly there. > > This changeset enables hiding / showing mnemonics on JMenuBar only. Do you > > plan to update ButtonUI and LabelUI for GTK Look-and-Feel too? > > Not as a part of this PR. It can be taken up as other bug. Does it make sense to handle mnemonics in buttons and labels as a separate bug? I doesn't seem worth it. You've done 98% of the job in this PR. Refactoring touched all the classes which hide mnemonics. The only thing left is to add handling into `SynthButtonUI` and `SynthLabelUI` where you need to replace `b.getDisplayedMnemonicIndex()` and `label.getDisplayedMnemonicIndex()` with a conditional display based on `MnemonicHandler.isMnemonicHidden()`. src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 44: > 42: import javax.swing.JMenuBar; > 43: import javax.swing.JMenuItem; > 44: import javax.swing.JMenu; Suggestion: import javax.swing.JMenu; import javax.swing.JMenuBar; import javax.swing.JMenuItem; src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java line 1: > 1: /* There are still unused imports in the list: `File`, `PrivilegedAction`, `Locale` and `OSInfo`. src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java line 46: > 44: import sun.swing.MenuItemLayoutHelper; > 45: import sun.swing.SwingUtilities2; > 46: import sun.swing.MnemonicHandler; Suggestion: import sun.swing.MenuItemLayoutHelper; import sun.swing.MnemonicHandler; import sun.swing.SwingUtilities2; src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 75: > 73: /* > 74: * Repaints all the components with the mnemonics in the given window > and all its owned windows. > 75: */ I suggest converting the comment to javadoc comment: Suggestion: /** * Repaints all the components with the mnemonics in the given window and all its owned windows. */ src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 92: > 90: * Repaints all the components with the mnemonics in container. > 91: * Recursively searches for all the subcomponents. > 92: */ Convert this one to javadoc comment too. test/jdk/javax/swing/JMenuBar/TestMenuMnemonicLinuxAndMac.java line 38: > 36: import javax.swing.SwingUtilities; > 37: import javax.swing.UIManager; > 38: import javax.swing.plaf.synth.SynthLookAndFeel; `Point` and `SynthLookAndFeel` aren't used in the test. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2160433537 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r134817 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r137877 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r140228 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r141169 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r142504 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r150816
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v11]
On Wed, 3 Jul 2024 04:16:53 GMT, Abhishek Kumar wrote: > Updated the wild imports. My IDE doesn't indicate if imports are sorted or > not or if any imports are unused. Does it require to do some settings in IDE? Perhaps, it depends on how you configured your IDE and the project for JDK. I just use **Code** > **Optimize Imports** command (or rather Ctrl+Alt+O shortcut), and I expect to see no changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665931601
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v13]
On Wed, 3 Jul 2024 11:17:56 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Test update to refer MnemonicHandler class
This changeset enables hiding / showing mnemonics on `JMenuBar` only. Do you
plan to update `ButtonUI` and `LabelUI` for GTK Look-and-Feel too?
src/java.desktop/macosx/classes/com/apple/laf/AquaLabelUI.java line 37:
> 35: import javax.swing.plaf.UIResource;
> 36:
> 37: import javax.swing.plaf.basic.BasicLabelUI;
I would prefer not to add blank lines between these imports.
Having a blank line between `java.*` and `javax.*` above is fine.
src/java.desktop/macosx/classes/com/apple/laf/AquaLabelUI.java line 43:
> 41:
> 42: import com.apple.laf.AquaUtils.RecyclableSingleton;
> 43: import com.apple.laf.AquaUtils.RecyclableSingletonFromDefaultConstructor;
I'd sort them alphabetically too: `sun.swing` follows `com.apple.laf`. I'd not
add a blank line between these imports.
src/java.desktop/macosx/classes/com/apple/laf/AquaLookAndFeel.java line 58:
> 56: import apple.laf.JRSUIControl;
> 57: import apple.laf.JRSUIUtils;
> 58:
Remove the blank line?
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 37:
> 35: import java.awt.Rectangle;
> 36:
> 37: import java.awt.event.InputEvent;
No blank line here.
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 52:
> 50: import javax.swing.UIManager;
> 51:
> 52: import javax.swing.border.Border;
No blank line here.
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 62:
> 60: import apple.laf.JRSUIConstants.Widget;
> 61:
> 62: import com.apple.laf.AquaIcon.InvertableIcon;
Maybe preserve the blank lines in this section, yet it could be better to
remove these too.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 46:
> 44: import java.util.Map;
> 45: import sun.awt.SunToolkit;
> 46: import sun.awt.UNIXToolkit;
Optimize imports in this file too, to expand the wildcard imports.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsGraphicsUtils.java
line 43:
> 41: import javax.swing.JMenu;
> 42: import javax.swing.JMenuItem;
> 43: import javax.swing.UIManager;
Suggestion:
import javax.swing.AbstractButton;
import javax.swing.ButtonModel;
import javax.swing.JButton;
import javax.swing.JCheckBox;
import javax.swing.JMenu;
import javax.swing.JMenuItem;
import javax.swing.JRadioButton;
import javax.swing.JToggleButton;
import javax.swing.UIManager;
Keep the imports sorted.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsGraphicsUtils.java
line 49:
> 47: import sun.swing.SwingUtilities2;
> 48:
> 49: import com.sun.java.swing.plaf.windows.WindowsButtonUI;
The `WindowsButtonUI` import is unused.
test/jdk/javax/swing/JMenuBar/TestMenuMnemonicLinuxAndMac.java line 63:
> 61: System.out.println("Testing: "+laf.getName());
> 62: UIManager.setLookAndFeel(laf.getClassName());
> 63: }
Suggestion:
if (laf.getName().contains("GTK") ||
laf.getName().contains("Aqua")) {
System.out.println("Testing: " + laf.getName());
UIManager.setLookAndFeel(laf.getClassName());
break;
}
Since we expect one and only one L, break from the loop as soon as you found
a match.
-
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2159270954
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665920495
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665921960
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665923019
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665924234
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665924574
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665925751
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665926565
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665928024
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665927518
PR Review Comment:
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Thu, 4 Jul 2024 13:15:28 GMT, Thomas Stuefe wrote: > Using memmove is so uncommon that it is usually a clear indication for a > deliberate choice. It may still be an accidental choice. I didn't find any code review for [JDK-6680988](https://bugs.openjdk.org/browse/JDK-6680988) where this line had been added. - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2209220729
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Wed, 3 Jul 2024 13:34:38 GMT, Julian Waters wrote:
>> In [JDK-8302671](https://bugs.openjdk.org/browse/JDK-8302671) I fixed a
>> memmove decay bug by rewriting a sizeof on an array to an explicit size of
>> 256, but this is a bit of a band aid fix. It's come to my attention that in
>> C++, one can pass an array by reference, which causes sizeof to work
>> correctly on an array and has the added bonus of enforcing an array of that
>> size on the arguments passed to that method. I've reverted my change from
>> 8302671 and instead explicitly made kstate an array reference so that sizeof
>> works on the array as expected, and that the array size can be explicitly
>> set in the array brackets
>>
>> Verification: https://godbolt.org/z/Ezj76eWWY and GitHub Actions
>
> Julian Waters has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Formatting sizeof awt_Component.cpp
> - Formatting awt_Component.cpp
> - Merge branch 'openjdk:master' into patch-10
> - Update src/java.desktop/windows/native/libawt/windows/awt_Component.cpp
>
>Co-authored-by: Alexey Ivanov
> - 8334599
Still looks good, except for the minor formatting comments.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3365:
> 3363: }
> 3364: static void
> 3365: resetKbdState(BYTE () [AwtToolkit::KB_STATE_SIZE]) {
I don't know what is most used syntax for this type. I'd rather keep them
together without a space between `()` and `[]`.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368:
> 3366: BYTE tmpState[AwtToolkit::KB_STATE_SIZE];
> 3367: WCHAR wc[2];
> 3368: memmove(tmpState, kstate, sizeof (kstate));
Suggestion:
memmove(tmpState, kstate, sizeof(kstate));
There's usually no space after `sizeof`.
-
Marked as reviewed by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2158844075
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1665680146
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1665657251
Re: RFR: 8334599: Improve code from JDK-8302671 [v2]
On Wed, 3 Jul 2024 13:25:27 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368: >> >>> 3366: BYTE tmpState[256]; >>> 3367: WCHAR wc[2]; >>> 3368: memmove(tmpState, kstate, sizeof(kstate)); >> >> Using `memcpy` could be more performant, we know for sure that `tmpState` >> and `kstate` do not overlap. > > I can't quite comment on that since I don't really know what the purpose of > the memmove is. What does @prrace think? Both `memcpy` and `memmove` do the same thing, namely each function copies bytes from one location into another location. The difference between these two functions is in whether buffers are allowed to overlap or not: * [`memcpy`](https://en.cppreference.com/w/c/string/byte/memcpy): https://en.cppreference.com/w/c/string/byte/memcpy;>If the objects overlap […], the behavior is undefined.https://en.cppreference.com/w/c/string/byte/memcpy#Notes;>`memcpy` is the fastest library routine for memory-to-memory copy. It is usually more efficient than `strcpy`, which must scan the data it copies or `memmove`, which must take precautions to handle overlapping inputs. * [`memmove`](https://en.cppreference.com/w/c/string/byte/memmove): https://en.cppreference.com/w/c/string/byte/memmove;>The objects may overlap: copying takes place as if the characters were copied to a temporary character array and then the characters were copied from the array to `dest`. > we know for sure that `tmpState` and `kstate` do not overlap. For this reason, `memcpy` is safe to use, and it is more efficient than `memmove`. To be clear, I'm not proposing to incorporate this change under this bug. I just pointed out a possible optimisation. - PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1665654705
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v11]
On Fri, 28 Jun 2024 19:54:37 GMT, Alexey Ivanov wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove AquaMnemonicHandler class and unused APIs from WinDowsLookAndFeel,
>> copyright year update
>
> test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 63:
>
>> 61:
>> UIManager.setLookAndFeel("com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
>> 62: } else if (laf.getName().contains("Aqua")) {
>> 63:
>> UIManager.setLookAndFeel("com.apple.laf.AquaLookAndFeel");
>
> Suggestion:
>
> if (laf.getName().contains("GTK")) {
> UIManager.setLookAndFeel(laf.getClassName());
> } else if (laf.getName().contains("Aqua")) {
> UIManager.setLookAndFeel(laf.getClassName());
>
> Now that the body of both `if` statements is the same, you can combine the
> conditions into one `if` statement.
Alternatively, you can choose the look-and-feel before starting the test:
there's always only one available.
If you found a L, install it and run the test outside the loop (over
`LookAndFeelInfo` array).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659262410
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v11]
On Fri, 28 Jun 2024 11:32:49 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove AquaMnemonicHandler class and unused APIs from WinDowsLookAndFeel,
> copyright year update
Overall, the changes look good to me.
There are several minor comments though.
Now we have one common mnemonic handler instead of two… short of having three
of those.
src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java line 79:
> 77: import com.apple.laf.AquaUtils.RecyclableSingletonFromDefaultConstructor;
> 78: import sun.swing.SwingUtilities2;
> 79: import sun.swing.MnemonicHandler;
Suggestion:
import sun.swing.MnemonicHandler;
import sun.swing.SwingUtilities2;
Allow you IDE handle imports for you, we tend to keep the imports sorted
alphabetically.
src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java line 493:
> 491: final Graphics2D g2d = g instanceof Graphics2D ? (Graphics2D)g :
> null;
> 492:
> 493: final AbstractButton b = (AbstractButton)c;
Perhaps, it makes sense to remove `g2d` local variable — it's unused.
src/java.desktop/macosx/classes/com/apple/laf/AquaLabelUI.java line 32:
> 30: import javax.swing.*;
> 31: import javax.swing.plaf.*;
> 32: import javax.swing.plaf.basic.*;
Could you expand all the wildcard imports?
src/java.desktop/macosx/classes/com/apple/laf/AquaLookAndFeel.java line 61:
> 59: import sun.swing.SwingUtilities2;
> 60: import sun.swing.MnemonicHandler;
> 61: import sun.swing.AltProcessor;
Suggestion:
import sun.swing.AltProcessor;
import sun.swing.MnemonicHandler;
import sun.swing.SwingAccessor;
import sun.swing.SwingUtilities2;
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 34:
> 32: import javax.swing.border.Border;
> 33: import javax.swing.plaf.basic.BasicHTML;
> 34: import javax.swing.text.View;
Could you expand all the wildcard imports?
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 53:
> 51: import sun.swing.SwingUtilities2;
> 52: import sun.swing.MnemonicHandler;
> 53: import sun.swing.AltProcessor;
Could you expand all the wildcard imports?
Allow your IDE sort the imports; `AltProcessor` should go before
`DefaultLayoutStyle` and `MnemonicHandler` should go before `SwingAccessor`.
src/java.desktop/share/classes/sun/swing/AltProcessor.java line 35:
> 33: import javax.swing.SwingUtilities;
> 34:
> 35: import sun.swing.MnemonicHandler;
My IDE highlights `MnemonicHandler` import as redundant because it comes from
the same package…
src/java.desktop/share/classes/sun/swing/AltProcessor.java line 47:
> 45: }
> 46:
> 47: public boolean postProcessKeyEvent(final KeyEvent ev) {
I'd add `@Override` annotation to highlight it implements the
`KeyEventPostProcessor` interface.
src/java.desktop/share/classes/sun/swing/AltProcessor.java line 62:
> 60: MnemonicHandler.setMnemonicHidden(true);
> 61: break;
> 62: }
Just an idea…
Suggestion:
// Show mnemonics when Alt is pressed and hide them when released
MnemonicHandler.setMnemonicHidden(ev.getID() == KeyEvent.KEY_RELEASED);
The `switch` statement is more explicit, so easier to understand.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsGraphicsUtils.java
line 31:
> 29: import sun.swing.MnemonicHandler;
> 30:
> 31: import java.awt.*;
Could you expand all the wildcard imports?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLabelUI.java
line 39:
> 37: import sun.awt.AppContext;
> 38: import sun.swing.SwingUtilities2;
> 39: import sun.swing.MnemonicHandler;
Suggestion:
import sun.swing.MnemonicHandler;
import sun.swing.SwingUtilities2;
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLabelUI.java
line 82:
> 80: mnemonicIndex = -1;
> 81: }
> 82: if ( UIManager.getColor("Label.disabledForeground") instanceof
> Color &&
Perhaps, we could resolve the warning: `UIManager.getColor` returns `Color`
object, so what `instanceof Color` really verifies here is whether the returned
object is `null` or not.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java
line 101:
>
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
On Thu, 27 Jun 2024 10:06:49 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Mnemonic handler class
> - Mnemonic handler added and previous implementation revert back
I think we're moving in the right direction.
My idea was to extract the common functionality as the methods
`setMnemonicHidden`, `isMnemonicHidden` and `repaintMnemonicsInWindow`,
`repaintMnemonicsInContainer` into a helper class.
`AltProcessor`… Your version for GTK and the version in Aqua look the same to
me. I think it makes sense to create a common `AltProcessor` class which would
be used by GTK and Aqua.
The Windows version is different, so it'll stay this way… It doesn't look worth
trying to fit that version into a shared class.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1461:
> 1459:
> 1460: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 1461: addKeyEventPostProcessor(MnemonicHandler.altProcessor);
Suggestion:
KeyboardFocusManager.getCurrentKeyboardFocusManager()
.addKeyEventPostProcessor(MnemonicHandler.altProcessor);
Better wrap the line before the dot operator — this conveys that it's a
continuation of a call sequence rather than a wrapped parameter to a method.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1469:
> 1467: /**
> 1468: * Called by UIManager when this look and feel is uninstalled.
> 1469: */
Does it repeat the javadoc of the super class? Likely it does, use
`{@inheritDoc}` instead.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1470:
> 1468: * Called by UIManager when this look and feel is uninstalled.
> 1469: */
> 1470: @Override
You may want to add the `@Override` annotation to `initialize` method too… for
consistency.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 672:
> 670: int mnemIndex =
> lh.getMenuItem().getDisplayedMnemonicIndex();
> 671: // Check to see if the Mnemonic should be rendered in
> GTK.
> 672: if (mnemIndex >= 0 &&
> MnemonicHandler.isMnemonicHidden()) {
Is 0 a valid mnemonic? Probably not.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 40:
> 38: import javax.swing.UIManager;
> 39:
> 40: public class MnemonicHandler {
Suggestion:
public final class MnemonicHandler {
It's an utility class that should not be extended. In addition to that, it
should have a private constructor to prevent instantiating the class.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 41:
> 39:
> 40: public class MnemonicHandler {
> 41: public static final AltProcessor altProcessor = new AltProcessor();
I'd like to see `AltProcessor` here but it's different for Windows. Thus,
`AltProcessor` should be local to `*RootUI` as it is now.
If `AltProcessor` in GTK and Aqua are similar or the same, it makes sense to
extract them into a common helper class.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 43:
> 41: public static final AltProcessor altProcessor = new AltProcessor();
> 42:
> 43: protected static boolean isMnemonicHidden;
Suggestion:
private static boolean isMnemonicHidden;
It's accessed via `isMnemonicHidden` and `setMnemonicHidden`; since the class
cannot be extended, it should not have protected members.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 103:
> 101: * Repaints all the components with the mnemonics in the given
> window and all its owned windows.
> 102: */
> 103: static void repaintMnemonicsInWindow(final Window w) {
Suggestion:
public static void repaintMnemonicsInWindow(final Window w) {
This method would be called from `AltProcessor` class in the three different
L, it should be `public`.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 120:
> 118: * Recursively searches for all the subcomponents.
> 119: */
> 120: static void repaintMnemonicsInContainer(final Container cont) {
If `repaintMnemonicsInContainer` is never called directly but only from
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 13:15:19 GMT, Julian Waters wrote: > It would be helpful if JNI had a jobject unique_ptr type for C++ But `std::unique_ptr` wasn't available when JNI had been conceived… It could be added… The declaration in your sample still looks cumbersome… or _unwieldy_ as you said. Writing a custom deleter for each `jobject` doesn't look good either. Most local references don't require explicit `DeleteLocalRef`, which also helps, I guess. > Oops, you sent your correction about the custom deleter just as I sent my post C++ has got many new features… I'm learning these feature on the fly since I don't use C++ as often. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194699149
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 12:54:55 GMT, Alexey Ivanov wrote: > > …I believe I was referring to the use of C++'s std::unique_ptr, which has > > the functionality for cleanup that we need. > > Yes, `std::unique_ptr` could be useful for handling automatic deallocation of > objects created with the `new` operator. > > ~~It won't help with releasing references to Java objects though.~~ Since `std::unique_ptr` accepts a custom deleter, it may be used for deleting any object. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194662542
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 11:26:24 GMT, Julian Waters wrote: > …I believe I was referring to the use of C++'s std::unique_ptr, which has the > functionality for cleanup that we need. Yes, `std::unique_ptr` could be useful for handling automatic deallocation of objects created with the `new` operator. It won't help with releasing references to Java objects though. > That is currently blocked by awt.dll not being allowed to use the C++ > Standard Library, but if one looks at the current awt.dll with dumpbin, > awt.dll is actually already is linked to msvcp.dll, meaning it already uses > C++ Standard Library somehow (I don't know what exactly makes it dependent on > msvcp.dll) I remember STL has been banned from AWT code, yet I don't know the reasons for it. It could be to keep the size smaller. I can see that `awt.dll` depends on `msvcp140.dll` and imports one function from it: `?_Xlength_error@std@@YAXPEBD@Z`. It doesn't look like the C++ Standard Library is used. I can't find where it comes from though… It could be worth getting rid of this dependency. I built a small sample app which uses `std::unique_ptr` and `std::make_unique` and it does not depend on `msvcp140.dll` at all. This makes sense, template libraries are header-only and will become part of the executable (or the dynamic library). The only references to `std` namespace that I found in client are in `ShellFolder.cpp` which uses `std::bad_alloc`, it uses this type in `catch` blocks and it may throw an object of this class. So, it looks `awt.dll` doesn't use the C++ Standard Library. However, using C++ objects with [RAII](https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization) makes the code shorter and frees the programmer from repeating clean-up blocks¹. Indeed, it's already used by C++ classes in AWT: [`JNILocalFrame`](https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L64-L77) and [`CriticalSection`](https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L85-L119) along with its `Lock` class inside. ¹ Repeated clean-up blocks can be seen in #18584 in nearly all the files. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194603793
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Wed, 26 Jun 2024 20:30:55 GMT, Alisen Chung wrote: > > Yes, there is some disconnect. For all OS (MAC with above changes) print > > range is working properly only with following values firstPage =0 and > > lastPage =-1 at **RasterPrinterJob.java** and highly depend on > > **pageRangesAttr** . I don't think there is any issue with native code, > > with this change I have brought MAC same as other OS. > > Shouldn't the solution then be to fix the issue with RasterPrinterJob.java > not working properly with correct firstPage and lastPage values? The first question to answer here is whether the page range is set and preserved in the print job attributes. I haven't looked into it, but I presume this is the case. The page range could be handled on the Java level, it may not be passed to the native level at all. What I mean is that JDK uses the page range to print the specified pages. The implementation on macOS could be adjusted in a similar way, as I said in [my previous comment](https://github.com/openjdk/jdk/pull/19740#issuecomment-2183359828). It should be possible to print a range of pages (which can consist of several intervals) without displaying the Print dialog, thus the `PageRanges` should be handled. As far as I understand, the `SunPageSelection.RANGE` attribute is somewhat informational, and specifies that `PageRanges` attribute is set. > It also looks like the indices are different between macOS and java. Perhaps > there's code somewhere that should have but hasn't converted the indices over? It's true. Different classes use different indices. The intervals in `PageRanges` count pages from 1 whereas `PrinterJob` uses 0-based indexing. According to Apple documentation, [`knowsPageRange`](https://developer.apple.com/documentation/appkit/nsview/1483774-knowspagerange?language=objc) returns `YES` if the range is known, in which case the [NSRange](https://developer.apple.com/documentation/foundation/nsrange?language=objc#4292523) structure contains the range of pages, and the pages start from 1. - PR Comment: https://git.openjdk.org/jdk/pull/19740#issuecomment-2194389772
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> I'd rather keep `JNI_FALSE` here — it's more explicit and therefore clearer. >> You don't have to keep track of the value of `doIt` flag while reading the >> code. >> >> In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code >> below `if (ret)` slightly harder. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 537: > 535: env->DeleteLocalRef(page); > 536: env->DeleteLocalRef(self); > 537: }; In the long term, it is better to create a special class that deletes global and local references. It could be these constructs to which @TheShermanTanker referred in [this comment](https://github.com/openjdk/jdk/pull/18584#pullrequestreview-1975384014): https://github.com/openjdk/jdk/pull/18584#pullrequestreview-1975384014;>…I have a change in mind relying on _RAII_ that would look much cleaner and neater. - PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1656909846
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Wed, 26 Jun 2024 23:42:16 GMT, Alisen Chung wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 640:
>>
>>> 638: HGLOBAL oldG = AwtPrintControl::getPrintHDMode(env, self);
>>> 639: if (setup.hDevMode != oldG) {
>>> 640:AwtPrintControl::setPrintHDMode(env, self, setup.hDevMode);
>>
>> what does setPrintHDMode and setPrintHDName do? do they need to be called as
>> part of cleanup even if there is an exception and we return JNI_FALSE?
>
> i think this code preserves the printer settings after the dialog closes (and
> only on success before this fix), so is it possible in the case the printer
> is removed and causes an exception, we shouldn't preserve these settings?
It's a good question…
Yes, the code saves (or updates) the printer name and mode.
These lines of code were called in cases where OK or Cancel is clicked in the
print dialog. However, it was skipped in case of an error, just like
`CheckUninstallModalHook`.
I also feel that these lines belong in the successful case, right before
`return JNI_TRUE`, but I could quickly find any confirmation whether
`setup.hDevMode` or `setup.hDevNames` can be modified. For this reason, I
preserved the previous code flow.
I presume that neither `setup.hDevMode` or `setup.hDevNames` changes if the
user clicks Cancel in `::PageSetupDlg`. It still leaves the question open what
happens when the user clicks OK but we catch an error inside `if (ret)`. The
old handles stored inside `self` could have become invalid, so it's safer to
update the handles in `self` even if an error occurs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1656892141
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v7]
On Thu, 27 Jun 2024 07:40:43 GMT, Prasanta Sadhukhan wrote: >> Issue is seen in that if we call setEnabled(false) over JSplitPane than it >> can't be dragged via its divider, But if SplitPane have one touch expandable >> true than user can click those buttons and change the divider position. >> So, if splitpane is disabled, then both dragging in divider and >> one-touch-expandable click should be disabled. >> Fix is made to override setEnabled and disable one-touch-expandable buttons >> actions.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Remove headful Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2144903031
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v6]
On Wed, 26 Jun 2024 10:40:47 GMT, Prasanta Sadhukhan
wrote:
>> Issue is seen in that if we call setEnabled(false) over JSplitPane than it
>> can't be dragged via its divider, But if SplitPane have one touch expandable
>> true than user can click those buttons and change the divider position.
>> So, if splitpane is disabled, then both dragging in divider and
>> one-touch-expandable click should be disabled.
>> Fix is made to override setEnabled and disable one-touch-expandable buttons
>> actions..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix
Looks good now, except for a couple of minor comments.
test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 27:
> 25: * @test
> 26: * @bug 5021949
> 27: * @key headful
Please verify if the updated test still requires headful environment.
test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 46:
> 44: private static JButton rightOneTouchButton;
> 45: private static JSplitPane jsp;
> 46: private static volatile boolean btnEnabled;
Both `jsp` and `btnEnabled` are unused now.
test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 65:
> 63: }
> 64: System.out.println("Testing LAF : " + laf.getClassName());
> 65: SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf));
The `setLookAndFeel(laf)` can be moved into the main `invokeAndWait` block.
-
Marked as reviewed by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2142119084
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1655024660
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1655022678
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1655020372
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v4]
On Wed, 26 Jun 2024 09:52:38 GMT, Nizar Benalla wrote: >> If you're currently reviewing this PR, thank you! >> Most fixes here are according to the reports by the since checker tool in >> #18934 and are pretty simple. >> >> To make reviewing easier >> - `BasicSliderUI` has the constructor `public BasicSliderUI(JSlider b)` for >> a long time so the default constructor (without parameters) didn't exist >> until JDK 16 >> >> For the `package-info` files, it is pretty hard to find source code of JDK >> 1-5 so I used the `grep` command to find the oldest instance of an `@since` >> in those packages. >> >> I found instances of `@since 1.1` in the other packages but >> `javax/swing/plaf/synth/package-info.java` might be worth checking as most >> classes there had no `@since`. > > Nizar Benalla has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Merge remote-tracking branch 'upstream/master' into JDK-8332103 > ># Conflicts: ># src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java > - See if an empty commit removes sponsor label > - Swing was added in JDK 1.2 > - Add `@since` tags to `java.desktop` Looks good to me. Thank you for waiting and then resolving the conflict. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19192#pullrequestreview-2141374861
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Wed, 26 Jun 2024 08:36:45 GMT, Prasanta Sadhukhan
wrote:
>> Alisen's concern is valid.
>>
>> What if `setEnabled(false)` is called when `isOneTouchExpandable` is `false`
>> and then `setOneTouchExpandable(true)` is called which adds the buttons? The
>> buttons are enabled when they should be disabled.
>
> Why buttons will be enabled? The buttons state is determined by `setEnabled
> `param, if `setEnabled `is false, then irrespective of
> `isOneTouchExpandable`, it will be disabled...`isOneTouchExpandable `is just
> extra layer of check as that determines whether arrow buttons are rendered or
> notIf oneTouchExpandable is not there then arrow buttons itself is not
> created so no point set their state...
Consider the following scenario:
JSplitPane jsp = new JSplitPane(JSplitPane.HORIZONTAL_SPLIT,
new JButton("Left"),
new JButton("Right"));
jsp.setOneTouchExpandable(false);
jsp.setEnabled(false);
// At this point neither `leftButton` or `rightButton` are created
jsp.setOneTouchExpandable(true);
// This has created both `leftButton` or `rightButton`,
// and both buttons are enabled, aren't they?
// I can't see how either button could get disabled.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654434650
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Wed, 26 Jun 2024 08:43:40 GMT, Prasanta Sadhukhan wrote: >> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java >> line 376: >> >>> 374: leftButton.setEnabled(enabled); >>> 375: } >>> 376: } >> >> Is it possible to override `isEnabled` in `rightButton` and `leftButton` so >> that it returns the state of `JSplitPane`? >> >> Alternatively, the buttons could install a `PropertyChangeListener` for >> `"enabled"` property and align their state to the host split pane. > > Right now, the buttons are always enabled and rendered when > isOneTouchExpandable is enabled without any scope to disable...We need this > code anyway to set/reset buttons state..I guess on top of this code, we can > add isEnabled if it is needed but for this issue, isEnabled is not needed.. This approach will automatically resolve [the scenario above](https://github.com/openjdk/jdk/pull/19695/files#r1646653721) where buttons may have inconsistent state. - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654437914
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v5]
On Wed, 26 Jun 2024 09:00:28 GMT, Prasanta Sadhukhan
wrote:
>> Issue is seen in that if we call setEnabled(false) over JSplitPane than it
>> can't be dragged via its divider, But if SplitPane have one touch expandable
>> true than user can click those buttons and change the divider position.
>> So, if splitpane is disabled, then both dragging in divider and
>> one-touch-expandable click should be disabled.
>> Fix is made to override setEnabled and disable one-touch-expandable buttons
>> actions..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Test update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/JSplitPane.java line 1:
> 1: /*
Copyright year to be updated in `JSplitPane`.
test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 82:
> 80: jsp.setOneTouchExpandable(true);
> 81:
> 82: jsp.setEnabled(false);
> It should be simpler, it could be done without rendering the frame.
I'm pretty sure it could be done *without creating a frame* and without showing
the frame:
Suggestion:
jsp.setUI(new TestSplitPaneUI());
jsp.setOneTouchExpandable(true);
jsp.setEnabled(false);
if (leftOneTouchButton.isEnabled()) {
throw new RuntimeException("leftButton is enabled for
disabled JSplitPane");
}
if (rightOneTouchButton.isEnabled()) {
throw new RuntimeException("rightButton is enabled for
disabled JSplitPane");
}
Calling `jsp.setEnabled(false);` changes the state of the buttons immediately,
doesn't it?
-
PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2141208110
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654454389
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654450514
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Wed, 26 Jun 2024 08:40:24 GMT, Abhishek Kumar wrote: >> Thank you for looking into it. A `MnemonicHandler` class in `sun.swing` or >> `sun.swing.plaf` package could be a good candidate. The `sun.swing` package >> contains a lot of support classes for Swing, including `SwingUtilities2` and >> `UIAction`; the `sun.swing.plaf` may be better as the mnemonic handler is >> part of PLAF. >> >> Another option is the `com.sun.java.swing` package which currently contains >> `SwingUtilities3` and `plaf` subpackage with `gtk` and `motif` related >> classes. > > I tried moving the `repaintMnemonicsInWindow` and > `repaintMnemonicsInContainer` under `SwingUtilities` class but faced build > issue while accessing them in `WindowsPopupMenuUI` file. > > I will try with the suggested packages as you mentioned. The `javax.swing.SwingUtilities` is a public class, which means adding public methods to it requires a CSR; package-private methods aren't visible from other packages. The `sun.swing` and `com.sun.java.swing` packages aren't public, you can add public classes with public methods easily; since the classes are public and the methods are public, they're accessible from any other package in Swing. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1654417685
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 07:55:24 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove access modifier from method declaration
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 868:
> 866: "ctrl released ENTER", "release"
> 867: },
> 868: "RootPane.altPress", true,
`RootPane.hideMnemonics` or `RootPane.showMnemonicsOnAltPress` could be a more
descriptive property name.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 676:
> 674: int mnemIndex =
> lh.getMenuItem().getDisplayedMnemonicIndex();
> 675: // Check to see if the Mnemonic should be rendered in
> GTK.
> 676: if (UIManager.getBoolean("RootPane.altPress")
You may add another condition to the `if` statement, `mnemIndex < 0` — if
there's no mnemonic defined, there's no need to query the properties from
`UIManager` at all.
Is the new property even needed? Is `Button.showMnemonics` not enough? This
property controls mnemonics on all the components, including menu bar, in
Windows and Aqua. Can't Synth and GTK follow the same pattern?
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 679:
> 677: && SynthLookAndFeel.isMnemonicHidden()) {
> 678: mnemIndex = -1;
> 679: }
Perhaps, verifying the value of the `RootPane.altPress` property should also be
moved into `SynthLookAndFeel.isMnemonicHidden()` which already queries the
value of `Button.showMnemonics`. Keeping all the properties that affect hiding
or showing the mnemonics in one place is easier to reason about if anything
doesn't work as expected.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1046:
> 1044:
> 1045: // Toggle flag for drawing the mnemonic state
> 1046: private static boolean isMnemonicHidden = true;
I wonder why it defaults to `true` where only one L derived from Synth sets
it to true.
test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 72:
> 70: pt = fileMenu.getLocationOnScreen();
> 71: fileMenuWidth = fileMenu.getWidth();
> 72: fileMenuHeight = fileMenu.getHeight();
If you save width and height in a `Dimension` object, `fileMenuSize`, you'll be
able to use `new Rectangle(pt, fileMenuSize)` for capturing the screen.
Since you need that rectangle only, you can create the rectangle here:
Suggestion:
fileMenuRect = new Rectangle(fileMenu.getLocationOnScreen(),
fileMenu.getSize());
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2139047741
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653058424
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653054277
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653051513
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653063685
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653089074
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:18:29 GMT, Alexey Ivanov wrote: >>> In my initial fix, I added the `altProcessor` handler in >>> `SynthLookAndFeel.initialize` with condition check for GTK L Phil has >>> suggested not to check for GTK L instead look for some alternate way like >>> mentioned >>> [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). >>> >>> So, the handler implementation is moved to `SynthRootPaneUI`. >> >> Thus, out of Synth-derived Look and Feels, only GTK needs the feature. Can't >> the listener be installed in `GTKLookAndFeel.initialize`? >> >> At the same time, if you install the listener in `SynthLookAndFeel`, other >> L based on Synth could also use this feature, thus your way is more >> flexible. >> >> However, `SynthRootPaneUI` is still part of the base class… I still don't >> understand how it's different from what I'm proposing. >> >> I'd like to avoid keeping a flag whether the listener is installed or not… >> But there could be no other way since not all Synth-based L enable hiding >> mnemonics. > >> > Requesting the value of RootPane.altPress from UIManager each time >> > postProcessKeyEvent is called is inefficient, so you can store the value >> > in altProcessor when look and feel is installed. >> >> I guess you are pointing out the code in SynthGraphicsUtils.paintText method >> where RootPane.altPress value is retrieved from UIManager each time. Storing >> the value in it's constructor doesn't reflect the correct value and is >> always `false`. > > No, I didn't refer to this particular case. I referred to my suggestion where > you always add `altProcessor` as the keyboard listener and keep track of the > value of the `RootPane.altPress` property. > > Anyway this approach seems to be rejected. > > If RootPane.altPress can be changed dynamically, you can install a > > PropertyChangeListener to UIManager. > > I think it can be changed but is it really required to handle it ? I mean why > does a user change it dynamically ? Not impossible. On Windows, there used to be an option to hide or display mnemonics and focus indicators, and this option could be changed while the app is running. I think there's no option in GNOME, so we shouldn't go out of the way to support dynamic change of the `RootPane.altPress` property. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653037803
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 14:58:53 GMT, Alexey Ivanov wrote: >>> If RootPane.altPress can be changed dynamically, you can install a >>> PropertyChangeListener to UIManager. >> >> I think it can be changed but is it really required to handle it ? >> I mean why does a user change it dynamically ? > >> In my initial fix, I added the `altProcessor` handler in >> `SynthLookAndFeel.initialize` with condition check for GTK L Phil has >> suggested not to check for GTK L instead look for some alternate way like >> mentioned >> [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). >> >> So, the handler implementation is moved to `SynthRootPaneUI`. > > Thus, out of Synth-derived Look and Feels, only GTK needs the feature. Can't > the listener be installed in `GTKLookAndFeel.initialize`? > > At the same time, if you install the listener in `SynthLookAndFeel`, other > L based on Synth could also use this feature, thus your way is more > flexible. > > However, `SynthRootPaneUI` is still part of the base class… I still don't > understand how it's different from what I'm proposing. > > I'd like to avoid keeping a flag whether the listener is installed or not… > But there could be no other way since not all Synth-based L enable hiding > mnemonics. > > Requesting the value of RootPane.altPress from UIManager each time > > postProcessKeyEvent is called is inefficient, so you can store the value in > > altProcessor when look and feel is installed. > > I guess you are pointing out the code in SynthGraphicsUtils.paintText method > where RootPane.altPress value is retrieved from UIManager each time. Storing > the value in it's constructor doesn't reflect the correct value and is always > `false`. No, I didn't refer to this particular case. I referred to my suggestion where you always add `altProcessor` as the keyboard listener and keep track of the value of the `RootPane.altPress` property. Anyway this approach seems to be rejected. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653029069
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:20:24 GMT, Abhishek Kumar wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
>> line 751:
>>
>>> 749: * Repaints all the components with the mnemonics in the given
>>> window and all its owned windows.
>>> 750: */
>>> 751: static void repaintMnemonicsInWindow(final Window w) {
>>
>> The `repaintMnemonicsInWindow` and `repaintMnemonicsInContainer` are exactly
>> the same as methods in `WindowsGraphicsUtils`. And `AquaMnemonicHandler` has
>> yet another copy of the same code.
>>
>> Is it possible to move these methods to a utility class that's available for
>> all Look-and-Feels to avoid duplicating code?
>
> I will check and update if it is possible.
Thank you for looking into it. A `MnemonicHandler` class in `sun.swing` or
`sun.swing.plaf` package could be a good candidate. The `sun.swing` package
contains a lot of support classes for Swing, including `SwingUtilities2` and
`UIAction`; the `sun.swing.plaf` may be better as the mnemonic handler is part
of PLAF.
Another option is the `com.sun.java.swing` package which currently contains
`SwingUtilities3` and `plaf` subpackage with `gtk` and `motif` related classes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653017112
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:21:49 GMT, Abhishek Kumar wrote: >>>Requesting the value of RootPane.altPress from UIManager each time >>>postProcessKeyEvent is called is inefficient, so you can store the value in >>>altProcessor when look and feel is installed. >> >> I guess you are pointing out the code in SynthGraphicsUtils.paintText method >> where RootPane.altPress value is retrieved from UIManager each time. Storing >> the value in it's constructor doesn't reflect the correct value and is >> always `false`. > >> If RootPane.altPress can be changed dynamically, you can install a >> PropertyChangeListener to UIManager. > > I think it can be changed but is it really required to handle it ? > I mean why does a user change it dynamically ? > In my initial fix, I added the `altProcessor` handler in > `SynthLookAndFeel.initialize` with condition check for GTK L Phil has > suggested not to check for GTK L instead look for some alternate way like > mentioned > [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). > > So, the handler implementation is moved to `SynthRootPaneUI`. Thus, out of Synth-derived Look and Feels, only GTK needs the feature. Can't the listener be installed in `GTKLookAndFeel.initialize`? At the same time, if you install the listener in `SynthLookAndFeel`, other L based on Synth could also use this feature, thus your way is more flexible. However, `SynthRootPaneUI` is still part of the base class… I still don't understand how it's different from what I'm proposing. I'd like to avoid keeping a flag whether the listener is installed or not… But there could be no other way since not all Synth-based L enable hiding mnemonics. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1652996654
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:19:49 GMT, Abhishek Kumar wrote: > Should I revert it back to javadoc style comment ? Absolutely! - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1652814116
Re: [jdk23] RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
On Tue, 25 Jun 2024 03:41:05 GMT, Prasanta Sadhukhan wrote: > 8334580: Deprecate no-arg constructor BasicSliderUI() for removal Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19874#pullrequestreview-2138314011
Re: RFR: 8334599: Improve code from JDK-8302671
On Thu, 20 Jun 2024 08:29:39 GMT, Julian Waters wrote:
> In [JDK-8302671](https://bugs.openjdk.org/browse/JDK-8302671) I fixed a
> memmove decay bug by rewriting a sizeof on an array to an explicit size of
> 256, but this is a bit of a band aid fix. It's come to my attention that in
> C++, one can pass an array by reference, which causes sizeof to work
> correctly on an array and has the added bonus of enforcing an array of that
> size on the arguments passed to that method. I've reverted my change from
> 8302671 and instead explicitly made kstate an array reference so that sizeof
> works on the array as expected, and that the array size can be explicitly set
> in the array brackets
>
> Verification: https://godbolt.org/z/Ezj76eWWY and GitHub Actions
Looks good to me.
I added a suggestion to use the `enum` constant declared in `AwtToolkit`
instead of hard-coding the size of the array.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3366:
> 3364: static void
> 3365: resetKbdState( BYTE ()[256]) {
> 3366: BYTE tmpState[256];
Suggestion:
resetKbdState( BYTE ()[AwtToolkit::KB_STATE_SIZE]) {
BYTE tmpState[AwtToolkit::KB_STATE_SIZE];
Will this resolve Phil's concern? Both arrays will use the same size.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 3368:
> 3366: BYTE tmpState[256];
> 3367: WCHAR wc[2];
> 3368: memmove(tmpState, kstate, sizeof(kstate));
Using `memcpy` could be more performant, we know for sure that `tmpState` and
`kstate` do not overlap.
-
Marked as reviewed by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2136706441
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1651586867
PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1651589012
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Mon, 24 Jun 2024 19:32:15 GMT, Nizar Benalla wrote: > Let me add a new commit to remove the sponsor label Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2187363695
RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
This is somewhat a continuation for [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). The former removed the `doIt` flag in #18584, but it introduced a regression. The regression is resolved by the latter in #19786, and it added the 'doIt' flag again. I think using a flag makes the code less clear. When reading the code, one has to keep track of the current value of the `doIt` flag. I raised my concern in [comments in PR 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > I'd rather keep `JNI_FALSE` here — it's more explicit and therefore clearer. > You don't have to keep track of the value of `doIt` flag while reading the > code. > > In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code > below `if (ret)` slightly harder. I came up with a solution which simplifies handling the clean-up. The solution relies on C++ destructors which are called when the declared objects go out of scope. The C++ object wraps a lambda function to clean up and invokes this function in its destructor. Such C++ class has to be a templated class because there's no defined type to represent a lambda. The class has to be declared at the top of the file because it needs C++ linkage. There are two `Cleaner` objects declared in the code of `Java_sun_awt_windows_WPageDialogPeer__1show`: **`refCleaner`** is declared right after all the references to Java objects are initialised. The corresponding `refCleanup` lambda deletes all the references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the code jumped to a go-to label to perform the clean-up. **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is called. Its lambda `postCleanup` uninstalls the modal hook and handles focus transfer as well as saves the updated printer parameters. It is `postCleaner` that resolves the bug JDK-8334868 by ensuring `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if `::PageSetupDlg` is called. As the result of using `refCleaner`, all the `return` statements in the function use an explicit value, which makes the code cleaner. There's no need to use a `go to` statement or insert a macro to delete references to Java objects, about which one had to remember — the destructor of the `refCleaner` handles deleting references when `refCleaner` goes out of scope, that is when the function returns. - Commit messages: - 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show Changes: https://git.openjdk.org/jdk/pull/19867/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19867=00 Issue: https://bugs.openjdk.org/browse/JDK-8334868 Stats: 92 lines in 1 file changed: 48 ins; 36 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19867.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19867/head:pull/19867 PR: https://git.openjdk.org/jdk/pull/19867
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Wed, 15 May 2024 03:38:29 GMT, Nizar Benalla wrote: >> If you're currently reviewing this PR, thank you! >> Most fixes here are according to the reports by the since checker tool in >> #18934 and are pretty simple. >> >> To make reviewing easier >> - `BasicSliderUI` has the constructor `public BasicSliderUI(JSlider b)` for >> a long time so the default constructor (without parameters) didn't exist >> until JDK 16 >> >> For the `package-info` files, it is pretty hard to find source code of JDK >> 1-5 so I used the `grep` command to find the oldest instance of an `@since` >> in those packages. >> >> I found instances of `@since 1.1` in the other packages but >> `javax/swing/plaf/synth/package-info.java` might be worth checking as most >> classes there had no `@since`. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Swing was added in JDK 1.2 The PR still looks good for me. Yet I suggest waiting until #19819 is integrated. It will *facilitate* reviewing the CSR and backporting that change to jdk23. Once PR 19819 is integrated, you'll have to merge master into your PR branch and resolve the conflict. Thank you for your understanding. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19192#pullrequestreview-2136564079
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v4]
On Mon, 24 Jun 2024 16:28:36 GMT, Prasanta Sadhukhan wrote: >> The no-arg constructor BasicSliderUI() was added under >> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This >> constructor should be deprecated for removal in future release > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > javadoc fix Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19819#pullrequestreview-2136431285
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v5]
On Fri, 21 Jun 2024 15:51:29 GMT, Prasanta Sadhukhan wrote: >> On cancelling PageDialog, same PageFormat object should be returned which >> stopped working after >> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160). >> Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct >> value is returned from PageDialog.show action.. >> An automated printing testcase is created since the issue was caught by >> manual test and so having another manual test run the risk of not being >> executed during CI testing.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > ALways return doIt > /backport openjdk/jdk23u @prsadhuk You should backport it to 23 which corresponds to `jdk23` branch in `jdk` repo. The command should look like this: `/backport :jdk23` or `/backport jdk jdk23`, see the [`/backport` command](https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/backport). - PR Comment: https://git.openjdk.org/jdk/pull/19786#issuecomment-2186694016
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v3]
On Mon, 24 Jun 2024 05:50:40 GMT, Prasanta Sadhukhan
wrote:
>> The no-arg constructor BasicSliderUI() was added under
>> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This
>> constructor should be deprecated for removal in future release
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added why
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java line
154:
> 152: * Constructs a {@code BasicSliderUI}.
> 153: * @deprecated This constructor was exposed erroneously and will be
> removed in next version.
> 154: * Use {@link #BasicSliderUI(JSlider b)} instead.
Suggestion:
* @deprecated This constructor was exposed erroneously and will be removed
in a future release.
* Use {@link #BasicSliderUI(JSlider)} instead.
I agree, _“in a future release”_ or _“version”_ would be more accurate.
I verified that it builds with the parameter name, however, it's more common to
omit parameter names.
-
PR Review: https://git.openjdk.org/jdk/pull/19819#pullrequestreview-2135839371
PR Review Comment: https://git.openjdk.org/jdk/pull/19819#discussion_r1651053840
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 07:55:24 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove access modifier from method declaration
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 751:
> 749: * Repaints all the components with the mnemonics in the given
> window and all its owned windows.
> 750: */
> 751: static void repaintMnemonicsInWindow(final Window w) {
The `repaintMnemonicsInWindow` and `repaintMnemonicsInContainer` are exactly
the same as methods in `WindowsGraphicsUtils`. And `AquaMnemonicHandler` has
yet another copy of the same code.
Is it possible to move these methods to a utility class that's available for
all Look-and-Feels to avoid duplicating code?
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 779:
> 777: c.repaint();
> 778: continue;
> 779: } else if (c instanceof Container){
You said you were handling menu mnemonics only, yet this is more than menus.
However, this would make the UI look consistent.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1053:
> 1051: * This method is a non operation if the underlying operating system
> 1052: * does not support the mnemonic hiding feature.
> 1053: */
You can still write proper javadocs for members with any visibility, and IDE
will show you the description of a method or a field when you hover over its
usage anywhere in the code.
You already wrote the javadoc, leave them.
A regular comment won't be shown this way.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java line
45:
> 43: private SynthStyle style;
> 44: static final AltProcessor altProcessor = new AltProcessor();
> 45: static boolean altProcessorInstalledFlag;
Wouldn't it be easier to install `altProcessor` always in
`SynthLookAndFeel.initialize` and to uninstall it in
`SynthLookAndFeel.uninitialize` like it's done in `WindowsLookAndFeel`:
https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L197-L198
Then `altProcessor` would do nothing if the `RootPane.altPress` property isn't
set or is `false`.
Requesting the value of `RootPane.altPress` from `UIManager` each time
`postProcessKeyEvent` is called is inefficient, so you can store the value in
`altProcessor` when look and feel is installed.
If `RootPane.altPress` can be changed dynamically, you can install a
`PropertyChangeListener` to `UIManager`.
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2133329175
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649390460
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649377154
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649372792
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649386106
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Fri, 21 Jun 2024 14:46:33 GMT, Renjith Kannath Pariyangad wrote: > I don't think there is any issue with native code, with this change I have > brought MAC same as other OS. Hm… It works correctly now. But if a page range is used, `SunPageSelection.RANGE` is added to attributes. Now it's not added as far as I can see. In addition to that, when a page range is set, the native code doesn't work correctly even though the arguments with the range are passed correctly. I believe there's a problem with the native code or with passing the parameters. Perhaps, the fix could be to always return `NO` from `knowsPageRange`. - PR Comment: https://git.openjdk.org/jdk/pull/19740#issuecomment-2183359828
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v2]
On Fri, 21 Jun 2024 18:33:37 GMT, Alexey Ivanov wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add forRemoval
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java line
> 153:
>
>> 151: /**
>> 152: * Constructs a {@code BasicSliderUI}.
>> 153: * @deprecated This constructor will be removed in future release
>
> Sounds reasonable enough. It's what happened, isn't it? It's the reason why
> we're deprecating it and planning to remove it.
>
> Suggestion:
>
> * @deprecated This constructor was added by accident. Do not use it.
> * This constructor will be removed in a future release.
>
>
> There are comments that say this method shouldn't have been public.
>
> On the other hand, there are quite a few methods and classes in the list of
> terminally terminated elements which don't say anything at all.
Or:
* @deprecated This constructor will be removed in a future release.
* Use {@link #BasicSliderUI(JSlider b)} instead.
This is in the gist of the deprecation message for
[`SecurityManager`](https://download.java.net/java/early_access/jdk24/docs/api/java.base/java/lang/SecurityManager.html).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19819#discussion_r1649319001
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v2]
On Fri, 21 Jun 2024 12:11:21 GMT, Prasanta Sadhukhan
wrote:
>> The no-arg constructor BasicSliderUI() was added under
>> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This
>> constructor should be deprecated for removal in future release
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add forRemoval
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java line
153:
> 151: /**
> 152: * Constructs a {@code BasicSliderUI}.
> 153: * @deprecated This constructor will be removed in future release
Sounds reasonable enough. It's what happened, isn't it? It's the reason why
we're deprecating it and planning to remove it.
Suggestion:
* @deprecated This constructor was added by accident. Do not use it.
* This constructor will be removed in a future release.
There are comments that say this method shouldn't have been public.
On the other hand, there are quite a few methods and classes in the list of
terminally terminated elements which don't say anything at all.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19819#discussion_r1649309588
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v2]
On Fri, 21 Jun 2024 18:08:45 GMT, Phil Race wrote: > But it means that if we deprecated the consructor with args we'd probably > want to look at those too. It seems like the ripple effect isn't worth it. It would clean up the code… Yes, we would need to modify all the subclasses too. > And what if there is later a need for the JSlider ? Unlikely I know .. The `JSlider` object is set in `installUI` method where a UI object gets associated with a particular component. Before `installUI` is called, the `slider` field is `null`. > but I'd prefer to correct the short-term mistake and leave everything else > alone. So, considering the ripple effect is not worth it, I guess. - PR Comment: https://git.openjdk.org/jdk/pull/19819#issuecomment-2183230243
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 20:03:20 GMT, Phil Race wrote: >> All I wanted is to bring up the inconsistency so that a few people would >> take a look at it while reviewing this change. > > It does look odd. Focus would need transferring in both cases I'd expect. > It goes back to the very beginning of open source JDK so I can't see a > changeset to point to in order to explain it. > And I also can't find any bug reports that might be related - either one > about adding it, or one about things not working because it is not always > executed. I looked at the history before what's available in Git. I looks this has always been this way. Yet it doesn't look right. `AwtDialog::CheckInstallModalHook()` is called right before the page dialog is displayed by using `::PageSetupDlg`. I expect `AwtDialog::CheckUninstallModalHook()` needs to be called after it. Likely, the early returns (inside `if (ret)`) are very rare, if any of these has ever occurred at all. I'll submit a bug to include calling `AwtDialog::CheckUninstallModalHook()` in error cleanup. The `done` label which were before [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) should've been before the line which calls `CheckUninstallModalHook`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1649289792
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Fri, 21 Jun 2024 03:17:38 GMT, Prasanta Sadhukhan wrote: >> The way it was "before" is that we always returned the value of "doIt". Why >> not restore that for consistency ? > > I believe that's what this PR is doing, it returns value of "doIt" at end, > isn't it? > The way it was "before" is that we always returned the value of "doIt". Why > not restore that for consistency ? I think it's clearer with explicit `JNI_FALSE` even though it's inconsistent. You don't have to track in your mind what value `doIt` has. > I believe that's what this PR is doing, it returns value of "doIt" at end, > isn't it? Yes, it does *now*. You changed it after Phil had left his comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1649205235
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v5]
On Fri, 21 Jun 2024 15:51:29 GMT, Prasanta Sadhukhan
wrote:
>> On cancelling PageDialog, same PageFormat object should be returned which
>> stopped working after
>> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160).
>> Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct
>> value is returned from PageDialog.show action..
>> An automated printing testcase is created since the issue was caught by
>> manual test and so having another manual test run the risk of not being
>> executed during CI testing..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> ALways return doIt
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 581:
> 579: if ((setup.hDevMode == NULL) && (setup.hDevNames == NULL)) {
> 580: CLEANUP_SHOW;
> 581: return doIt;
I'd rather keep `JNI_FALSE` here — it's more explicit and therefore *clearer*.
You don't have to keep track of the value of `doIt` flag while reading the code.
In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code below
`if (ret)` slightly harder.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1649191308
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v2]
On Fri, 21 Jun 2024 12:11:21 GMT, Prasanta Sadhukhan wrote: >> The no-arg constructor BasicSliderUI() was added under >> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This >> constructor should be deprecated for removal in future release > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Add forRemoval > > I was under impression that BasicSliderUI(JSlider b) did use its parameter > > but it doesn't. > > > > Should we keep the new constructor and deprecate the _old_ one? > > I guess if we remove the old one, it might mean incompatibility to > applications which might be using it/calling it.. Even `createUI `uses this > JSlider param constructor This is why we *cannot* remove the old constructor with the parameter but we can deprecate it. The `createUI` method can use new no-arg constructor, and nothing will change. - PR Comment: https://git.openjdk.org/jdk/pull/19819#issuecomment-2183068827
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v2]
On Fri, 21 Jun 2024 12:11:21 GMT, Prasanta Sadhukhan wrote: >> The no-arg constructor BasicSliderUI() was added under >> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This >> constructor should be deprecated for removal in future release > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Add forRemoval Now that I looked at the code more thoroughly, the no-arg constructor makes more sense actually. https://github.com/openjdk/jdk/blob/c41293a70834a79c79e859ebcdb8869884ac87dc/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java#L202-L207 I was under impression that `BasicSliderUI(JSlider b)` did use its parameter but it doesn't. Should we keep the new constructor and deprecate the *old* one? Should we remove the new constructor and keep the things as they've always been? - PR Comment: https://git.openjdk.org/jdk/pull/19819#issuecomment-2182916810
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v2]
On Fri, 21 Jun 2024 12:11:21 GMT, Prasanta Sadhukhan
wrote:
>> The no-arg constructor BasicSliderUI() was added under
>> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This
>> constructor should be deprecated for removal in future release
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add forRemoval
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java line
153:
> 151: /**
> 152: * Constructs a {@code BasicSliderUI}.
> 153: * @deprecated This constructor will be removed in future release
This needs a full stop at the end. It's usually added in `@deprecated` tags.
Suggestion:
* @deprecated This constructor will be removed in future release.
Should we explain *why* it's deprecated?
-
PR Review: https://git.openjdk.org/jdk/pull/19819#pullrequestreview-2132839628
PR Review Comment: https://git.openjdk.org/jdk/pull/19819#discussion_r1649072864
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
On Fri, 21 Jun 2024 11:10:56 GMT, Kevin Rushforth wrote:
>> The no-arg constructor BasicSliderUI() was added under
>> [JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852) by mistake. This
>> constructor should be deprecated for removal in future release
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java line
> 154:
>
>> 152: * Constructs a {@code BasicSliderUI}.
>> 153: */
>> 154: @Deprecated(since = "23")
>
> You need to add `forRemoval = true` to deprecate this for removal. Also, we
> typically add a `@deprecated` javadoc tag in addition to the `@Deprecated`
> annotation.
Yes, you're right.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19819#discussion_r1648826971
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Mon, 17 Jun 2024 05:54:37 GMT, Renjith Kannath Pariyangad
wrote:
> Hi Reviewers,
>
> This fix will resolve page range not printing proper pages if the rage begin
> from 2 or above on Mac machines.
> I have verified the manual range related tests like PageRanges.java,
> ClippedImages.java and test.java and confirmed its fixing the issue.
>
> Please review and let me know your suggestions if any.
I added some traces and ran the test `PageRanges.java` with and without your
proposed changes.
The diff:
diff --git a/src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java
b/src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java
index 416a3ee002b..6ddf46896b2 100644
--- a/src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java
+++ b/src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java
@@ -33,6 +33,7 @@
import java.net.URI;
import java.security.AccessController;
import java.security.PrivilegedAction;
+import java.util.Arrays;
import java.util.concurrent.atomic.AtomicReference;
import javax.print.*;
@@ -325,6 +325,8 @@ public void print(PrintRequestAttributeSet attributes)
throws PrinterException {
lastPage = mDocument.getNumberOfPages() - 1;
}
}
+System.out.println("print: firstPage = " + firstPage + "; "
+ + "lastPage = " + lastPage);
try {
synchronized (this) {
@@ -338,7 +340,11 @@ public void print(PrintRequestAttributeSet attributes)
throws PrinterException {
:
(PageRanges)attributes.get(PageRanges.class);
int[][] prMembers = (pr == null) ? new int[0][0] : pr.getMembers();
int loopi = 0;
+System.out.println("pr = " + pr);
+System.out.println("prMembers = " +
Arrays.deepToString(prMembers));
do {
+System.out.println("printLoop: firstPage = " + firstPage + "; "
+ + "lastPage = " + lastPage);
if (EventQueue.isDispatchThread()) {
// This is an AWT EventQueue, and this print rendering
loop needs to block it.
The output:
2-3 + 3-5 without Renjith changes
print: firstPage = 1; lastPage = 2
pr = 2-3
prMembers = [[2, 3]]
printLoop: firstPage = 1; lastPage = 2
print: firstPage = 2; lastPage = 4
pr = 3-5
prMembers = [[3, 5]]
printLoop: firstPage = 2; lastPage = 4
2-3 + 3-5 with Renjith changes
print: firstPage = 0; lastPage = -1
pr = 2-3
prMembers = [[2, 3]]
printLoop: firstPage = 0; lastPage = -1
print: firstPage = 0; lastPage = -1
pr = 3-5
prMembers = [[3, 5]]
printLoop: firstPage = 0; lastPage = -1
In the first case, I got page 3 and page 5 printed; with Renjith's changes, I
got the correct range printed 2-3 and 3-5.
So it works… kind of. I believe the bug is somewhere in native code.
What would be printed if `PageRanges` contains several ranges? This case should
be supported as well.
Since the last page to be printed is -1, `PrinterView->knowsPageRange` sets
`aRange->length` to `NSIntegerMax`. This does not look right, even though it
works.
https://github.com/openjdk/jdk/blob/08ace27da1d9cd215c77471eabf41417ff6282d2/src/java.desktop/macosx/native/libawt_lwawt/awt/PrinterView.m#L130-L152
Then, the Print dialog itself: with Renjith's changes the range of pages is
preserved. Without the changes, the range of pages isn't preserved. It's a
different bug: the dialog should be initialised using the attributes.
If `PageRanges` attribute is set and is supported, the dialog should select
**Range** radio button and set the correct page range.
A new test could be written (if it doesn't exist already) to confirm whether
attributes are taken into account when initialising the Print dialog and then a
new bug should be submitted.
-
PR Comment: https://git.openjdk.org/jdk/pull/19740#issuecomment-2182540891
PR Comment: https://git.openjdk.org/jdk/pull/19740#issuecomment-2182543047
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Thu, 20 Jun 2024 11:10:59 GMT, Renjith Kannath Pariyangad
wrote:
>> src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 225:
>>
>>> 223: if (isRangeSet) {
>>> 224: attributes.add(new PageRanges(from+1, to+1));
>>> 225: attributes.add(SunPageSelection.RANGE);
>>
>> why was this attribute added, and why is it being removed now? is the bug in
>> SunPageSelection?
>
>> why was this attribute added, and why is it being removed now? is the bug in
>> SunPageSelection?
>
> I am not sure why this attribute added, but noticed for other OS's this
> workflow will be taken care by _RasterPrinterJob_ . With this attribute
> application pass through range and ended up in invalid page printing range.
This attribute is usually set, it should be set, I think.
The proposed changeset does resolve the problem where some pages are missing
from printouts, but I cannot find a reasonable explanation as to why the
problem goes away.
On the unmodified version, I tried adding traces into the printing code on
macOS, the range of pages that's passed to the native code is correct, but the
result is wrong number of pages pages printed if the first page is equal or
greater than 2.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19740#discussion_r1647963800
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Mon, 17 Jun 2024 05:54:37 GMT, Renjith Kannath Pariyangad wrote: > Hi Reviewers, > > This fix will resolve page range not printing proper pages if the rage begin > from 2 or above on Mac machines. > I have verified the manual range related tests like PageRanges.java, > ClippedImages.java and test.java and confirmed its fixing the issue. > > Please review and let me know your suggestions if any. Is it possible to write an automated test which prints to a file? It would reduce the effort for testing if the expected range of pages is printed. Do you refer to [`test/jdk/java/awt/print/PrinterJob/PageRanges.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/PrinterJob/PageRanges.java) and [`test/jdk/java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java)? Is `Test.java` the test case attached to [JDK-8297191](https://bugs.openjdk.org/browse/JDK-8297191)? I couldn't test with `ClippedImages.java`, I didn't find a way to print to PDF. I used `PageRanges.java`; this test cannot be run as a jtreg test, but it works as a stand-alone app that can be run directly. It is relatively more convenient this way, the test needs to be updated, it is part of [JDK-8320677](https://bugs.openjdk.org/browse/JDK-8320677). You should add 8297191 to the list of bugids to the tests that you're using to verify this fix. I also noticed that after I apply your patch, the Print dialog preserves the previously selected range. I mean if I select pages 2 to 3 when the Print dialog is shown for the first time, the dialog comes up with Range from 2 to 3 selected when it's shown for the second time. Without the patch, the dialog has no selection (neither All Pages nor Range is selected) when it's shown for the first time. As far as I can see, there were two bugs which added support for page ranges on macOS: [JDK-7183520](https://bugs.openjdk.org/browse/JDK-7183520) and [JDK-8042713](https://bugs.openjdk.org/browse/JDK-8042713). - PR Review: https://git.openjdk.org/jdk/pull/19740#pullrequestreview-2131074084
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Wed, 15 May 2024 03:38:29 GMT, Nizar Benalla wrote: >> If you're currently reviewing this PR, thank you! >> Most fixes here are according to the reports by the since checker tool in >> #18934 and are pretty simple. >> >> To make reviewing easier >> - `BasicSliderUI` has the constructor `public BasicSliderUI(JSlider b)` for >> a long time so the default constructor (without parameters) didn't exist >> until JDK 16 >> >> For the `package-info` files, it is pretty hard to find source code of JDK >> 1-5 so I used the `grep` command to find the oldest instance of an `@since` >> in those packages. >> >> I found instances of `@since 1.1` in the other packages but >> `javax/swing/plaf/synth/package-info.java` might be worth checking as most >> classes there had no `@since`. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Swing was added in JDK 1.2 I verified all the added `@since` declarations, I found no inconsistencies. The no-arg constructor `BasicSliderUI()` was added in 16, therefore `@since 16` is correct. The constructor will be deprecated and removed by the follow-up bugs. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19192#pullrequestreview-2130421950
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Tue, 18 Jun 2024 17:09:08 GMT, Alexey Ivanov wrote: > > > How do we remove this constructor? Can it be removed right away? Should > > > it be deprecated for several releases before it's removed? > > > > > > Just delete it in all versions of 17+? > > Now it is part of Java 17 and 21. It can't be removed from these releases, I > believe. > > Can it be removed _quickly_ from the upcoming JDK 23? Probably, not as well. I've submitted the follow-up bugs to deal with the no-arg constructor `BasicSliderUI()`: 1. [JDK-8334580](https://bugs.openjdk.org/browse/JDK-8334580): Deprecate no-arg constructor BasicSliderUI() for removal 2. [JDK-8334581](https://bugs.openjdk.org/browse/JDK-8334581): Remove no-arg constructor BasicSliderUI() The plan is to deprecate `BasicSliderUI()` for removal in 23 and then to remove it in 25 or, if possible, in 24. - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2180653753
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 03:39:47 GMT, Prasanta Sadhukhan
wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 694:
>>
>>> 692: ::GlobalUnlock(setup.hDevMode);
>>> 693: }
>>> 694: doIt = JNI_TRUE;
>>
>> Another option would be to return `JNI_TRUE` here and to return `JNI_FALSE`
>> in the end of the function.
>
> Yes, it could have been done that way and my intiial fix in JBS is that only
> but I wanted to reinstate the flag as it is before
> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) which was working
> till now..
The only problem with the flag is that the data flow is not as clear.
I'm not insisting, it worked before and it will work in the future.
>> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 697:
>>
>>> 695: }
>>> 696:
>>> 697: AwtDialog::CheckUninstallModalHook();
>>
>> I wonder why the block of code at lines 697–709 is not needed if `JNI_FALSE`
>> is returned as a result of an error condition.
>>
>> No, it is not directly connected to the bug you're fixing, yet it doesn't
>> look right to me.
>
> We can have a followup bug on this I guess since it was existing before
> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) also..
All I wanted is to bring up the inconsistency so that a few people would take a
look at it while reviewing this change.
>> test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 60:
>>
>>> 58: t1.start();
>>> 59: PageFormat newFormat = pj.pageDialog(oldFormat);
>>> 60: if (!newFormat.equals(oldFormat)) {
>>
>> You should interrupt the `t1` thread after `pj.pageDialog` returns to stop
>> robot from sending `keyPress` and `keyRelease` after the dialog is hidden.
>> You can even use `Thread.sleep` instead of `Robot.delay` so that
>> interrupting the thread throws `InterruptedException` and its handler just
>> exits. (`Robot.delay` catches `InterruptedException` and then restores the
>> interrupted flag.)
>>
>> I wonder if any AWT event is sent when the Page Dialog is shown on the
>> screen, an event is more reliable and key presses won't be sent to an
>> arbitrary window in the system.
>
> I am not sure I understand this..I guess this thread execution will be a
> one-time activity so I guess we dont need to do any thread cleanup specially
> when the dialog is modal so it will only be cancelled (ie pageDialog will
> return) by VK_ESCAPE in the thread
You've resolved the problem.
In previous iteration, the keys were sent in a loop, which meant that the
thread could continue to run even after the page dialog was closed.
Now the `t1` thread presses `VK_ESCAPE` once and exits.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647370601
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647372620
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647377851
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v4]
On Thu, 20 Jun 2024 05:12:28 GMT, Prasanta Sadhukhan wrote: >> On cancelling PageDialog, same PageFormat object should be returned which >> stopped working after >> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160). >> Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct >> value is returned from PageDialog.show action.. >> An automated printing testcase is created since the issue was caught by >> manual test and so having another manual test run the risk of not being >> executed during CI testing.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Test fix headful Marked as reviewed by aivanov (Reviewer). test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 49: > 47: robot.keyPress(KeyEvent.VK_ESCAPE); > 48: robot.keyRelease(KeyEvent.VK_ESCAPE); > 49: robot.waitForIdle(); I think `waitForIdle` is redundant here: the thread doesn't do anything after pressing `VK_ESCAPE`. - PR Review: https://git.openjdk.org/jdk/pull/19786#pullrequestreview-2130079193 PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647379108
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Mon, 17 Jun 2024 05:07:22 GMT, Prasanta Sadhukhan
wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
>> line 369:
>>
>>> 367: @Override
>>> 368: public void setEnabled(boolean enabled) {
>>> 369: if (splitPane.isOneTouchExpandable() &&
>>
>> i think for setEnabled the buttons should be enabled/disabled regardless of
>> if splitPane isOneTouchExpandable is true so that the state is preserved
>> even if setEnabled(false) is called before
>> setOneTouchExpandable(true)
>
> No, the left and right arrow buttons are created ONLY when
> `isOneTouchExpandable` is true so it should be checked here too to
> enable/disable actions
Alisen's concern is valid.
What if `setEnabled(false)` is called when `isOneTouchExpandable` is `false`
and then `setOneTouchExpandable(true)` is called which adds the buttons? The
buttons are enabled when they should be disabled.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1646653721
Re: RFR: 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4
On Wed, 19 Jun 2024 19:20:24 GMT, Alexey Ivanov wrote: >>> You should rather call >>> [setDelay(50)](https://github.com/openjdk/jdk/blob/50bed6c67b1edd7736bdf79308d135a4e1047ff0/test/jdk/javax/swing/regtesthelpers/SwingTestHelper.java#L284-L294) >>> to add the delay between method calls. >> >> this change does exactly that, it calls `SwingTestHelper#setDelay` since >> `bug6492108 extends SwingTestHelper` >> >>> You should ask Vitaly to test your changeset in his environment to confirm >>> the failure is gone. >> >> I am able to reproduce the issue locally on Ubuntu 22.04, and the provided >> fix works fine for me. >> >> I assume @vprovodin has already done this testing, since he was the one who >> provided the solution in the JBS issue description. > > My bad, I read it as if it were `delay(50)`. Would it be clearer if `setDelay(50)` was called in the constructor of `bug6492108`? - PR Review Comment: https://git.openjdk.org/jdk/pull/19788#discussion_r1646628379
Re: RFR: 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4
On Wed, 19 Jun 2024 08:38:33 GMT, Abhishek Kumar wrote: > Test failed intermittently on Ubuntu 20.04, Ubuntu 22.04 system. Added a > delay to stable the test and multiple run in CI is Ok. Link is added in JBS. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19788#pullrequestreview-2128930008
Re: RFR: 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4
On Wed, 19 Jun 2024 15:38:10 GMT, Alexander Zvegintsev wrote: >>> How does it help? You're delaying EDT. >> >> I was unable to reproduce the failure scenario in my local machine but >> didn't observe any failure in mach5 also. Will ask Vitaly or @azvegint to >> verify as they are able to replicate the failure. > >> You should rather call >> [setDelay(50)](https://github.com/openjdk/jdk/blob/50bed6c67b1edd7736bdf79308d135a4e1047ff0/test/jdk/javax/swing/regtesthelpers/SwingTestHelper.java#L284-L294) >> to add the delay between method calls. > > this change does exactly that, it calls `SwingTestHelper#setDelay` since > `bug6492108 extends SwingTestHelper` > >> You should ask Vitaly to test your changeset in his environment to confirm >> the failure is gone. > > I am able to reproduce the issue locally on Ubuntu 22.04, and the provided > fix works fine for me. > > I assume @vprovodin has already done this testing, since he was the one who > provided the solution in the JBS issue description. My bad, I read it as if it were `delay(50)`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19788#discussion_r1646618947
Re: RFR: 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4
On Wed, 19 Jun 2024 08:38:33 GMT, Abhishek Kumar wrote: > Test failed intermittently on Ubuntu 20.04, Ubuntu 22.04 system. Added a > delay to stable the test and multiple run in CI is Ok. Link is added in JBS. You should ask Vitaly to test your changeset in his environment to confirm the failure is gone. test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 113: > 111: } > 112: setDelay(50); > 113: return panel; How does it help? You're delaying EDT. You should rather call [`setDelay(50)`](https://github.com/openjdk/jdk/blob/50bed6c67b1edd7736bdf79308d135a4e1047ff0/test/jdk/javax/swing/regtesthelpers/SwingTestHelper.java#L284-L294) to add the delay between method calls. Alternatively, you can add `-delay 50` to [the test arguments](https://github.com/openjdk/jdk/blob/50bed6c67b1edd7736bdf79308d135a4e1047ff0/test/jdk/javax/swing/regtesthelpers/SwingTestHelper.java#L460-L467) in its `@run` tag. - PR Review: https://git.openjdk.org/jdk/pull/19788#pullrequestreview-2127884342 PR Review Comment: https://git.openjdk.org/jdk/pull/19788#discussion_r1645934622
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 08:30:42 GMT, Prasanta Sadhukhan
wrote:
>> On cancelling PageDialog, same PageFormat object should be returned which
>> stopped working after
>> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160).
>> Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct
>> value is returned from PageDialog.show action..
>> An automated printing testcase is created since the issue was caught by
>> manual test and so having another manual test run the risk of not being
>> executed during CI testing..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Test fix
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 694:
> 692: ::GlobalUnlock(setup.hDevMode);
> 693: }
> 694: doIt = JNI_TRUE;
Another option would be to return `JNI_TRUE` here and to return `JNI_FALSE` in
the end of the function.
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 697:
> 695: }
> 696:
> 697: AwtDialog::CheckUninstallModalHook();
I wonder why the block of code at lines 697–709 is not needed if `JNI_FALSE` is
returned as a result of an error condition.
No, it is not directly connected to the bug you're fixing, yet it doesn't look
right to me.
test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 57:
> 55: robot.waitForIdle();
> 56: robot.delay(500);
> 57: });
These key presses are sent to whatever window has focus… before the Page Dialog
is shown and after it's hidden. Is pressing `VK_ESCAPE` not enough? Pressing
Esc is the same as clicking Cancel button.
test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 60:
> 58: t1.start();
> 59: PageFormat newFormat = pj.pageDialog(oldFormat);
> 60: if (!newFormat.equals(oldFormat)) {
You should interrupt the `t1` thread after `pj.pageDialog` returns to stop
robot from sending `keyPress` and `keyRelease` after the dialog is hidden. You
can even use `Thread.sleep` instead of `Robot.delay` so that interrupting the
thread throws `InterruptedException` and its handler just exits. (`Robot.delay`
catches `InterruptedException` and then restores the interrupted flag.)
I wonder if any AWT event is sent when the Page Dialog is shown on the screen,
an event is more reliable and key presses won't be sent to an arbitrary window
in the system.
-
PR Review: https://git.openjdk.org/jdk/pull/19786#pullrequestreview-2127813477
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645895912
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645900131
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645886899
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645894197
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Wed, 12 Jun 2024 20:55:51 GMT, Sergey Bylokhov wrote: > > How do we remove this constructor? Can it be removed right away? Should it > > be deprecated for several releases before it's removed? > > Just delete it in all versions of 17+? Now it is part of Java 17 and 21. It can't be removed from these releases, I believe. Can it be removed _quickly_ from the upcoming JDK 23? Probably, not as well. - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2176586495
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit**), the menu
>> mnemonics toggle on press of `ALT` key. Menu mnemonics are hidden initially
>> and then toggles between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> condition update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java line
82:
> 80: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 81: removeKeyEventPostProcessor(altProcessor);
> 82: }
I think you have to keep track of whether `altProcessor` was installed or not.
If the value of `"RootPane.altPress"` changes, the listener may be left
installed.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java line
115:
> 113: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 114: addKeyEventPostProcessor(altProcessor);
> 115: }
Can this lead to installing `altProcessor` twice or more?
test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 44:
> 42: import javax.swing.plaf.synth.SynthLookAndFeel;
> 43:
> 44: public class TestMenuMnemonicOnAltPress {
This test seems to repeat the
[`JMenuBar/TestMenuMnemonic.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java)
test that you created for Windows L when you worked on
[JDK-8326458](https://bugs.openjdk.org/browse/JDK-8326458).
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2126001490
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644763217
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644755969
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644770126
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit**), the menu
>> mnemonics toggle on press of `ALT` key. Menu mnemonics are hidden initially
>> and then toggles between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> condition update
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1063:
> 1061: } else {
> 1062: isMnemonicHidden = hide;
> 1063: }
Can we use or even re-use the logic in Windows L to display and hide menu
mnemonics?
Is this fix limited to menu mnemonics? Does it apply to all mnemonics in
components?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644753520
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Fri, 14 Jun 2024 09:28:28 GMT, Prasanta Sadhukhan wrote: >> Issue is seen in that if we call setEnabled(false) over JSplitPane than it >> can't be dragged via its divider, But if SplitPane have one touch expandable >> true than user can click those buttons and change the divider position. >> So, if splitpane is disabled, then both dragging in divider and >> one-touch-expandable click should be disabled. >> Fix is made to override setEnabled and disable one-touch-expandable buttons >> actions.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Omit gtk Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java line 376: > 374: leftButton.setEnabled(enabled); > 375: } > 376: } Is it possible to override `isEnabled` in `rightButton` and `leftButton` so that it returns the state of `JSplitPane`? Alternatively, the buttons could install a `PropertyChangeListener` for `"enabled"` property and align their state to the host split pane. test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 100: > 98: robot.mouseMove(loc.x, loc.y); > 99: robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); > 100: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); Isn't it enough to verify the state of the buttons whether they're enabled or not. It should be simpler, it could be done without rendering the frame. test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 147: > 145: rightOneTouchButton = super.createRightOneTouchButton(); > 146: return rightOneTouchButton; > 147: } Alternatively, you could add methods which return `leftButton` and `rightButton` fields. - PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2125353120 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644368671 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644371184 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644374062
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Mon, 17 Jun 2024 05:08:29 GMT, Prasanta Sadhukhan wrote: > All L including Aqua extends BasicSplitPaneUI so it's ok...The existing > test iterates through all L without any issue.. That is true, yet it is still possible to set a L that doesn't extend `BasicSplitPaneUI` and the updated code will throw `ClassCastException`. I'm sure such a situation is rare, if it exists at all, yet I don't think the public API should have such a limitation: `JSplitPane.setUI` accepts `SplitPaneUI`, so it is valid to pass an object that is not subclass of `BasicSplitPaneUI`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644363798
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Wed, 15 May 2024 03:38:29 GMT, Nizar Benalla wrote: >> If you're currently reviewing this PR, thank you! >> Most fixes here are according to the reports by the since checker tool in >> #18934 and are pretty simple. >> >> To make reviewing easier >> - `BasicSliderUI` has the constructor `public BasicSliderUI(JSlider b)` for >> a long time so the default constructor (without parameters) didn't exist >> until JDK 16 >> >> For the `package-info` files, it is pretty hard to find source code of JDK >> 1-5 so I used the `grep` command to find the oldest instance of an `@since` >> in those packages. >> >> I found instances of `@since 1.1` in the other packages but >> `javax/swing/plaf/synth/package-info.java` might be worth checking as most >> classes there had no `@since`. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Swing was added in JDK 1.2 Referring to the [discussion about BasicSliderUI() constructor](https://github.com/openjdk/jdk/pull/19192#discussion_r1600699721): > > Hmm, the _explicit_ default constructor was added in JDK 16, but it was > > implicit before then. > > I believe there was *no default* constructor in `BasicSliderUI()` because > there was a constructor with a parameter `BasicSliderUI(JSlider b)`. > > Thus, this case seem to be correct `BasicSliderUI()` is available since 16. > > > It seems that `BasicSliderUI()` was added by the mistake? it was not > > mentioned in the bug report... > > [[JDK-8250852](https://bugs.openjdk.org/browse/JDK-8250852)] Seems it is > > too late to delete it? > > I agree. It shouldn't have been added. > > Instead of adding `@since`, the constructor should be removed. It requires a > CSR. > > The longer it exists, the more chances there are that it's used. Taking all these points into account all these points above, _adding `@since 16` to the no-argument constructor of `BasicSliderUI()` is **correct**_. How do we remove this constructor? Can it be removed right away? Should it be deprecated for several releases before it's removed? @prrace @prsadhuk @mrserb src/java.desktop/share/classes/javax/swing/plaf/synth/package-info.java line 143: > 141: * } > 142: * > 143: * @since 1.5 This is correct, I verified the `javax.swing.plaf.synth` package is available in [Java 5](https://docs.oracle.com/javase/1.5.0/docs/api/javax/swing/plaf/synth/package-summary.html) but it wasn't in Java 1.4. - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2162815600 PR Review Comment: https://git.openjdk.org/jdk/pull/19192#discussion_r1636329855
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Tue, 11 Jun 2024 19:17:43 GMT, Jonathan Gibbons wrote:
>> src/java.desktop/share/classes/java/awt/geom/Path2D.java line 297:
>>
>>> 295: /**
>>> 296: * @since 10
>>> 297: */
>>
>> Not sure it's required…
>>
>> If it is, you should also add explicit `{@inheritDoc}`:
>>
>> Suggestion:
>>
>> /**
>> * {@inheritDoc}
>> *
>> * @since 10
>> */
>
> 1. You don't _need_ an explicit `{@inheritDoc}` although you may choose to do
> so for explicit clarity.
>
> 2. `@since` can be omitted for a member if the value would be the same as on
> the enclosing class
>
> 3. When present, `@since` on a method should indicate the first release in
> which the method is available for use on that class, with that VM signature
> (includes args and return type)
Thank you for clarification, @jonathan-gibbons. So, `@since` is required here.
I prefer an explicit `{@inheritDoc}`, this way the javadoc comment doesn't look
empty. I'm fine without adding `{@inheritDoc}` though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19192#discussion_r1636305506
Re: RFR: JDK-8333360 : PrintNullString.java doesn't use float arguments
On Mon, 10 Jun 2024 20:33:05 GMT, Alexey Ivanov wrote: > These improvements to the test can be combined with fixing > [JDK-8333026](https://bugs.openjdk.org/browse/JDK-8333026), or the test can > be updated separately. > > I'll submit a new bug to track this activity. Thank you for bringing it up. I've submitted [JDK-8334016](https://bugs.openjdk.org/browse/JDK-8334016): _Make `PrintNullString.java` semi-automatic_. - PR Comment: https://git.openjdk.org/jdk/pull/19540#issuecomment-2160667957
Re: RFR: JDK-8333360 : PrintNullString.java doesn't use float arguments
On Mon, 10 Jun 2024 20:15:55 GMT, Alisen Chung wrote: > Maybe outside the scope of this issue, but could this test be automated? I've been thinking about it, too. No, it can't be automated completely: the tester has to click Print / OK button in the Print dialog. Yes, other aspects of the test can be automated. The test code already detects a failure and reports it on the screen or on paper, thus the test can fail automatically if a failure condition is found. However, the first part where it paints on the screen can be fully automated. It could be moved to an independent test. These improvements to the test can be combined with fixing [JDK-8333026](https://bugs.openjdk.org/browse/JDK-8333026), or the test can be updated separately. I'll submit a new bug to track this activity. Thank you for bringing it up. - PR Comment: https://git.openjdk.org/jdk/pull/19540#issuecomment-2159232777
Re: RFR: JDK-8333360 : PrintNullString.java doesn't use float arguments
On Wed, 5 Jun 2024 04:53:56 GMT, Abhishek Kumar wrote:
>> Hi Reviewers,
>> I have updated the test case with passing float value for evaluation and a
>> typo. Please review and let me know your suggestions if any.
>
> test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 172:
>
>> 170:
>> 171: try {
>> 172: g2d.drawString(emptyIterator, 20.0f, 180.0f);
>
> Does the line below also need to change ?
> `g2d.drawString("FAILURE: No IAE for empty iterator, float", 20.0f 180.0f);`
No, it does not. It may though.
The test declares it verifies `drawString(Iterator, float, float)` — the test
does it now. How the error message is output on the screen isn't important:
either `int` or `float` version will do the same thing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19540#discussion_r1627655778
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Mon, 3 Jun 2024 23:33:53 GMT, Nizar Benalla wrote: > method: void java.awt.geom.Path2D.Double.trimToSize(): `@since` version is 9 > instead of 10 > method: void java.awt.geom.Path2D.Float.trimToSize(): `@since` version is 9 > instead of 10 In JDK 10, a new method `trimToSize` was added to Path2D. It is marked with `@since 10`. `Path2D.Float` and `Path2D.Double` override the method or rather implement it. Does this require an explicit `@since` tag? I'm unsure about it. - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2147928711
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Wed, 15 May 2024 03:38:29 GMT, Nizar Benalla wrote:
>> If you're currently reviewing this PR, thank you!
>> Most fixes here are according to the reports by the since checker tool in
>> #18934 and are pretty simple.
>>
>> To make reviewing easier
>> - `BasicSliderUI` has the constructor `public BasicSliderUI(JSlider b)` for
>> a long time so the default constructor (without parameters) didn't exist
>> until JDK 16
>>
>> For the `package-info` files, it is pretty hard to find source code of JDK
>> 1-5 so I used the `grep` command to find the oldest instance of an `@since`
>> in those packages.
>>
>> I found instances of `@since 1.1` in the other packages but
>> `javax/swing/plaf/synth/package-info.java` might be worth checking as most
>> classes there had no `@since`.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Swing was added in JDK 1.2
src/java.desktop/share/classes/java/awt/geom/Path2D.java line 297:
> 295: /**
> 296: * @since 10
> 297: */
Not sure it's required…
If it is, you should also add explicit `{@inheritDoc}`:
Suggestion:
/**
* {@inheritDoc}
*
* @since 10
*/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19192#discussion_r1626298196
Re: RFR: 8332103: since-checker - Add missing @ since tags to java.desktop [v2]
On Tue, 4 Jun 2024 00:02:56 GMT, Nizar Benalla wrote: >> It seems that BasicSliderUI() was added by the mistake? it was not mentioned >> in the bug report...Seems it is too late to delete it? > > I'm sorry but `method: void javax.swing.plaf.basic.BasicSliderUI.()` > refers to the constructor, > as I use [this > method](https://docs.oracle.com/en/java/javase/22/docs/api/java.compiler/javax/lang/model/element/ExecutableElement.html#getSimpleName()) > to get a method's name. > > I am saying that there was no default constructor before JDK 16 as it doesn't > appear in the compiler's historical data until then and therefore warrants an > `@since` > > I am stealing my colleagues words but here is the general rule for when we > want to add an `@since` until we publish a doc with rules for `@since` > >> As a practical rule for deciding whether any declaration is new or not, >> imagine writing a test program that refers to the most specific form of the >> declaration. If that test program does not compile on JDK version N-1 and >> does compile on version N, then it warrants having `@since N`. Put another >> way, `@since N` should identify the first release in which the declaration >> can be used in the given form > It seems that BasicSliderUI() was added by the mistake? it was not mentioned > in the bug report...Seems it is too late to delete it? I agree. It shouldn't have been added. Instead of adding `@since`, the constructor should be removed. It requires a CSR. The longer it exists, the more chances there are that it's used. - PR Review Comment: https://git.openjdk.org/jdk/pull/19192#discussion_r1626272799
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v7]
On Tue, 4 Jun 2024 15:37:00 GMT, Abhishek Kumar wrote: >> bug6492108.java test always fails in GTK L in single as well as dual >> screen linux machines. Since this test was not marked as "_headful_" in it's >> initial version, it never failed but after the fix of >> [JDK-8287051](https://bugs.openjdk.org/browse/JDK-8287051) this test was >> problem listed as it always >> failed which is captured in the JBS. >> The reason of failure is the pixel color mismatch between JEditorPane and >> JTextArea/JTextPane which is caused by the JEditorPane's default opaque >> value which is false for GTK3 at >> [L767](https://github.com/kumarabhi006/jdk/blob/c642f44bbe1e4cdbc23496a34ddaae30990ce7c0/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L767). >> In initial load JEditorPane, JTextArea and JTextPane components are opaque >> at >> [L718](https://github.com/kumarabhi006/jdk/blame/6c7656678916ff3f5c9fc70efcbb69ce76801458/jdk/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L718) >> but after the fix for >> [JDK-8145547](https://bugs.openjdk.org/browse/JDK-8145547) the >> implementation was changed to provide conditional support for GTK3 on linux >> where few components like Editor Pane, Formatted text Field, Password Field >> etc are opaque only if the [GTK version is not >> 3](https://github.com/kumarabhi006/jdk/blame/73d2181d56063f6015e4fc42e130591bee39bc36/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L746C21-L746C21). >> JTextPane's issue was observed by >> [JDK-8218479](https://bugs.openjdk.org/browse/JDK-8218479) and then the >> default opacity is set to true for JTextPane [irrespective of GTK >> version](https://github.com/kumarabhi006/jdk/blame/c642f44bbe1e4cdbc23496a34ddaae30990ce7c0/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L750C16-L750C16). >> Extending the fix in isOpaque() method in GTKStyle.java for JEditorPane >> similar to JTextPane resolves the issue. >> >> Test verified in both single and dual screen linux machines. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > copyright year update Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19381#pullrequestreview-2096810770
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v5]
On Tue, 4 Jun 2024 14:46:37 GMT, Abhishek Kumar wrote:
>> src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java
>> line 1:
>>
>>> 1: /*
>>
>> You should update the copyright year.
>
> Updated.
I can't see it in the PR. Didn't push?
>> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 148:
>>
>>> 146: if (refPixel != testPixel) {
>>> 147: fail("Image comparison failed at (" +
>>> 148: x + "," + y + ") for image " + index);
>>
>> Adding `count` and `k` to diagnostic message could help identifying the
>> failing case quicker.
>
> Don't find adding `k` to diagnostic message useful.. failure message is more
> relevant now about the images which are compared.
The images themselves are more useful for sure. Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626209941
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626214172
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v3]
On Mon, 3 Jun 2024 05:48:18 GMT, Abhishek Kumar wrote: >> Previous formatting was less confusing and aligned with Java Coding Style >> Guidelines. >> >> The parameters to the method are aligned to the opening parenthesis; the >> `throws` clause is not part of the parameters and it's placed on another >> line according to rules of wrapping lines (it should've used >> double-indentation of 8 spaces rather than 4 spaces to indicate a >> continuation line). > > Reverted back to previous formatting with double-indentation. Hopefully this > should be ok now. This formatting looks good to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626008412
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v5]
On Mon, 3 Jun 2024 05:52:31 GMT, Abhishek Kumar wrote:
>> bug6492108.java test always fails in GTK L in single as well as dual
>> screen linux machines. Since this test was not marked as "_headful_" in it's
>> initial version, it never failed but after the fix of
>> [JDK-8287051](https://bugs.openjdk.org/browse/JDK-8287051) this test was
>> problem listed as it always
>> failed which is captured in the JBS.
>> The reason of failure is the pixel color mismatch between JEditorPane and
>> JTextArea/JTextPane which is caused by the JEditorPane's default opaque
>> value which is false for GTK3 at
>> [L767](https://github.com/kumarabhi006/jdk/blob/c642f44bbe1e4cdbc23496a34ddaae30990ce7c0/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L767).
>> In initial load JEditorPane, JTextArea and JTextPane components are opaque
>> at
>> [L718](https://github.com/kumarabhi006/jdk/blame/6c7656678916ff3f5c9fc70efcbb69ce76801458/jdk/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L718)
>> but after the fix for
>> [JDK-8145547](https://bugs.openjdk.org/browse/JDK-8145547) the
>> implementation was changed to provide conditional support for GTK3 on linux
>> where few components like Editor Pane, Formatted text Field, Password Field
>> etc are opaque only if the [GTK version is not
>> 3](https://github.com/kumarabhi006/jdk/blame/73d2181d56063f6015e4fc42e130591bee39bc36/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L746C21-L746C21).
>> JTextPane's issue was observed by
>> [JDK-8218479](https://bugs.openjdk.org/browse/JDK-8218479) and then the
>> default opacity is set to true for JTextPane [irrespective of GTK
>> version](https://github.com/kumarabhi006/jdk/blame/c642f44bbe1e4cdbc23496a34ddaae30990ce7c0/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L750C16-L750C16).
>> Extending the fix in isOpaque() method in GTKStyle.java for JEditorPane
>> similar to JTextPane resolves the issue.
>>
>> Test verified in both single and dual screen linux machines.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Code formatting update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java line 1:
> 1: /*
You should update the copyright year.
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 64:
> 62: } catch (Exception e) {
> 63: throw new SkippedException("GTK LAF is not supported on this
> system;" +
> 64: " test passes");
Suggestion:
throw new SkippedException("GTK LAF is not supported on this
system");
I'd remove the additional message. I believe it was `println` before.
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 116:
> 114: }
> 115:
> 116: private void onBackgroundThread20() {
This method should run on EDT because it accesses the state of the components.
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 124:
> 122: Point loc = ref.getLocationOnScreen();
> 123: Rectangle refRect =
> 124: new Rectangle(loc.x, loc.y, ref.getWidth(),
> ref.getHeight());
Suggestion:
Rectangle refRect = new Rectangle(loc, ref.getSize());
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 133:
> 131: Rectangle testRect =
> 132: new Rectangle(loc.x, loc.y,
> 133: test.getWidth(), test.getHeight());
Suggestion:
loc = test.getLocationOnScreen();
Rectangle testRect = new Rectangle(test.getLocationOnScreen(),
test.getSize());
Choose one style and adjust both statements, `refRect` and `testRect`.
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
> 134: BufferedImage testimg =
> robot.createScreenCapture(testRect);
> 135:
> 136: if (refimg.getWidth() != testimg.getWidth()
I suggest moving the part that compares the images into a helper method.
Alternatively, you can use
[`Util.compareBufferedImages`](https://github.com/openjdk/jdk/blob/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L80).
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 137:
> 135:
> 136: if (refimg.getWidth() != testimg.getWidth()
> 137:|| refimg.getHeight() != testimg.getHeight())
Suggestion:
if (refimg.getWidth() != testimg.getWidth()
|| refimg.getHeight() != testimg.getHeight())
One more space, it should be aligned to the inside of the parenthesis.
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 148:
> 146: if (refPixel != testPixel) {
> 147: fail("Image comparison failed at (" +
Re: RFR: JDK-8333360 : PrintNullString.java doesn't use float arguments
On Tue, 4 Jun 2024 12:07:40 GMT, Renjith Kannath Pariyangad wrote: > Hi Reviewers, > I have updated the test case with passing float value for evaluation and a > typo. Please review and let me know your suggestions if any. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19540#pullrequestreview-2096332476
Re: RFR: 8325435: [macos] Menu or JPopupMenu not closed when main window is resized [v2]
On Mon, 3 Jun 2024 06:07:37 GMT, Prasanta Sadhukhan
wrote:
>> src/java.desktop/macosx/native/libawt_lwawt/awt/AWTWindow.m line 1031:
>>
>>> 1029: (p.x >= (frame.origin.x + contentRect.size.width -
>>> 3)) ||
>>> 1030: (fabs(frame.origin.x - p.x) < 3) ||
>>> 1031: (fabs(frame.origin.y - p.y) < 3)) {
>>
>> Does `fabs` use float? It makes code more compact but it may be imprecise
>> and less performant than integer arithmetics.
>
> Yes..https://man7.org/linux/man-pages/man3/fabs.3.html and [CGFLoat
> ](https://developer.apple.com/documentation/corefoundation/cgfloat?language=objc)
> can be double/float and fabs can accomodate both, in this case it uses
> double...We have tested on 3 different systems without any issue so would
> keep it for now until we find any adverse effect and also it is better to
> accommodate double in hi-dpi environment which is the case in OSX
I see, the coordinates are floats, then it's perfectly fine. I assumed the
coordinates were integers.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19474#discussion_r1625831262
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v3]
On Fri, 31 May 2024 04:39:24 GMT, Abhishek Kumar wrote:
>> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
>>
>>> 134: if (refimg.getWidth() != testimg.getWidth() ||
>>> 135: refimg.getHeight() != testimg.getHeight())
>>> 136: {
>>
>> move bracket to previous line to be consistent
>
> Initially I thought of doing so but in one of the PR it was mentioned that
> "We generally prefer the { on a new line if the conditional is multi-line"
> https://github.com/openjdk/jdk/pull/18703#discussion_r1571126135.
>
> So I left like that.
I can't say that “We generally prefer the { on a new line if the conditional is
multi-line” really holds, yet I agree it makes code less confusing by
separating the condition and its continuation lines from the inside block.
To make the continuation line stand out as continuation line, place the `||` at
the start of wrapped line.
Again, there's no agreed upon set of style guidelines.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1622939563
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v3]
On Fri, 31 May 2024 05:02:16 GMT, Abhishek Kumar wrote:
>> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 70:
>>
>>> 68: Class
>>> type)
>>> 69: throws Throwable
>>> 70: {
>>
>> I think formatting here looks a little odd, could probably move the bracket
>> onto the same line as `throws Throwable` and shift it over to the right to
>> match `Class type)`
>
> Updated.
Previous formatting was less confusing and aligned with Java Coding Style
Guidelines.
The parameters to the method are aligned to the opening parenthesis; the
`throws` clause is not part of the parameters and it's placed on another line
according to rules of wrapping lines (it should've used double-indentation of 8
spaces rather than 4 spaces to indicate a continuation line).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1622937272
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L [v4]
On Thu, 30 May 2024 07:30:10 GMT, Abhishek Kumar wrote: >> Yeah, true.. > > Tested in CI and there is no failure for multiple run. Removed this method. > But as I told before this may not impact the result of the test, it is just > to add extra delay to provide visual verification. In this case, it can be removed. We want tests to run fast, don't we? When debugging, it makes sense to increase the delays so that you can see what's going on on the screen. > Tested in CI and there is no failure for multiple run. Removed this method. Thanks! I noticed it after I had written the comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1622931792
Re: RFR: 8325435: [macos] Menu or JPopupMenu not closed when main window is resized [v2]
On Fri, 31 May 2024 12:24:42 GMT, Prasanta Sadhukhan
wrote:
>> Issue is in macosx, when a JMenu or JPopupmenu is opened and then window is
>> resized from the lower right corner, then the Menu / JPopupmenu stays open
>> unlike in native osx apps like Notes, Mail etc..
>>
>> This is because when LMouseButton is pressed on non-client area, the window
>> should get a UngrabEvent for it to close/cancel the popupmenu (as is done in
>> [windows](https://github.com/openjdk/jdk/blob/f608918df3f887277845db383cf07b0863bba615/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#L618-L620))
>> but it was seen that the mac AWTWindow code only recognizes the title-bar
>> as the non-client area so
>> [notifyNCMouseDown](https://github.com/openjdk/jdk/blob/f608918df3f887277845db383cf07b0863bba615/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#L797-L804)
>> is not called.
>> Fix is made to recognize the edges of the window as non-client area
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Non-client aread edge area modified, test format
src/java.desktop/macosx/native/libawt_lwawt/awt/AWTWindow.m line 1031:
> 1029: (p.x >= (frame.origin.x + contentRect.size.width - 3))
> ||
> 1030: (fabs(frame.origin.x - p.x) < 3) ||
> 1031: (fabs(frame.origin.y - p.y) < 3)) {
Does `fabs` use float? It makes code more compact but it may be imprecise and
less performant than integer arithmetics.
test/jdk/javax/swing/JMenu/TestUngrab.java line 82:
> 80: robot.waitForIdle();
> 81: robot.delay(1000);
> 82: SwingUtilities.invokeAndWait(() -> {
Suggestion:
robot.delay(1000);
SwingUtilities.invokeAndWait(() -> {
Adding blank lines between sections of code could improve its readability.
test/jdk/javax/swing/JMenu/TestUngrab.java line 101:
> 99: robot.delay(1000);
> 100: System.out.println("isPopupMenuVisible " +
> menu.isPopupMenuVisible());
> 101: if (menu.isPopupMenuVisible()) {
`menu.isPopupMenuVisible()` should also be accessed on EDT.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19474#discussion_r1622540182
PR Review Comment: https://git.openjdk.org/jdk/pull/19474#discussion_r1622542519
PR Review Comment: https://git.openjdk.org/jdk/pull/19474#discussion_r1622533420
Re: RFR: 8332103: Add missing `@since` tags to `java.desktop` [v2]
On Tue, 14 May 2024 23:36:13 GMT, Nizar Benalla wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSliderUI.java
>> line 154:
>>
>>> 152: * Constructs a {@code BasicSliderUI}.
>>> 153: *
>>> 154: * @since 16
>>
>> Hmm, the *explicit* default constructor was added in JDK 16, but it was
>> implicit before then.
>> So I am not 100% sure what the right answer is - the same as the class, or
>> when it was explicitly added.
>
> When mapping methods and when they first appeared (by using the historical
> record built into javac) I use an id in the form of
> `method:
> .()` so for
> covariant overrides in general, when the return type changes I consider it to
> be a new method.
>
> Looking at the contents of the dictionnary:
> This explicit constructor existed for a long time but then this new was added
> a new one was added in JDK 16
> | Key | Value |
> | - | - |
> | `method: void
> javax.swing.plaf.basic.BasicSliderUI.(javax.swing.JSlider):` | 9 |
> | `method: void javax.swing.plaf.basic.BasicSliderUI.():` | 16 |
>
> Note: JDK 9 is used as the "base" as that's how far I can reliably use the
> `--release` info, so if something was added in JDK 2,5.7,9. It has a value of
> "9" in the dictionnary. I mainly check for errors in newer code.
> Hmm, the _explicit_ default constructor was added in JDK 16, but it was
> implicit before then. So I am not 100% sure what the right answer is - the
> same as the class, or when it was explicitly added.
I believe there was no default constructor in `BasicSliderUI()` because there
was a constructor with a parameter `BasicSliderUI(JSlider b)`.
Thus, this case seem to be correct `BasicSliderUI()` is available since 16.
At the same time, `BasicSliderUI(JSlider b)` has existed since at least 7, the
constructor is present in the history of the file. The history in GitHub goes
up to 1st December 2007 which corresponds to Java 7 timeline. I'm pretty sure
this constructor existed in previous releases, and you have to dig further to
find when it was added.
Very much likely, the constructor `BasicSliderUI(JSlider b)` was added when the
`BasicSliderUI` class was added. The class does not have `@since` tag, so it's
inherited from the package, isn't it? The same rule applies to the constructor,
doesn't it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19192#discussion_r1618748176
Re: RFR: 8332103: Add missing `@since` tags to `java.desktop`
On Tue, 14 May 2024 23:45:23 GMT, Nizar Benalla wrote: > but for older code you can only guess "Element: X existed before JDK 10". So > I was left to check on my own, and made a mistake. Why is it? There's history beyond 10 and 9, yet accessing it requires more effort. In addition to that, old JDK releases are available in an archive so that you can verify if a method existed in a certain release. If all the methods need `@since` marker, it has to be accurate and represent the real picture rather than an estimate. - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2137200506
Integrated: 8326734: text-decoration applied to lost when mixed with or
On Fri, 29 Mar 2024 15:32:12 GMT, Alexey Ivanov wrote: > The value of the > [`text-decoration`](https://www.w3.org/TR/REC-CSS1/#text-decoration) CSS > property is not inherited correctly in Swing. If the `` element is > mixed with `` or ``, only the value from the `style` attribute of > `` is applied. > > The fix to this issue is not as simple as that for the previous one in PR > #17659, [JDK-8323801](https://bugs.openjdk.org/browse/JDK-8323801). Even in > the seemingly simple case where `` is followed by ` style='text-decoration: line-through'>`, the situation is more complex > because the styles are stored in `MuxingAttributeSet` in different elements > of the array. > > To resolve this problem, `CSS.Attribute.TEXT_DECORATION` is treated as a > special case. Indeed, it is a special case: the values set to a single > `text-decoration` property should be combined across the entire tree of > nested HTML elements and their styles. > > So, `MuxingAttributeSet` looks for `text-decoration` in the entire array and > combines all the values. > > The same way, `StyleSheet` also goes up the inheritance chain by combining > the current value of `text-decoration` with that from `getResolveParent`. > > The `ConvertSpanAction` combines the value of `text-decoration` of adjacent > `` elements. > > Finally, `ConvertAction` and `CharacterAction` are refactored. The > `ConvertAction` class duplicated the code from `CharacterAction`. Now > `ConvertAction` extends `CharacterAction` and overrides a method to provide > additional handling. > > Thus, [JDK-8325620](https://bugs.openjdk.org/browse/JDK-8325620) is also > resolved by this PR, the action used for ``, ``, `` is > `CharacterAction` as specified. This pull request has now been integrated. Changeset: cd3e4c03 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/cd3e4c03661f770ebeefcd3637d56589243ac0a9 Stats: 595 lines in 8 files changed: 539 ins; 32 del; 24 mod 8326734: text-decoration applied to lost when mixed with or 8325620: HTMLReader uses ConvertAction instead of specified CharacterAction for , , Reviewed-by: honkar, prr - PR: https://git.openjdk.org/jdk/pull/18550
Re: RFR: 8332403: Anachronistic reference to Netscape Communicator in Swing API docs [v2]
On Fri, 17 May 2024 12:16:15 GMT, Prasanta Sadhukhan wrote: >> Inadvertent mention of Netscape in Javadoc is removed.. > > Prasanta Sadhukhan has updated the pull request incrementally with two > additional commits since the last revision: > > - doc clarity > - doc clarity Marked as reviewed by aivanov (Reviewer). src/java.desktop/share/classes/javax/swing/CellEditor.java line 91: > 89: * cell should be selected. However, it is useful to return false to > 90: * keep the selection from changing for some types of edits, > 91: * eg. in a table that contains a column of check boxes, the user > might Most guides recommend to spell _“e.g.”_ with the period after each letter, see [an article at Grammarly](https://www.grammarly.com/blog/know-your-latin-i-e-vs-e-g/). Alternatively, it could be replaced with _“for example”_. - PR Review: https://git.openjdk.org/jdk/pull/19276#pullrequestreview-2063530879 PR Review Comment: https://git.openjdk.org/jdk/pull/19276#discussion_r1605063445
Re: RFR: 8321428: Deprecate for removal the package java.beans.beancontext [v5]
On Wed, 15 May 2024 17:39:16 GMT, Larry Cable wrote: >> the beancontext package was added (by me) in JDK 1.2 to provide >> JavaBeans(tm) with a containment and services hierarchy. >> >> based upon concepts from OpenDoc, which was a popular component model at the >> time, the API pre-dated the addition of language features such as >> annotations, and the invention and adoption of programming design patterns >> such as "Inversion of Control" and/or "Dependency Injection". >> >> This API (package) has not evolved with either the language or current >> design trends, as such it is, at best, anachronistic, and is probably an >> anti-pattern that should be avoided. >> >> This package is therefore deprecated **FOR REMOVAL IN AT FUTURE RELEASE** > > Larry Cable has updated the pull request incrementally with one additional > commit since the last revision: > > removed trailing whitespace reported by jcheck Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18569#pullrequestreview-2063230682
