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

8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI #3167

Closed
wants to merge 6 commits into from

Conversation

trebari
Copy link
Member

@trebari trebari commented Mar 24, 2021

Hi All,
Please review the following fix for jdk17.

Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
installProperty should work as the opaque property is not set by the client.

Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.

Test : Added a test to check the same. Also tested internal tests which all are passing.
Link is in JBS.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3167/head:pull/3167
$ git checkout pull/3167

Update a local copy of the PR:
$ git checkout pull/3167
$ git pull https://git.openjdk.java.net/jdk pull/3167/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3167

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3167.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 24, 2021

👋 Welcome back trebari! 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.

@trebari trebari marked this pull request as ready for review March 24, 2021 06:18
@openjdk
Copy link

openjdk bot commented Mar 24, 2021

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

  • swing

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 swing client-libs-dev@openjdk.org rfr Pull request is ready for review labels Mar 24, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 24, 2021

Webrevs

Copy link
Contributor

@prsadhuk prsadhuk left a comment

Choose a reason for hiding this comment

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

I am not sure if this is the correct fix, as then we would have to remove setOpaque call for JTree, JToolTip, JRootPane etc where we might face same installProperty issue.
Also, if we have to change this, we then need to change "autoScrolls" property too which is set in this 2 component and setUIProperty() can change those too.
Probably, we need to check if the value to be set is different in setUIProperty and then set it
or
override setUIProperty per component basis (as it is done for JTable) and then not check for OPAQUE_SET flag for opaque property accordingly.

@trebari
Copy link
Member Author

trebari commented Mar 25, 2021

I am not sure if this is the correct fix, as then we would have to remove setOpaque call for JTree, JToolTip, JRootPane etc where we might face same installProperty issue.

Yes we face the same issue in JTree, JToolTip, and JViewport ,
but many of the components do not have this problem like JButton JRootPane , JCheckbox, JInternalFrame, JMenuItem, JCheckBoxMenuItem, JPopupMenu, JMenu ..etc
We can fix the issue with JTree, JTooltip and JViewport same way.

Also, if we have to change this, we then need to change "autoScrolls" property too which is set in this 2 component and setUIProperty() can change those too.
Probably, we need to check if the value to be set is different in setUIProperty and then set it
or
override setUIProperty per component basis (as it is done for JTable) and then not check for OPAQUE_SET flag for opaque property accordingly.

OPAQUE_SET is private variable and can not be accessed in JTable or JTree.
From the spec of LookAndFeel.installProperty and setUIProperty it

  • Sets the property with the specified name to the specified value if
    • the property has not already been set by the client program.*
      is clear that once the property is set by the client setUIProperty can not change it. And OPAUQE_SET is used for that
      .

Removing the setOpaque() from the constructor of these component will not make any initial difference as it is also set to true in BasicListUI, BasicTableUI, BasicTreeUI, BasicTooltipUI and BasicViewportUI by calling LookAndFeel.installProperty.
The difference which it will make is now by calling LookAndFeel.installProperty(c,opaque,false) will clear the opaque property.

@trebari
Copy link
Member Author

trebari commented Mar 31, 2021

@prsadhuk @mrserb is there any more comments on this.

Copy link
Member

@mrserb mrserb left a comment

Choose a reason for hiding this comment

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

Please wait for the @prsadhuk approval as well.

@openjdk
Copy link

openjdk bot commented Mar 31, 2021

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

8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Reviewed-by: psadhukhan, 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 189 new commits pushed to the master branch:

  • 7f9ece2: 8264650: Cross-compilation to macos/aarch64
  • 0039c18: 8264475: CopyArea ignores clip state in metal rendering pipeline
  • f084bd2: 8262355: Support for AVX-512 opmask register allocation.
  • 0780666: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier
  • f259eea: 8264393: JDK-8258284 introduced dangling TLH race
  • 9b2232b: 8264123: add ThreadsList.is_valid() support
  • e8eda65: 8264664: use text blocks in javac module tests
  • cec66cf: 8264572: ForkJoinPool.getCommonPoolParallelism() reports always 1
  • 9c283da: 8264662: ProblemList vmTestbase/jit/escape/AdaptiveBlocking/AdaptiveBlocking001/AdaptiveBlocking001.java on win-x64 with ZGC
  • eb0ac86: 8264655: Minor internal doc comment cleanup
  • ... and 179 more: https://git.openjdk.java.net/jdk/compare/fd3a33a85667eec10f5a41dd7ffd7b1e2e410141...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 31, 2021
@trebari
Copy link
Member Author

trebari commented Apr 1, 2021

I have fixed the issue with JTree, JTooltip and JViewport same way.
While fixing for JTooltip discovered that default value for JToolTip opaque property for Nimbus LookAndFeel is set to false in the skin.laf file. This value was never taken into account because the setOpaque(true) from the JTooltip was overriding this.
As we are now removing it from the constructor of JTooptip, NimbusLAF is setting to the false.
To keep the behaviour same as before i have set it to true for NimbusLAF.

Also modified the test to check for JTooltip , JTree and JViewport.

@trebari
Copy link
Member Author

trebari commented Apr 2, 2021

Internal testing looks good, link is in JBS

@@ -27248,7 +27248,7 @@
<borderStates/>
<regions/>
</uiComponent>
<uiComponent opaque="false" type="javax.swing.JToolTip" name="ToolTip" ui="ToolTipUI" subregion="false">
<uiComponent opaque="true" type="javax.swing.JToolTip" name="ToolTip" ui="ToolTipUI" subregion="false">
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, instead of changing skin.laf we probably should update SynthToolTipUI.installDefaults() and add
LookAndFeel.installProperty(c, "opaque", Boolean.TRUE);

similar to what we do for BasicToolTipUI.installDefaults. Will it not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't work, i have checked.
The change needs to be skin.laf file only.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure where you placed the code. I think it's need to be added after updateStyle() as updateStyle() uninstall the default and install for new style.

protected void installDefaults(JComponent c) {
        updateStyle(c);
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah setting the property after updateStyle(c) works fine.
updating the change in the SynthToolTipUI.installDefaults()

Copy link
Member

Choose a reason for hiding this comment

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

What is the difference from setting it via "skin.laf"? The "skin.laf" is converted to the NimbusDefaults during the build and these defaults are used by the NimbusLookAndFeel.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally feel setting in code will be easier to maintain going forward than to go through property file settings.

Copy link
Member

Choose a reason for hiding this comment

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

Then why we need this file at all? Now we will set the false value in the config, and true at runtime, what is the profit?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should set it in the config file since this is the place where all other properties are stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before this fix, I see it was set in property file and then overridden in JToolTip. Current fix does the same, set in property file and then overridden in SynthToolTipUI. But I don't have objection if it is only controlled by property file.

test/jdk/javax/swing/JList/TestOpaqueListTable.java Outdated Show resolved Hide resolved
@trebari
Copy link
Member Author

trebari commented Apr 5, 2021

/integrate

@openjdk openjdk bot closed this Apr 5, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 5, 2021
@openjdk
Copy link

openjdk bot commented Apr 5, 2021

@trebari Since your change was applied there have been 190 commits pushed to the master branch:

  • a8005ef: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
  • 7f9ece2: 8264650: Cross-compilation to macos/aarch64
  • 0039c18: 8264475: CopyArea ignores clip state in metal rendering pipeline
  • f084bd2: 8262355: Support for AVX-512 opmask register allocation.
  • 0780666: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier
  • f259eea: 8264393: JDK-8258284 introduced dangling TLH race
  • 9b2232b: 8264123: add ThreadsList.is_valid() support
  • e8eda65: 8264664: use text blocks in javac module tests
  • cec66cf: 8264572: ForkJoinPool.getCommonPoolParallelism() reports always 1
  • 9c283da: 8264662: ProblemList vmTestbase/jit/escape/AdaptiveBlocking/AdaptiveBlocking001/AdaptiveBlocking001.java on win-x64 with ZGC
  • ... and 180 more: https://git.openjdk.java.net/jdk/compare/fd3a33a85667eec10f5a41dd7ffd7b1e2e410141...master

Your commit was automatically rebased without conflicts.

Pushed as commit 39719da.

💡 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
integrated Pull request has been integrated swing client-libs-dev@openjdk.org
3 participants