[jdk23] RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
8334580: Deprecate no-arg constructor BasicSliderUI() for removal - Commit messages: - Backport e527e1c32fcc7b2560cec540bcde930075ac284a Changes: https://git.openjdk.org/jdk/pull/19874/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19874=00 Issue: https://bugs.openjdk.org/browse/JDK-8334580 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19874.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19874/head:pull/19874 PR: https://git.openjdk.org/jdk/pull/19874
Integrated: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
On Fri, 21 Jun 2024 03:31:50 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 This pull request has now been integrated. Changeset: e527e1c3 Author:Prasanta Sadhukhan URL: https://git.openjdk.org/jdk/commit/e527e1c32fcc7b2560cec540bcde930075ac284a Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod 8334580: Deprecate no-arg constructor BasicSliderUI() for removal Reviewed-by: kcr, aivanov, iris, prr - PR: https://git.openjdk.org/jdk/pull/19819
Integrated: 8185429: [macos] After a modal dialog is closed, no window becomes active
On Thu, 6 Jun 2024 23:28:07 GMT, Alisen Chung wrote: > Add a check for previous focused window on modal unblocking. If the owner of > a closing dialog was the last focused window, then the owner of the dialog > should regain focus. This pull request has now been integrated. Changeset: 3a26bbce Author:Alisen Chung URL: https://git.openjdk.org/jdk/commit/3a26bbcebc2f7d11b172f2b16192a3adefeb8111 Stats: 8 lines in 2 files changed: 6 ins; 1 del; 1 mod 8185429: [macos] After a modal dialog is closed, no window becomes active Reviewed-by: tr, dnguyen, serb - PR: https://git.openjdk.org/jdk/pull/19588
Re: RFR: 8333403: Write a test to check various components events are triggered properly [v3]
On Fri, 14 Jun 2024 04:44:24 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
Tested on MacOS. The test passes and looks functional. Minor comments.
Have you tried on Windows with different scaling? Looks like the only component
location you access is for the panel position, which should be safe. I tried
different scalings on MacOS with no issue. Curious because this was an issue
with JButton a while before.
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();
Suggestion:
private volatile static ArrayList events =
new ArrayList<>();
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 187:
> 185: for (int j = 0; j < events.size();
> 186: System.err.print(events.get(j) + "; "), j++);
> 187: System.err.println("");
Suggestion:
System.err.println();
Same for the one on line 202.
-
PR Review: https://git.openjdk.org/jdk/pull/19521#pullrequestreview-2137062290
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1651815703
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1651815934
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation [v2]
On Mon, 24 Jun 2024 22:25:22 GMT, Phil Race wrote: >> Migrate font code from jdk.internal.misc.Unsafe to using FFM. >> This reduces the coupling between the java.desktop module and the internals >> of the java.base module. >> >> The code being changed here is not particularly performance sensitive, and >> it is not executed in the most common cases. >> The main impact performance-wise is a total of around 37ms in initialisation >> costs on my x64 macbook. >> A minimal program that just draws a string to an image - does not even put >> up a window - runs at around 690-700ms. >> There's variability in that number and the overall time for a JDK without >> the change is around (660-670ms) >> In the small test, this is the first and only use of FFM, so the one-off >> part cost should move elsewhere when FFM starts >> to be used earlier in the JDK itself. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8334495 DId ffm team check from where that slow down come from? is it expected to lose 5%? >In the small test, this is the first and only use of FFM, so the one-off part >cost should move elsewhere when FFM starts to be used earlier in the JDK itself. So the changed code path will work as before (w/o slowdown) if the https://github.com/openjdk/jdk/pull/15476 will be touched by the test? and slow down will occur if "sun.font.layout.ffm" will be set to false? Just would like to confirm that we will not get "multiplication of slowdowns" for each ffm usage. - PR Comment: https://git.openjdk.org/jdk/pull/19777#issuecomment-2187695619
Re: RFR: 8185429: [macos] After a modal dialog is closed, no window becomes active [v2]
On Fri, 7 Jun 2024 20:34:23 GMT, Alisen Chung wrote: >> Add a check for previous focused window on modal unblocking. If the owner of >> a closing dialog was the last focused window, then the owner of the dialog >> should regain focus. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > use == over .equals Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19588#pullrequestreview-2137035590
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation [v2]
> Migrate font code from jdk.internal.misc.Unsafe to using FFM. > This reduces the coupling between the java.desktop module and the internals > of the java.base module. > > The code being changed here is not particularly performance sensitive, and it > is not executed in the most common cases. > The main impact performance-wise is a total of around 37ms in initialisation > costs on my x64 macbook. > A minimal program that just draws a string to an image - does not even put up > a window - runs at around 690-700ms. > There's variability in that number and the overall time for a JDK without the > change is around (660-670ms) > In the small test, this is the first and only use of FFM, so the one-off part > cost should move elsewhere when FFM starts > to be used earlier in the JDK itself. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8334495 - Changes: - all: https://git.openjdk.org/jdk/pull/19777/files - new: https://git.openjdk.org/jdk/pull/19777/files/aa566257..99acf847 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19777=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19777=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19777.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19777/head:pull/19777 PR: https://git.openjdk.org/jdk/pull/19777
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 [v3]
> 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: See if an empty commit removes sponsor label - Changes: - all: https://git.openjdk.org/jdk/pull/19192/files - new: https://git.openjdk.org/jdk/pull/19192/files/7e8244db..5f68fe92 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19192=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19192=01-02 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19192.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19192/head:pull/19192 PR: https://git.openjdk.org/jdk/pull/19192
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 Let me add a new commit to remove the sponsor label - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2187265007
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: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation
On Tue, 18 Jun 2024 20:31:58 GMT, Phil Race wrote: > Migrate font code from jdk.internal.misc.Unsafe to using FFM. > This reduces the coupling between the java.desktop module and the internals > of the java.base module. > > The code being changed here is not particularly performance sensitive, and it > is not executed in the most common cases. > The main impact performance-wise is a total of around 37ms in initialisation > costs on my x64 macbook. > A minimal program that just draws a string to an image - does not even put up > a window - runs at around 690-700ms. > There's variability in that number and the overall time for a JDK without the > change is around (660-670ms) > In the small test, this is the first and only use of FFM, so the one-off part > cost should move elsewhere when FFM starts > to be used earlier in the JDK itself. src/java.desktop/share/classes/sun/font/StrikeCache.java line 257: > 255: byte[] bytes = new byte[sz]; > 256: MemorySegment.copy(pixelData, ValueLayout.JAVA_BYTE, 0, bytes, > 0, sz); > 257: return bytes; You could use `toArray` here: Suggestion: return pixelData.toArray(ValueLayout.JAVA_BYTE); - PR Review Comment: https://git.openjdk.org/jdk/pull/19777#discussion_r1651481722
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation
On Mon, 24 Jun 2024 18:07:11 GMT, Maurizio Cimadamore
wrote:
>> src/java.desktop/share/classes/sun/font/StrikeCache.java line 151:
>>
>>> 149:
>>> 150: @SuppressWarnings("restricted")
>>> 151: static final float getGlyphXAdvance(long ptr) {
>>
>> now, I'm not an expert of this code, but I notice that you have accessors
>> that seem to take a bare `long` instead of `MemorySegment`. Have you tried
>> pushing segments deeper in the implementation? That way I think you could
>> completely auto-generate this code using jextract. (of course what you have
>> is not bad - I'm mostly trying to see if there's a way to get there w/o all
>> these calls to `MemorySegment::reinterpret`).
>
> To be clear - I'm assuming the `ptr` parameter comes from a single native
> call. That call returns a pointer to a struct, or maybe an array of structs.
> If so, we could reinterpret right after the native call, with the right size
> (the number of structs returned by the native call), and then all the
> accessors will be just "plain" memory segment accessors, no need to
> reinterpret (as the segment will already have the correct size).
Each pointer refers to a single struct, not an array.
In this fix I am focused on not using Unsafe.
I don't want the change to go further than that.
The 'long's are retrieved/ passed around/ back down to native in lots of places
so it would turn a focused change into a much bigger change.
grep in java.desktop for "getGlyphImage" to see the tip of the iceberg.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19777#discussion_r1651455679
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation
On Mon, 24 Jun 2024 18:04:38 GMT, Maurizio Cimadamore
wrote:
>> Migrate font code from jdk.internal.misc.Unsafe to using FFM.
>> This reduces the coupling between the java.desktop module and the internals
>> of the java.base module.
>>
>> The code being changed here is not particularly performance sensitive, and
>> it is not executed in the most common cases.
>> The main impact performance-wise is a total of around 37ms in initialisation
>> costs on my x64 macbook.
>> A minimal program that just draws a string to an image - does not even put
>> up a window - runs at around 690-700ms.
>> There's variability in that number and the overall time for a JDK without
>> the change is around (660-670ms)
>> In the small test, this is the first and only use of FFM, so the one-off
>> part cost should move elsewhere when FFM starts
>> to be used earlier in the JDK itself.
>
> src/java.desktop/share/classes/sun/font/StrikeCache.java line 151:
>
>> 149:
>> 150: @SuppressWarnings("restricted")
>> 151: static final float getGlyphXAdvance(long ptr) {
>
> now, I'm not an expert of this code, but I notice that you have accessors
> that seem to take a bare `long` instead of `MemorySegment`. Have you tried
> pushing segments deeper in the implementation? That way I think you could
> completely auto-generate this code using jextract. (of course what you have
> is not bad - I'm mostly trying to see if there's a way to get there w/o all
> these calls to `MemorySegment::reinterpret`).
To be clear - I'm assuming the `ptr` parameter comes from a single native call.
That call returns a pointer to a struct, or maybe an array of structs. If so,
we could reinterpret right after the native call, with the right size (the
number of structs returned by the native call), and then all the accessors will
be just "plain" memory segment accessors, no need to reinterpret (as the
segment will already have the correct size).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19777#discussion_r1651431857
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation
On Tue, 18 Jun 2024 20:31:58 GMT, Phil Race wrote:
> Migrate font code from jdk.internal.misc.Unsafe to using FFM.
> This reduces the coupling between the java.desktop module and the internals
> of the java.base module.
>
> The code being changed here is not particularly performance sensitive, and it
> is not executed in the most common cases.
> The main impact performance-wise is a total of around 37ms in initialisation
> costs on my x64 macbook.
> A minimal program that just draws a string to an image - does not even put up
> a window - runs at around 690-700ms.
> There's variability in that number and the overall time for a JDK without the
> change is around (660-670ms)
> In the small test, this is the first and only use of FFM, so the one-off part
> cost should move elsewhere when FFM starts
> to be used earlier in the JDK itself.
src/java.desktop/share/classes/sun/font/StrikeCache.java line 151:
> 149:
> 150: @SuppressWarnings("restricted")
> 151: static final float getGlyphXAdvance(long ptr) {
now, I'm not an expert of this code, but I notice that you have accessors that
seem to take a bare `long` instead of `MemorySegment`. Have you tried pushing
segments deeper in the implementation? That way I think you could completely
auto-generate this code using jextract. (of course what you have is not bad -
I'm mostly trying to see if there's a way to get there w/o all these calls to
`MemorySegment::reinterpret`).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19777#discussion_r1651425494
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
[jdk23] Integrated: 8334509: Cancelling PageDialog does not return the same PageFormat object
On Mon, 24 Jun 2024 16:33:56 GMT, Prasanta Sadhukhan wrote: > 8334509: Cancelling PageDialog does not return the same PageFormat object This pull request has now been integrated. Changeset: fbcf6d9c Author:Prasanta Sadhukhan URL: https://git.openjdk.org/jdk/commit/fbcf6d9c4f751ca4b410d63ac78048471cad611b Stats: 67 lines in 2 files changed: 60 ins; 0 del; 7 mod 8334509: Cancelling PageDialog does not return the same PageFormat object Reviewed-by: prr Backport-of: 689cee3d0950e15e88a1f6738bfded00655dca9c - PR: https://git.openjdk.org/jdk/pull/19865
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 prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19819#pullrequestreview-2136363950
Re: [jdk23] RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object
On Mon, 24 Jun 2024 16:33:56 GMT, Prasanta Sadhukhan wrote: > 8334509: Cancelling PageDialog does not return the same PageFormat object Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19865#pullrequestreview-2136352506
[jdk23] RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object
8334509: Cancelling PageDialog does not return the same PageFormat object - Commit messages: - Backport 689cee3d0950e15e88a1f6738bfded00655dca9c Changes: https://git.openjdk.org/jdk/pull/19865/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19865=00 Issue: https://bugs.openjdk.org/browse/JDK-8334509 Stats: 67 lines in 2 files changed: 60 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19865.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19865/head:pull/19865 PR: https://git.openjdk.org/jdk/pull/19865
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 kcr (Author). - PR Review: https://git.openjdk.org/jdk/pull/19819#pullrequestreview-2136280195
Re: RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19819/files - new: https://git.openjdk.org/jdk/pull/19819/files/40312082..6844be05 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19819=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19819=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19819/head:pull/19819 PR: https://git.openjdk.org/jdk/pull/19819
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: 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 Thank you Alexey and Tejesh for your reviews! - PR Comment: https://git.openjdk.org/jdk/pull/19192#issuecomment-2186644760
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: 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
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.
Did you build the docs and check that the `@link` command works? I would have
expected it without the named parameter: `{@link #BasicSliderUI(JSlider)}`.
Also, you might want to change "next version" to "a future version" to allow
more flexibility as to the timing of the removal.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19819#discussion_r1650968421
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:17:19 GMT, Abhishek Kumar wrote: >>> 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. >> >> 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). >> >> Now I left with few options: >> 1. Install `altProcessor` handler similar to `WindowsLookAndFeel` >> implementation but that results in unnecessary installation of `alt handler >> for Nimbus L (derived from Synth L)` as well. >> 2. Add the `RootPane.altPress` condition check before installing the >> `altProcessor` handler but the value returned for `RootPane.altPress` is >> always **false** and that left with `altProcessor` handler uninstalled. >> >> So, the handler implementation is moved to `SynthRootPaneUI`. > >>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 ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650477692
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 06:21:51 GMT, Abhishek Kumar wrote: >> 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`. > >> 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. > > 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). > > Now I left with few options: > 1. Install `altProcessor` handler similar to `WindowsLookAndFeel` > implementation but that results in unnecessary installation of `alt handler > for Nimbus L (derived from Synth L)` as well. > 2. Add the `RootPane.altPress` condition check before installing the > `altProcessor` handler but the value returned for `RootPane.altPress` is > always **false** and that left with `altProcessor` handler uninstalled. > > So, the handler implementation is moved to `SynthRootPaneUI`. >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`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650472661
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 20:14:28 GMT, Alexey Ivanov wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove access modifier from method declaration
>
> 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.
>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.
Should I revert it back to javadoc style comment ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650476132
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650475455
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 20:08:58 GMT, Alexey Ivanov wrote: > 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. 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). Now I left with few options: 1. Install `altProcessor` handler similar to `WindowsLookAndFeel` implementation but that results in unnecessary installation of `alt handler for Nimbus L (derived from Synth L)` as well. 2. Add the `RootPane.altPress` condition check before installing the `altProcessor` handler but the value returned for `RootPane.altPress` is always **false** and that left with `altProcessor` handler uninstalled. So, the handler implementation is moved to `SynthRootPaneUI`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650410849
