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 Yes, it doesn't change any existing behaviour, it's just a more robust way of expressing what was already there before, so it should be harmless. I'm not sure I follow your other concern about it being a mixture though, I checked and this method is static and only used in this file, and the few callsites that do use it all initialize their arrays to KB_STATE_SIZE. Do you mean getting rid of the hardcoded 256 in this method's array parameter? The other thing about passing an array by reference means if the caller doesn't get the array size exactly right, it will not result in a silent bug, it is an immediate compilation error, so if the caller passes a shorter array it will simply refuse to compile, which is likely what we want here - PR Comment: https://git.openjdk.org/jdk/pull/19798#issuecomment-2181982129
RFR: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
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 - Commit messages: - -8334580: Deprecate no-arg constructor BasicSliderUI() for removal - -8334580: Deprecate no-arg constructor BasicSliderUI() for removal Changes: https://git.openjdk.org/jdk/pull/19819/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19819=00 Issue: https://bugs.openjdk.org/browse/JDK-8334580 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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 [v2]
On Thu, 20 Jun 2024 20:04:47 GMT, Phil Race wrote: >> 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. > > 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? - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1648364406
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows make/common/native/Link.gmk line 72: > 70: define CreateStaticLibrary > 71: # Include partial linking when building the static library with clang > on linux > 72: ifeq ($(call isTargetOs, linux macosx), true) Is this mainly for `clang` support for now? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648293391
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows make/modules/java.desktop/lib/AwtLibraries.gmk line 257: > 255: JDK_LIBS := libawt java.base:libjava, \ > 256: LIBS_unix := $(LIBDL) $(LIBM) $(X_LIBS) -lX11 -lXext -lXi > -lXrender \ > 257: -lXtst, \ Same question as for the STATIC_LIB_EXCLUDE_OBJS change with `libjli`. Are the duplicate symbol issues resolved by symbol hiding? I think it's still better to not include those .o files to avoid unnecessary footprint overhead in the binary. - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648292220
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows make/modules/java.base/lib/CoreLibraries.gmk line 148: > 146: $(LIBJLI_EXTRA_FILE_LIST)) > 147: > 148: # Do not include these libz objects in the static libjli library. Why this is no longer needed? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648290693
Re: RFR: 8333268: Fixes for static build [v2]
On Tue, 18 Jun 2024 17:57:29 GMT, Magnus Ihse Bursie wrote:
>> Magnus Ihse Bursie 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 seven
>> additional commits since the last revision:
>>
>> - Merge branch 'master' into static-linking-progress
>> - Merge branch 'master' into static-linking-progress
>> - Move the exported JVM_IsStaticallyLinked to a better location
>> - Use runtime lookup of static vs dynamic instead of #ifdef STATIC_BUILD
>> - Copy fix for init_system_properties_values on linux
>> - Make sure we do not try to build static libraries on Windows
>> - 8333268: Fixes for static build
>
> src/hotspot/os/linux/os_linux.cpp line 605:
>
>> 603:
>> 604: // Get rid of /{client|server|hotspot}, if binary is libjvm.so.
>> 605: // Or, cut off /.
>
> @jianglizhou This code is based on changes in the Hermetic Java repo, but I
> do not fully understand neither the comment nor what the purpose is. If you
> could explain this a bit I'd be grateful.
The specific related commit in the hermetic Java branch is
https://github.com/openjdk/leyden/commit/53aa8f0cf418ab5f435a4b9996c7754fb8505d4b.
The change in os_linux.cpp here is to make sure that the libjvm.so related
path manipulation is conditionally done only. The check at line 599 looks for
"/libjvm.so" substring, so we only chop off (`*pslash = `\0` at line 601) that
part when necessary. In the static JDK case, there is no `libjvm.so` and the
path string is `/bin/javastatic`, which should not be affected.
Otherwise, it could fail.
I found the code was not very easy to follow when running into problems and
fixing for static support. So I added a bit more comments in the code here. The
comment above about `/{client|server|hotspot}` was there originally. I think we
no longer have those directories. We can cleanup that later, since it needs
some more testing.
@magicus, thanks a lot for extracting/reworking/cleaning-up the static Java
changes from the hermetic Java branch. That's a substantial amount of work!
I have one quick comment about the removal of `STATIC_LIB_EXCLUDE_OBJS`
changes. Will post it as separate comment for the related code.
I'll also look closely of the vm & jdk changes and compare those with the
changes in the hermetic Java branch this week.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648283151
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 13:15:00 GMT, Abhishek Kumar wrote: >>> Hmm. So .. new public API ? Is this absolutely necessary ? >> I don't see why an app would need to call this directly. >> >> `setMnemonicHidden` can be changed to a `protected` member as you pointed >> out it may not be required by an app to call this method. >> But I guess the `isMnemonicHidden` should be public API. >> >>> And it would need a CSR, and it is too late for 23 anyway. >> >> Will update the `@since to 24` for `isMnemonicHidden` method if we are ok >> with `isMnemonicHidden` method's access modifier. > > Infact `isMnemonicHidden` can also be changed to a `protected` member of the > class. I will check and update. protected members of public classes are part of the API Go look at javadoc - or generate javadoc for this change and see for yourself. So this still requires a CSR as written. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1648120126
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 Harmless, I hope, but it doesn't change that 256 is hard-coded in multiple places AND that the caller has to get it right too .. if the caller passes a shorter array things can still go wrong. Why can't we EVERYWHERE use the definition used to create the array passed in KB_STATE_SIZE = 256 instead of a mixture. - PR Review: https://git.openjdk.org/jdk/pull/19798#pullrequestreview-2131336310
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 07:29:45 GMT, Tejesh R wrote: >> DetailsView removes JTable TAB, SHIFT-TAB, ENTER and SHIFT-ENTER >> functionalities to disable navigation within JTable of FilePane in >> DetailsView. This is causing the issue mentioned in the bug where on >> invoking DetailsView the functionalities are removed from JTable. I don't >> see it's effect/significance in disabling the navigation since the focus >> shifts outside the when TAB is presses and the folder opens when ENTER is >> pressed without this changed. >> I have tested the fix on CI system, its green. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review fix - remove null initialization for table I think it is clear that modifying a shared map is a bad idea, so removing the code that is doing this is right. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19725#pullrequestreview-2131313263
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation
On Tue, 18 Jun 2024 22:01:26 GMT, Alisen Chung wrote: > How do I test this fix? There isn't a specific test because it is code that is used internally in somewhat random cases. A number of automated tests will use it (and they do all pass) and just running a UI app will sometimes exercise this code. Running SwingSet on Linux would be a reasonable thing to do. - PR Comment: https://git.openjdk.org/jdk/pull/19777#issuecomment-2181492032
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 10:39:48 GMT, Alexey Ivanov wrote: >> 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. The way it was "before" is that we always returned the value of "doIt". Why not restore that for consistency ? >> 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. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1648070159 PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1648068766
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: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Tue, 18 Jun 2024 06:21:12 GMT, Tejesh R wrote: >> Yes, modifying the shared `ActionMap` is causing this issue though. As far >> as I have seen the copy first solution is mentioned in this bug >> https://bugs.openjdk.org/browse/JDK-8166352 as customer submitted workaround. > > I did thought of few solutions for this issue: > 1. To reset (If possible, but not sure how to do this, yet we have > `SwingUtilities.replaceUIActionMap`) the ActionMap. But when to reset is > again a question? > 2. To consider what customer has suggested about making a copy and then using > that which again I'm not sure since here shared defaults are used from > BasicTableUI. > 3. To remove the lines causing issue which I have proposed. I feel it is safe > now to remove it since TAB/ENTER functionalities (Basically TAB being moved > out of FilePane and ENTER on selecting file/opening Directory) is handled > without these lines too. I did CI test for any regression, but its look fine > without this lines too. TAB navigation is prevented by this fix https://bugs.openjdk.org/browse/JDK-4835633 where the TAB is rejected in [this](https://github.com/openjdk/jdk/blame/a81e1bf1e1a6f00280b9be987c03fe20915fd52c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java#L683) line. - PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1647807264
Integrated: 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. This pull request has now been integrated. Changeset: 9ef86da5 Author:Abhishek Kumar URL: https://git.openjdk.org/jdk/commit/9ef86da5f8e2579fa1fdf40b4a6f556882e1177d Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4 Reviewed-by: aivanov, azvegint - PR: https://git.openjdk.org/jdk/pull/19788
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: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows The changes to the launcher look okay. The move from `ifdef STATIC_BUILD` to `JLI_IsStaticallyLinked` is quite nice. Having libjdwp link to libjvm was a surprise but I think okay. I think it would be useful to provide a brief summary on the when/where the static builds are tested to ensure that the changes don't bit rot. I realise we already have static builds but it isn't obvious where this is tested. src/hotspot/share/runtime/linkType.cpp line 36: > 34: return JNI_TRUE; > 35: #else > 36: return JNI_FALSE; bool != jboolean, I assume you don't want that. The naming is a bit unusual, a function that returns a boolean is usually name is_XXX, but maybe there is reason for this? - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2180635747 PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1647480992
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Tue, 18 Jun 2024 21:48:54 GMT, Alisen Chung wrote: > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/19740#discussion_r1647405586
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: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 20:21:16 GMT, Phil Race wrote:
>> 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/SynthGraphicsUtils.java
> line 780:
>
>> 778: }
>> 779:
>> 780: if (c instanceof JLabel && ((JLabel)
>> c).getDisplayedMnemonic() != '\0') {
>
> you can use else if in all these cases
Updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1647252426
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 16:28:50 GMT, Alexey Ivanov wrote: >> 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/SynthRootPaneUI.java > line 115: > >> 113: KeyboardFocusManager.getCurrentKeyboardFocusManager(). >> 114: addKeyEventPostProcessor(altProcessor); >> 115: } > > Can this lead to installing `altProcessor` twice or more? Added flag variable to check of it is installed or not. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1647253418
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v7]
> 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 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 eight additional commits since the last revision: - Remove public API and test update - Merge - condition update - Review comment fix - condition check update - Alt press property added for GTK L - Var moved to local scope - Mnemonic toggle fix - Changes: - all: https://git.openjdk.org/jdk/pull/18992/files - new: https://git.openjdk.org/jdk/pull/18992/files/47d4c18c..dc9465ac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18992=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18992=05-06 Stats: 265947 lines in 5365 files changed: 151720 ins; 84798 del; 29429 mod Patch: https://git.openjdk.org/jdk/pull/18992.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18992/head:pull/18992 PR: https://git.openjdk.org/jdk/pull/18992
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows Build changes look ok. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19478#pullrequestreview-2129807207
Re: RFR: 8302671: libawt has a memmove decay error
On Fri, 17 Feb 2023 14:35:45 GMT, Laurent Bourgès wrote: > Or use #define KBD_BUF_LEN 256 And use it explicitely in array init & > memmove... as plain old C. Turns out C++ has an even more elegant solution for this: Passing an array by reference https://github.com/openjdk/jdk/pull/19798/files - PR Comment: https://git.openjdk.org/jdk/pull/12597#issuecomment-2180125172
RFR: 8334599: Improve code from JDK-8302671
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 - Commit messages: - 8334599 Changes: https://git.openjdk.org/jdk/pull/19798/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19798=00 Issue: https://bugs.openjdk.org/browse/JDK-8334599 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19798.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19798/head:pull/19798 PR: https://git.openjdk.org/jdk/pull/19798
Re: RFR: 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4
On Wed, 19 Jun 2024 19:35:54 GMT, Alexey Ivanov wrote: > Would it be clearer if setDelay(50) was called in the constructor of > bug6492108? I am ok with the current placing of setDelay(50). - PR Review Comment: https://git.openjdk.org/jdk/pull/19788#discussion_r1647193228
