8321140: Add comment to note difference in Metal's JButton margins #20482

DamonGuy · 2024-08-06T23:12:44Z

Previously in JDK-8282772, there was a fix for JButtons with HTML content alignment for non-MacOS L&Fs. This fix was to make other L&Fs in line with the Aqua fix found in JDK-8015854.

However, the non-MacOS L&F fix caused a regression and the details and original discussion can be found in JDK-8318590. This non-MacOS L&F fix was backed out and the current behavior for JButtons with HTML content for these L&Fs was deemed to be correct as is.

Through discussion, a note about Metal's different margin values should be added somewhere for future reference. Metal's horizontal margins are 14 where other L&Fs are zero or some small value. This discrepancy is what caused the original issue to be filed. This change is to add a comment to MetalLookAndFeel.java since there is no exact spot where these values are set for the margin. It's been added where other Metal components (CheckBox, RadioButton, etc.) have comments on margins as well.

Here are some L&F values for reference/comparison:

Metal's margins: [top=2,left=14,bottom=2,right=14]
Aqua's margins: [top=0,left=2,bottom=0,right=2]
Motif's margins: [top=2,left=4,bottom=2,right=4]
Nimbus's margins: [top=0,left=0,bottom=0,right=0]

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-8321140: Add comment to note difference in Metal's JButton margins (Bug - P4)

Reviewers

Harshitha Onkar (@honkar-jdk - Reviewer)
Alexey Ivanov (@aivanov-jdk - Reviewer) 🔄 Re-review required (review applies to a16b29be)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20482

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-06T23:13:22Z

👋 Welcome back dnguyen! 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 · 2024-08-06T23:14:29Z

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

8321140: Add comment to note difference in Metal's JButton margins

Reviewed-by: honkar, aivanov

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

3ca359a: 8335771: Improve stability of java/nio/channels/DatagramChannel tests
2766b09: 8338452: (dc) DatagramChannelImpl.blockingReceive with timeout may block indefinitely if all datagrams blocked by SecurityManager
f0fe313: 8338564: Remove obsolete AbstractNamedEntry::equals method
6ff6b09: 8290501: Typo in javax.swing.BoundedRangeModel documentation
e07a5b6: 8338512: JFR: Revert changes to TestCodeSweeper
6d430f2: 8338314: JFR: Split JFRCheckpoint VM operation
f0374a0: 8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime
15b20cb: 8337886: java/awt/Frame/MaximizeUndecoratedTest.java fails in OEL due to a slight color difference
56a007d: 8338488: Add screen capture for failure case
2f7ba78: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment
... and 122 more: https://git.openjdk.org/jdk/compare/3f8b3e55276336cbb814d3b746d4337678389969...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 · 2024-08-06T23:14:54Z

@DamonGuy 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 · 2024-08-06T23:17:41Z

Webrevs

honkar-jdk

In some L&Fs I see margin defaults set using Button.margin.
Where does Metal LAF finally obtain these margin values from?[top=2,left=14,bottom=2,right=14].

Adding your margin comment will provide clarity & quick reference since these values are not evident.

aivanov-jdk

I've no objections from adding a comment in the code. However, the comment looks as if hanging in the air. There are CheckBox.totalInsets and RadioButton.totalInsets but there's no similar property for Button.

As for me, “Doc change” implies javadoc, but it is not the case. You're add a regular code comment which no one sees, except for developers.

The subject has be clearer in this respect. Like “Comment about difference” or “Note about difference…”, to avoid any confusion.

DamonGuy · 2024-08-07T22:32:43Z

I've no objections from adding a comment in the code. However, the comment looks as if hanging in the air. There are CheckBox.totalInsets and RadioButton.totalInsets but there's no similar property for Button.

As for me, “Doc change” implies javadoc, but it is not the case. You're add a regular code comment which no one sees, except for developers.

The subject has be clearer in this respect. Like “Comment about difference” or “Note about difference…”, to avoid any confusion.

Updated thanks. Originally this was meant to be a visible doc change when I created the issue long ago. But after looking thru the docs for Metal and any other appropriate APIs, I couldn't find a great spot to place it other than here.

I agree that the comment seems to be out of place. Do you have any suggestions for where to place it otherwise? I'm currently looking into where the Metal margin defaults are set for @honkar-jdk comment.

DamonGuy · 2024-08-09T22:35:06Z

I found the values to be set in BasicLookAndFeel.java which explains why other custom L&F's are also affected. Moving the doc to there and changing this back to a public doc change can be done instead. Otherwise, I can leave it as a comment note where the value is set.

honkar-jdk · 2024-08-10T00:14:59Z

I found the values to be set in BasicLookAndFeel.java which explains why other custom L&F's are also affected. Moving the doc to there and changing this back to a public doc change can be done instead. Otherwise, I can leave it as a comment note where the value is set.

It looks okay to me to have it has a comment in MetalLAF as it is now.

Moving the doc to there and changing this back to a public doc change can be done instead

I think what @aivanov-jdk meant here is that since this is a comment, only the JBS title needs a change to reflect that it is a comment and not a javadoc change. Even if it is moved to BasicLAF, it would still be added as a comment, correct?

honkar-jdk

LGTM

aivanov-jdk · 2024-08-12T09:53:10Z

I still don't quite understand where the margin comes from.

A comment, like the one you have currently, which points to where the values come from would be good.

Adding anything to public javadoc requires a CSR. Not sure if you want to go this way. It's not worth it. If developer can customise the border, it should not be specified in javadoc.

DamonGuy · 2024-08-13T19:16:02Z

I still don't quite understand where the margin comes from.

A comment, like the one you have currently, which points to where the values come from would be good.

Adding anything to public javadoc requires a CSR. Not sure if you want to go this way. It's not worth it. If developer can customise the border, it should not be specified in javadoc.

Metal (and the custom L&Fs) seem to use BasicLookAndFeel's default value for Button.margin which is (2, 14, 2, 14). This can be found in BasicLookAndFeel.java. Other L&Fs have set values for Button.margin but Metal does not. For example, AquaLookAndFeel has a value of (0, 2, 0, 2) for Button.margin.

I agree it's more difficult to add anything to public javadocs. I can add the note to BasicLookAndFeel.java instead and note that the default installed L&Fs have a vastly different horizontal margin compared to Basic's value minus Metal L&F. Not sure of the best way to phrase this yet, but this is where the margin values itself comes from.

alisenchung · 2024-08-14T20:43:35Z

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalLookAndFeel.java

@@ -782,6 +807,8 @@ protected void initComponentDefaults(UIDefaults table) {
                          "SPACE", "pressed",
                 "released SPACE", "released"
              }),
+            // margin is (2, 14, 2, 14) which is vastly larger in horizontal


I see below that there is another comment saying margin is 2 all the way around, what is the difference? how are these comments placed to indicate what margins they are referring to?

The below comments apply to check box and radio button; this comment applies to regular push button.

Yep. The issue is with regular buttons as Alexey mentioned. This caused a long discussion chain described in the linked issues. Nothing to do with CheckBox or RadioButton.

aivanov-jdk · 2024-08-15T18:32:08Z

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalLookAndFeel.java

@@ -782,6 +807,8 @@ protected void initComponentDefaults(UIDefaults table) {
                          "SPACE", "pressed",
                 "released SPACE", "released"
              }),
+            // margin is (2, 14, 2, 14) which is vastly larger in horizontal


I still suggest adding a reference to BasicBorder class or BasicLookAndFeel where this border is defined.

The borders for check boxes and radio buttons reference BasicBorders.RadioButtonBorder which helps to understand where the values are coming from.

Specifically, BasicLookAndFeel defines Button.margin property:

jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicLookAndFeel.java

Line 729 in 1cd4884

"Button.margin", new InsetsUIResource(2, 14, 2, 14),

It is set to the button in installDefaults:

jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicButtonUI.java

Lines 158 to 160 in 1cd4884

if(b.getMargin() == null || (b.getMargin() instanceof UIResource)) {

b.setMargin(UIManager.getInsets(pp + "margin"));

}

BasicBorders.getButtonBorder returns a border which defines colours and margins; the margins come from MarginBorder class which just returns the margins of a button component:

jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicBorders.java

Lines 493 to 495 in 1cd4884

if (c instanceof AbstractButton) {

AbstractButton b = (AbstractButton)c;

margin = b.getMargin();

Agreed. This is what I found as well. Had a bit of trouble figuring out how to phrase the note since the comment was meant to differentiate Metal from the other default installed LAFs.

I moved it to BasicLookAndFeel.java since that's where the values are specifically defined.

I moved it to BasicLookAndFeel.java since that's where the values are specifically defined.

Perhaps, we need a note in both files.

In MetalLookAndFeel.java to refer to BasicLookAndFeel where the margin is defined, similar to what's done for check boxes and radio button.

And in BasicLookAndFeel.java to note that the above margins are vastly different from other L&Fs.

What about this text?

MetalLookAndFeel.java

// default margin is (2, 14, 2, 14), defined in // BasicLookAndFeel via "Button.margin" UI property.

BasicLookAndFeel.java

// The above margin has vastly larger horizontal values when // compared to other look and feels that don't rely on these values

I agree, adding a note at both places looks clearer. The additional note in MetalLookAndFeel.java as above gives a quick lookup.

That does sound best. Added the notes to both areas as suggested. I still don't know how to not make the note in MetalLookAndFeel.java not feel like it's hanging, but I added the note to the same spot again. I think it's the most logical area.

LGTM, copyright years need to be updated for the files.

aivanov-jdk

Looks good to me.

Please, update copyright year in both files.

openjdk · 2024-08-19T16:28:05Z

@DamonGuy This pull request has not yet been marked as ready for integration.

DamonGuy · 2024-08-19T16:42:16Z

/integrate

openjdk · 2024-08-19T16:43:43Z

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

3ca359a: 8335771: Improve stability of java/nio/channels/DatagramChannel tests
2766b09: 8338452: (dc) DatagramChannelImpl.blockingReceive with timeout may block indefinitely if all datagrams blocked by SecurityManager
f0fe313: 8338564: Remove obsolete AbstractNamedEntry::equals method
6ff6b09: 8290501: Typo in javax.swing.BoundedRangeModel documentation
e07a5b6: 8338512: JFR: Revert changes to TestCodeSweeper
6d430f2: 8338314: JFR: Split JFRCheckpoint VM operation
f0374a0: 8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime
15b20cb: 8337886: java/awt/Frame/MaximizeUndecoratedTest.java fails in OEL due to a slight color difference
56a007d: 8338488: Add screen capture for failure case
2f7ba78: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment
... and 122 more: https://git.openjdk.org/jdk/compare/3f8b3e55276336cbb814d3b746d4337678389969...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-19T16:43:48Z

@DamonGuy Pushed as commit 6460b30.

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

Initial commit

ba4a832

openjdk bot added the rfr Pull request is ready for review label Aug 6, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Aug 6, 2024

honkar-jdk reviewed Aug 7, 2024

View reviewed changes

aivanov-jdk suggested changes Aug 7, 2024

View reviewed changes

DamonGuy changed the title ~~8321140: Doc change to note difference in Metal's JButton margins~~ 8321140: Add comment to note difference in Metal's JButton margins Aug 7, 2024

honkar-jdk approved these changes Aug 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 10, 2024

alisenchung reviewed Aug 14, 2024

View reviewed changes

aivanov-jdk suggested changes Aug 15, 2024

View reviewed changes

Move note

e590b8d

openjdk bot removed the ready Pull request is ready to be integrated label Aug 15, 2024

Notes in both LAFs

a16b29b

aivanov-jdk approved these changes Aug 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 19, 2024

Update copyright year

5bd6948

openjdk bot removed the ready Pull request is ready to be integrated label Aug 19, 2024

honkar-jdk approved these changes Aug 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 19, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 19, 2024

openjdk bot closed this Aug 19, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8321140: Add comment to note difference in Metal's JButton margins #20482

8321140: Add comment to note difference in Metal's JButton margins #20482

DamonGuy commented Aug 6, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 6, 2024

openjdk bot commented Aug 6, 2024 •

edited

Loading

openjdk bot commented Aug 6, 2024

mlbridge bot commented Aug 6, 2024 •

edited

Loading

honkar-jdk left a comment

aivanov-jdk left a comment

DamonGuy commented Aug 7, 2024

DamonGuy commented Aug 9, 2024

honkar-jdk commented Aug 10, 2024

honkar-jdk left a comment

aivanov-jdk commented Aug 12, 2024

DamonGuy commented Aug 13, 2024

alisenchung Aug 14, 2024

aivanov-jdk Aug 15, 2024

DamonGuy Aug 15, 2024

aivanov-jdk Aug 15, 2024

DamonGuy Aug 15, 2024

aivanov-jdk Aug 16, 2024

honkar-jdk Aug 16, 2024 •

edited

Loading

DamonGuy Aug 16, 2024

honkar-jdk Aug 16, 2024

aivanov-jdk left a comment

openjdk bot commented Aug 19, 2024

DamonGuy commented Aug 19, 2024

openjdk bot commented Aug 19, 2024

openjdk bot commented Aug 19, 2024

	if(b.getMargin() == null \|\| (b.getMargin() instanceof UIResource)) {
	b.setMargin(UIManager.getInsets(pp + "margin"));
	}

	if (c instanceof AbstractButton) {
	AbstractButton b = (AbstractButton)c;
	margin = b.getMargin();

8321140: Add comment to note difference in Metal's JButton margins #20482

8321140: Add comment to note difference in Metal's JButton margins #20482

Conversation

DamonGuy commented Aug 6, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 6, 2024

openjdk bot commented Aug 6, 2024 • edited Loading

openjdk bot commented Aug 6, 2024

mlbridge bot commented Aug 6, 2024 • edited Loading

Webrevs

honkar-jdk left a comment

Choose a reason for hiding this comment

aivanov-jdk left a comment

Choose a reason for hiding this comment

DamonGuy commented Aug 7, 2024

DamonGuy commented Aug 9, 2024

honkar-jdk commented Aug 10, 2024

honkar-jdk left a comment

Choose a reason for hiding this comment

aivanov-jdk commented Aug 12, 2024

DamonGuy commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 19, 2024

DamonGuy commented Aug 19, 2024

openjdk bot commented Aug 19, 2024

openjdk bot commented Aug 19, 2024

DamonGuy commented Aug 6, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 6, 2024 •

edited

Loading

mlbridge bot commented Aug 6, 2024 •

edited

Loading

honkar-jdk Aug 16, 2024 •

edited

Loading