8300084: AquaFileChooserUI.getDefaultButton returns null #12008

TejeshR13 · 2023-01-16T11:17:10Z

In Aqua look and feel, getDefaultButton() of JFileChooser returns null. The fix overrides the method from FileChooserUI to return the ApproveButton of FileChooser which is similar to other L&F.
The test is run for multiple iterations in CI machine.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8300084: AquaFileChooserUI.getDefaultButton returns null

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12008/head:pull/12008
$ git checkout pull/12008

Update a local copy of the PR:
$ git checkout pull/12008
$ git pull https://git.openjdk.org/jdk pull/12008/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12008

View PR using the GUI difftool:
$ git pr show -t 12008

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12008.diff

bridgekeeper · 2023-01-16T11:18:56Z

👋 Welcome back tr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-16T11:22:23Z

@TejeshR13 The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-16T11:25:56Z

Webrevs

aivanov-jdk

Looks good to me.

I doubt a separate test for this issue is worth it. This case is entirely covered by the test in #11901 for JDK-8299522, which also ensures the button text isn't empty.

Adding 8300084 to the @bug tag in that test is enough. It was my original suggestion:

Now you can fix this bug so that Aqua L&F doesn't return null and add that bugid to this test.

aivanov-jdk · 2023-01-16T11:33:39Z

test/jdk/javax/swing/JFileChooser/AquaDefaultButtonTest.java

+                JFileChooser fileChooser = new JFileChooser();
+                JButton defaultApproveButton = fileChooser.getUI().getDefaultButton(fileChooser);
+                if (defaultApproveButton == null) {
+                    throw new RuntimeException("getDefaultButton() method returns null for Aqua L&F!");
+                }


If you choose shorter names for variables, the line would fit into 80-column limit. Now the two lines are nearly 100 characters long, though not a big issue.

Could you please the copyright year in AquaFileChooserUI.java?

openjdk · 2023-01-16T11:41:57Z

@TejeshR13 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8300084: AquaFileChooserUI.getDefaultButton returns null

Reviewed-by: aivanov, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 228 new commits pushed to the master branch:

506c818: 8296401: ConcurrentHashTable::bulk_delete might miss to delete some objects
319de6a: 8300124: Remove unnecessary assert in GenCollectedHeap::initialize
289aed4: 8298128: runtime/ErrorHandling/TestSigInfoInHsErrFile.java fails to find pattern with slowdebug
a2f6766: 8288415: java/awt/PopupMenu/PopupMenuLocation.java is unstable in MacOS machines
a734285: 8299879: CollectedHeap hierarchy should use override
98d75f1: 8299038: Add AArch64 backend support for auto-vectorized FP16 conversions
cac72a6: 8300053: Shenandoah: Handle more GCCauses in ShenandoahControlThread::request_gc
6fea233: 8299960: Speed up test/hotspot/jtreg/compiler/c2/irTests/TestVectorizeURShiftSubword.java
7c1ebcc: 8299962: Speed up compiler/intrinsics/unsafe/DirectByteBufferTest.java and HeapByteBufferTest.java
abfd7e8: 8300113: C2: Single-bit fields with signed type in TypePtr after JDK-8297933
... and 218 more: https://git.openjdk.org/jdk/compare/a3693ccc617d06137a61050b34646e8a90ed3d7e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

TejeshR13 · 2023-01-16T12:53:06Z

Looks good to me.

I doubt a separate test for this issue is worth it. This case is entirely covered by the test in #11901 for JDK-8299522, which also ensures the button text isn't empty.

Adding 8300084 to the @bug tag in that test is enough. It was my original suggestion:

Now you can fix this bug so that Aqua L&F doesn't return null and add that bugid to this test.

Yeah, but since #11901 handles only Approve button size except for Aqua L&F and this fix is only for Aqua L&F I thought it would be better to not to maintain any dependency between the test/bug (Though this bug was initiated as part of #11901 fix). I hope maintaining simple test case for two bug would be fine.

aivanov-jdk · 2023-01-16T13:33:04Z

This case is entirely covered by the test in #11901 for JDK-8299522, which also ensures the button text isn't empty.
Adding 8300084 to the @bug tag in that test is enough. It was my original suggestion:

Now you can fix this bug so that Aqua L&F doesn't return null and add that bugid to this test.

Yeah, but since #11901 handles only Approve button size except for Aqua L&F and this fix is only for Aqua L&F I thought it would be better to not to maintain any dependency between the test/bug (Though this bug was initiated as part of #11901 fix). I hope maintaining simple test case for two bug would be fine.

It's exactly the point: include Aqua L&F in #11901. The Aqua L&F is excluded only because getDefaultButton returns null, isn't it? This fix makes it return non-null, therefore Aqua L&F can be included in testing.

This fix should be integrated before JDK-8299522. I will not approve #11901 until Aqua L&F is part of the CustomApproveButtonTest.java test.

TejeshR13 · 2023-01-16T13:38:32Z

This case is entirely covered by the test in #11901 for JDK-8299522, which also ensures the button text isn't empty.
Adding 8300084 to the @bug tag in that test is enough. It was my original suggestion:

Now you can fix this bug so that Aqua L&F doesn't return null and add that bugid to this test.

Yeah, but since #11901 handles only Approve button size except for Aqua L&F and this fix is only for Aqua L&F I thought it would be better to not to maintain any dependency between the test/bug (Though this bug was initiated as part of #11901 fix). I hope maintaining simple test case for two bug would be fine.

It's exactly the point: include Aqua L&F in #11901. The Aqua L&F is excluded only because getDefaultButton returns null, isn't it? This fix makes it return non-null, therefore Aqua L&F can be included in testing.

This fix should be integrated before JDK-8299522. I will not approve #11901 until Aqua L&F is part of the CustomApproveButtonTest.java test.
The Aqua L&F is excluded not only because getDefaultButton returns null, but it is also excluded because the fix doesn't apply to Aqua L&F. Still if u insist to use the same test then can do that, as u said first this fix will go in and then #11901.

aivanov-jdk · 2023-01-16T13:40:22Z

test/jdk/javax/swing/JFileChooser/AquaDefaultButtonTest.java

+                JButton defApproveBtn = fc.getUI().getDefaultButton(fc);
+                if (defApproveBtn == null) {


Suggested change

JButton defApproveBtn = fc.getUI().getDefaultButton(fc);

if (defApproveBtn == null) {

JButton approve = fc.getUI().getDefaultButton(fc);

if (approve == null) {

On the scope of two lines, its type — JButton — as well as method name makes it obvious it's a button.

approveButton would also be good.

Yet I am for removing this test completely because it's functionality is entirely covered by the CustomApproveButtonTest.java in #11901.

Ok, then I'll remove this test and update #11901.

aivanov-jdk · 2023-01-16T13:45:28Z

It's exactly the point: include Aqua L&F in #11901. The Aqua L&F is excluded only because getDefaultButton returns null, isn't it? This fix makes it return non-null, therefore Aqua L&F can be included in testing.

This fix should be integrated before JDK-8299522. I will not approve #11901 until Aqua L&F is part of the CustomApproveButtonTest.java test.

The Aqua L&F is excluded not only because getDefaultButton returns null, but it is also excluded because the fix doesn't apply to Aqua L&F. Still if u insist to use the same test then can do that, as u said first this fix will go in and then #11901.

I do insist.

The fix does not apply to Aqua L&F only because this situation has already be taken care of in Aqua L&F. Now all L&Fs are expected to return non-null text for the Approve button, I see no reason why Aqua L&F is skipped in testing: it is still expected that the Approve button in Aqua L&F is not null.

aivanov-jdk

Looks good to me.

TejeshR13 · 2023-01-17T04:42:11Z

/integrate

openjdk · 2023-01-17T04:44:52Z

Going to push as commit 8365c67.
Since your change was applied there have been 235 commits pushed to the master branch:

382fe51: 8163229: several regression tests have a main method that is never executed
06f9374: 8298867: Basics.java fails with SSL handshake exception
859ccd1: 8299847: RISC-V: Improve PrintOptoAssembly output of CMoveI/L nodes
240830d: 8297092: [macos_aarch64] Add support for SHA feature detection
f52f6e6: 8299972: Remove metaprogramming/removeReference.hpp
4c1e66e: 8299971: Remove metaprogramming/conditional.hpp
4073b68: 8298047: Remove all non-significant trailing whitespace from properties files
506c818: 8296401: ConcurrentHashTable::bulk_delete might miss to delete some objects
319de6a: 8300124: Remove unnecessary assert in GenCollectedHeap::initialize
289aed4: 8298128: runtime/ErrorHandling/TestSigInfoInHsErrFile.java fails to find pattern with slowdebug
... and 225 more: https://git.openjdk.org/jdk/compare/a3693ccc617d06137a61050b34646e8a90ed3d7e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-17T04:45:08Z

@TejeshR13 Pushed as commit 8365c67.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

fix + test

59a7963

openjdk bot added the rfr Pull request is ready for review label Jan 16, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Jan 16, 2023

aivanov-jdk approved these changes Jan 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2023

Updated based on review comments

81d2cda

aivanov-jdk suggested changes Jan 16, 2023

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Jan 16, 2023

Removed the test - Updating openjdk#11901 test

603fd66

TejeshR13 added a commit to TejeshR13/jdk that referenced this pull request Jan 16, 2023

Added Aqua L&F - linked to openjdk#12008 fix

6ed23ab

aivanov-jdk approved these changes Jan 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2023

mrserb approved these changes Jan 16, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 17, 2023

openjdk bot closed this Jan 17, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 17, 2023

aivanov-jdk mentioned this pull request Jan 18, 2023

8299522: Incorrect size of Approve button in custom JFileChooser #11901

Closed

3 tasks

		JButton defApproveBtn = fc.getUI().getDefaultButton(fc);
		if (defApproveBtn == null) {

8300084: AquaFileChooserUI.getDefaultButton returns null #12008

8300084: AquaFileChooserUI.getDefaultButton returns null #12008

Uh oh!

Conversation

TejeshR13 commented Jan 16, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 16, 2023

Uh oh!

openjdk bot commented Jan 16, 2023

Uh oh!

mlbridge bot commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

TejeshR13 Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TejeshR13 commented Jan 16, 2023

Uh oh!

aivanov-jdk commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TejeshR13 commented Jan 16, 2023

Uh oh!

aivanov-jdk Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

TejeshR13 Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Jan 16, 2023

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

TejeshR13 commented Jan 17, 2023

Uh oh!

openjdk bot commented Jan 17, 2023

Uh oh!

openjdk bot commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

TejeshR13 commented Jan 16, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 16, 2023 •

edited

Loading

openjdk bot commented Jan 16, 2023 •

edited

Loading

aivanov-jdk commented Jan 16, 2023 •

edited

Loading