Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8295006: Colored text is not shown on disabled checkbox and radio button with GTK LAF for bug4314194. #10755

Closed
wants to merge 6 commits into from

Conversation

kumarabhi006
Copy link
Contributor

@kumarabhi006 kumarabhi006 commented Oct 19, 2022

Existing test open/test/jdk/javax/swing/JRadioButton/4314194/bug4314194.java was not showing colored text for disabled checkbox and radiobutton in GTK LAF.

The fix is to get the disabled state color for checkbox and radiobutton from UIManager if it exists.

Test case open/test/jdk/javax/swing/JRadioButton/4314194/bug4314194.java has been checked in CI pipeline. Link is attached in JBS.


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-8295006: Colored text is not shown on disabled checkbox and radio button with GTK LAF for bug4314194.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10755

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 19, 2022

👋 Welcome back kumarabhi006! 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
Copy link

openjdk bot commented Oct 19, 2022

@kumarabhi006 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.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Oct 19, 2022
@kumarabhi006 kumarabhi006 changed the title Colored text is not shown on disabled checkbox and radiobutton with GTK LAF for bug4314194 8295006: Colored text is not shown on disabled checkbox and radiobutton with GTK LAF for bug4314194 Oct 19, 2022
@openjdk
Copy link

openjdk bot commented Oct 19, 2022

@kumarabhi006 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8295006
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Oct 19, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 19, 2022

Webrevs

@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 19, 2022
@kumarabhi006
Copy link
Contributor Author

@TejeshR13 @prsadhuk Please review the PR.

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

Why we did not get this colors from the native system? I also suggest to check an initial fix. looks like it contradicts the specification.

@kumarabhi006
Copy link
Contributor Author

Why we did not get this colors from the native system? I also suggest to check an initial fix. looks like it contradicts the specification.

The color returned from the native system is grey even though the user has explicitly set it to be some other color in GTK LAF. So, UIManager.getColor is used to retrieve the color for disabled checkbox and radiobutton if property is set by user else getColorForState will return the appropriate color.

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

I wonder do the GTK/Nimbus L&F actually use that UIDefault properties or they all based on SynthContext/SynthStyle? I see that all usages of UIManager.getColor() is added there by @prsadhuk, any reason why it was not used before?

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

For example maybe the test should uses the properties supported by the L&F? what happen if the test sets RadioButton[Disabled].textForeground to some value?

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

Something like this should work I think:

        checkBox = new JCheckBox("\u2588".repeat(5));
        UIDefaults checkBoxDefaults = new UIDefaults();
        checkBoxDefaults.put("CheckBox.textForeground", checkboxColor);
        checkBox.putClientProperty("Nimbus.Overrides",checkBoxDefaults);

        radioButton = new JRadioButton("\u2588".repeat(5));
        UIDefaults radioDefaults = new UIDefaults();
        radioDefaults.put("RadioButton.textForeground", radioButtonColor);
        radioButton.putClientProperty("Nimbus.Overrides",radioDefaults);

@kumarabhi006
Copy link
Contributor Author

I wonder do the GTK/Nimbus L&F actually use that UIDefault properties or they all based on SynthContext/SynthStyle? I see that all usages of UIManager.getColor() is added there by @prsadhuk, any reason why it was not used before?

I am not sure about it, maybe @prsadhuk can add his opinion.

@prsadhuk
Copy link
Contributor

As per https://docs.oracle.com/en/java/javase/19/docs/api/java.desktop/javax/swing/plaf/nimbus/package-summary.html
it seems UIDefaults (ie UIManager.getDefaults) should be honoured first which can be overridden by altering the table via UIManager.put. There's also client property named "Nimbus.Overrides" which override the UIManager settings, so as I interpret it the UIManager.getDefaults() should be honoured unless it is overridden by Nimbus.Overrides which was not the case in my fix.

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

That text mention the Nimbus properties, which are different from the properties used in the test.
The next line should change the color globally:
UIManager.getDefaults().put("CheckBox[Enabled].textForeground", Color.RED);
But it is unclear why the next does not work
UIManager.getDefaults().put("CheckBox[Disabled].textForeground", Color.GREEN);

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

It is interesting that the next line works fine in 11 and 19, but does not work in 20.
UIManager.getDefaults().put("CheckBox.textForeground", Color.RED);
I think that previouse fixes should be updated.

@mrserb
Copy link
Member

mrserb commented Oct 19, 2022

The next line is also does not work in 20
UIManager.getDefaults().put("CheckBox[Enabled].textForeground", Color.GREEN);

@kumarabhi006
Copy link
Contributor Author

@mrserb I think what you are suggesting is for Nimbus LAF. In case of GTK LAF, as I mentioned earlier that native system returned gray color for disabled checkbox and radio button.

As per my understanding I think user defined color should have precedence over native system color.
What should I do?

@kumarabhi006 kumarabhi006 changed the title 8295006: Colored text is not shown on disabled checkbox and radiobutton with GTK LAF for bug4314194 8295006: Colored text is not shown on disabled checkbox and radio button with GTK LAF for bug4314194. Oct 20, 2022
@mrserb
Copy link
Member

mrserb commented Oct 20, 2022

The current change mostly replicates the previous fix in Nimbus L&F. Since that fix is questionable we should rethink it first. The open questions are:

  • Why did the Nimbus properties above stop working in 20
  • Why the "[Disabled]" does not work as expected
  • Why Nimbus should honor "CheckBox.disabledText" property and not these properties

After that we can answer the next question: "what property GTK L&F should honor". The GTK implementation is based on SynthContext/SynthStyle so I assume it should use the same props as Nimbus, and provide a way to override it by the user when needed(same as Nimbus).

@mrserb
Copy link
Member

mrserb commented Oct 27, 2022

The similar fix #9900, as in this one we should check is it necessary to support the property expected by the test or the property from the "Nimbus list" above should be supported.

Copy link
Contributor

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"test/jdk/javax/swing/JRadioButton/4314194/bug4314194.java has been checked"

I hope you ran ALL Swing tests .. not just this single one .. ??

@openjdk
Copy link

openjdk bot commented Nov 3, 2022

@kumarabhi006 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:

8295006: Colored text is not shown on disabled checkbox and radio button with GTK LAF for bug4314194.

Reviewed-by: prr, tr

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 290 new commits pushed to the master branch:

  • f05bfb1: 8297515: serialVersionUID fields are not annotated with @serial
  • 22f5d01: 8252584: HotSpot Style Guide should permit alignas
  • 8ffed34: 8297681: Unnecessary color conversion during 4BYTE_ABGR_PRE to INT_ARGB_PRE blit
  • abe532a: 8296924: C2: assert(is_valid_AArch64_address(dest.target())) failed: bad address
  • 5dcaf6c: 8297749: Remove duplicate space in the ProtocolException message being thrown from HttpURLConnection
  • c7a679f: 8297290: Use int indices to reference CDS archived primitive mirrors
  • 37f613b: 8297676: DataBuffer.TYPE_SHORT/TYPE_FLOAT/TYPE_DOUBLE are not placeholders
  • 87f00f4: 8296878: Document Filter attached to JPasswordField and setText("") is not cleared instead inserted characters replaced with unicode null characters
  • 9ced2ea: 8297311: Improve exception message thrown by java.net.HostPortrange::toLowerCase(String s)
  • defe060: 8296905: Replace the native LCMS#getProfileID() method with the accessor
  • ... and 280 more: https://git.openjdk.org/jdk/compare/956d75bcc0a358b7ff6d7ea7eb501d789096e518...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace, @TejeshR13) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2022
@kumarabhi006
Copy link
Contributor Author

"test/jdk/javax/swing/JRadioButton/4314194/bug4314194.java has been checked"

I hope you ran ALL Swing tests .. not just this single one .. ??

Yeah, I ran all swing tests.

@mrserb
Copy link
Member

mrserb commented Nov 11, 2022

Does anybody look into the regression in jdk20 reported above? The fork will be done soon, it is better to fix it early than later, if the fix is not known it is probably better to roll back some changes.

Instead of fixing the product due to the test expectations, the test should be updated to use the correct properties for Nimbus and GTK.

@prsadhuk
Copy link
Contributor

prsadhuk commented Nov 11, 2022

Instead of fixing the product due to the test expectations, the test should be updated to use the correct properties for Nimbus and GTK.

If you believe it's test issue, can you please raise a PR updating the test to what it should have using the "correct properties"?
We can then roll back the product changes if need be or you also can rollback in the same PR the changes you think are incorrect

@kumarabhi006
Copy link
Contributor Author

@prrace Just a minor change in condition check. Updated to bitwise operator to check if component is disabled as it has been done for other widget.
CI testing looks fine.

@TejeshR13
Copy link
Contributor

Fix is tested and working fine.

@mrserb
Copy link
Member

mrserb commented Nov 30, 2022

If you believe it's test issue, can you please raise a PR updating the test to what it should have using the "correct properties"?
We can then roll back the product changes if need be or you also can rollback in the same PR the changes you think are incorrect

I will update the test case and also will file a bug to fix the usage of the Synth properties.

@kumarabhi006
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 30, 2022
@openjdk
Copy link

openjdk bot commented Nov 30, 2022

@kumarabhi006
Your change (at version 81dc776) is now ready to be sponsored by a Committer.

@jayathirthrao
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Dec 1, 2022

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 1, 2022
@openjdk openjdk bot closed this Dec 1, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 1, 2022
@openjdk
Copy link

openjdk bot commented Dec 1, 2022

@jayathirthrao @kumarabhi006 Pushed as commit ce048e7.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
6 participants