8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

prsadhuk · 2021-04-01T12:20:54Z

The current spec for javax/swing/plaf/BorderUIResource.BevelBorderUIResource.html(int,java.awt.Color,java.awt.Color) does not clearly specify the usage of color for hightlight and shadow as there are outer/inner hightlight and outer/inner shadow.
Updated spec to clarify the same.

Progress

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

Issue

JDK-8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color

Reviewers

Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3305

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

Using diff file

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

…n that the passed colors are for highlightInner and shadowOuter props

bridgekeeper · 2021-04-01T12:21:05Z

👋 Welcome back psadhukhan! 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 · 2021-04-01T12:23:01Z

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

mlbridge · 2021-04-01T12:26:35Z

Webrevs

dbessono · 2021-04-05T11:10:40Z

Is there a normative definition somewhere in the spec of what "derived color" is for this domain?

prsadhuk · 2021-04-05T11:23:36Z

As per https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/border/BevelBorder.java#L151
and https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/border/BevelBorder.java#L183
derived is used to obtain the darker or brighter version of the same color, which is being done here for this highlight and shadow color too.

prsadhuk · 2021-04-06T11:12:27Z

any comments on the spec wording?

dbessono · 2021-04-06T12:15:20Z

Does 'derived' in javax.swing domain mean the color would be different, or 'equal color' also qualifies as 'derived' javax.swing?

prsadhuk · 2021-04-06T12:31:00Z

In this context and BevelBorder context, 'derived' is used for brighter/darker version of the same color and as per Color.brighter method https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/Color.java#L636
it can return the same color on case-to-case basis.

mrserb · 2021-04-08T00:56:34Z

Please update the title of the bug/PR, since the change now in another class.

mrserb · 2021-04-08T00:56:45Z

/csr

openjdk · 2021-04-08T00:57:25Z

@mrserb this pull request will not be integrated until the CSR request JDK-8264706 for issue JDK-8264398 has been approved.

mrserb · 2021-04-08T01:00:37Z

BevelBorder vs BevelBorderUIResource. The BevelBorderUIResource is a UIResource version of BevelBorder and its spec is just a copy.

prsadhuk · 2021-04-08T04:20:16Z

Not sure I understand...The change is still in BevelorderUIResource(int, Color, Color) method so what should be changed in PR title?

mrserb · 2021-04-08T04:27:02Z

Not sure I understand...The change is still in BevelorderUIResource(int, Color, Color) method so what should be changed in PR title?

It should be Bevelorder as well, the class from where the spec is copied to the BevelorderUIResource.

prsadhuk · 2021-04-08T04:31:12Z

I don't think the spec is coped from BevelBorder...Yes, the spec is inspired by how BevelBorder treates the shadow/highlight color!!
You want same spec to be copied to BevelBorder(int, Color, Color) constrcutor too?

mrserb · 2021-04-08T04:44:29Z

I don't think the spec is coped from BevelBorder...Yes, the spec is inspired by how BevelBorder treates the shadow/highlight color!

The BevelBorderUIResource is just a tagged wrapper on top of the BevelBorder, it does not provide any additional functionality.

You want same spec to be copied to BevelBorder(int, Color, Color) constrcutor too?

Yes, this constructor is called from the BevelBorderUIResource, and it will be strange if the subclass will have a better spec than the parent.

mrserb · 2021-04-09T00:44:54Z

src/java.desktop/share/classes/javax/swing/border/BevelBorder.java

@@ -86,6 +86,10 @@ public BevelBorder(int bevelType) {
    /**
     * Creates a bevel border with the specified type, highlight and
     * shadow colors.
+     * highlightOuterColor and highlightInnerColor will be derived from


I think we should use just a brief text for "highlightOuterColor"/etc like in the other parts of class "the outer shadow"/"the inner shadow"/"the inner highlight" etc. Since in this particular case the highlightOuterColor is not a parameter nor a field.
And please update the copyright dates.

I guess I have done what is being asked. Are there any other comments? If not, can this PR and CSR be approved please?

So as per the updated version of the specification - what could be checked is that the resulting inner/outer/highlight/shadow colors should be either:

equal to the passed

::brighter() than the passed

darker() than the passed
?

Yes, the passed highlight and shadow color to the constructor would be equal to getHighlightInnerColor(), getShadowOuterColor but again this is for our jdk implementation. As told in the JBS, these are implementation details which should hold true if JDK's BevelBorder class is in use
but some other JDK can override the class and can give its own implementation as the class is not 'final'.

src/java.desktop/share/classes/javax/swing/border/BevelBorder.java

prsadhuk · 2021-04-13T09:22:04Z

Please have a look at the CSR too https://bugs.openjdk.java.net/browse/JDK-8264706

openjdk · 2021-04-15T05:27:28Z

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

8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color

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

e7cbeba: 8259070: Add jcmd option to dump CDS
5931948: 8265246: Fix macos-Aarch64 build after JDK-8263709
79bff21: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines
f4c3efd: 8265192: [macos_aarch64] configure script fails if GNU uname in PATH
e167577: 8265078: jpackage tests on Windows leave large temp files
05f851e: 8265236: ProblemList java/foreign/TestUpcall.java on macosx-aarch64
d1b28e7: 8265231: (fc) ReadDirect and WriteDirect tests fail after fix for JDK-8264821
57f86a0: 8265235: ProblemList java/foreign/TestIntrinsics.java on macosx-aarch64
4c83d24: 8058176: [mlvm] tests should not allow code cache exhaustion
9406744: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator
... and 372 more: https://git.openjdk.java.net/jdk/compare/d24e4cfef36026b781906a9e0c5cf519eb72696e...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.

prsadhuk · 2021-05-05T06:18:33Z

/integrate

openjdk · 2021-05-05T06:20:21Z

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

b172555: 8266171: -Warray-bounds happens in imageioJPEG.c
8bcebe7: 8265505: findsym does not work on remote debug server
b88785d: 8266038: Move newAddress() to JVMDebugger
2c53654: 8266179: [macos] jpackage should specify architecture for produced pkg files
d282799: 8255566: Add size validation when parsing values from VersionProps
61365d5: 8266465: Add wildcard to JTwork/JTreport exclude in jib-profiles.js
f00b70e: 8266527: RandomTestCoverage.java failing due to API removal
c53dee7: 8266227: Fix help text for --mac-signing-keychain
80323b7: 8261169: Upgrade HarfBuzz to the latest 2.8.0
9c4efdd: 8263124: Missed initialization of baselineY in sun.font.StrikeMetrics
... and 730 more: https://git.openjdk.java.net/jdk/compare/d24e4cfef36026b781906a9e0c5cf519eb72696e...master

Your commit was automatically rebased without conflicts.

Pushed as commit b71f85a.

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

8264398: BevelBorderUIResource(int, Color, Color) spec should mentio…

ea84775

…n that the passed colors are for highlightInner and shadowOuter props

openjdk bot added the rfr Pull request is ready for review label Apr 1, 2021

openjdk bot added the swing client-libs-dev@openjdk.org label Apr 1, 2021

Fix javadoc

cb0b25c

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 8, 2021

javadoc change in BevelBorder constructor

f5d6be5

mrserb reviewed Apr 9, 2021

View reviewed changes

prsadhuk added 2 commits April 9, 2021 04:10

javadoc change

7605c18

javadoc change

4e650b0

mrserb reviewed Apr 10, 2021

View reviewed changes

src/java.desktop/share/classes/javax/swing/border/BevelBorder.java Show resolved Hide resolved

Formatting

f0d9811

mrserb approved these changes Apr 12, 2021

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 15, 2021

openjdk bot added the ready Pull request is ready to be integrated label Apr 15, 2021

openjdk bot closed this May 5, 2021

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 May 5, 2021

prsadhuk deleted the 8264398 branch May 5, 2021 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

prsadhuk commented Apr 1, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 1, 2021

openjdk bot commented Apr 1, 2021

mlbridge bot commented Apr 1, 2021 •

edited

Loading

dbessono commented Apr 5, 2021

prsadhuk commented Apr 5, 2021

prsadhuk commented Apr 6, 2021

dbessono commented Apr 6, 2021

prsadhuk commented Apr 6, 2021

mrserb commented Apr 8, 2021

mrserb commented Apr 8, 2021

openjdk bot commented Apr 8, 2021

mrserb commented Apr 8, 2021

prsadhuk commented Apr 8, 2021

mrserb commented Apr 8, 2021

prsadhuk commented Apr 8, 2021

mrserb commented Apr 8, 2021

mrserb Apr 9, 2021

prsadhuk Apr 10, 2021

dbessono Apr 10, 2021

prsadhuk Apr 10, 2021

prsadhuk commented Apr 13, 2021

openjdk bot commented Apr 15, 2021 •

edited

Loading

prsadhuk commented May 5, 2021

openjdk bot commented May 5, 2021

8264398: BevelBorderUIResource​(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

8264398: BevelBorderUIResource​(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

Conversation

prsadhuk commented Apr 1, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 1, 2021

openjdk bot commented Apr 1, 2021

mlbridge bot commented Apr 1, 2021 • edited Loading

Webrevs

dbessono commented Apr 5, 2021

prsadhuk commented Apr 5, 2021

prsadhuk commented Apr 6, 2021

dbessono commented Apr 6, 2021

prsadhuk commented Apr 6, 2021

mrserb commented Apr 8, 2021

mrserb commented Apr 8, 2021

openjdk bot commented Apr 8, 2021

mrserb commented Apr 8, 2021

prsadhuk commented Apr 8, 2021

mrserb commented Apr 8, 2021

prsadhuk commented Apr 8, 2021

mrserb commented Apr 8, 2021

mrserb Apr 9, 2021

Choose a reason for hiding this comment

prsadhuk Apr 10, 2021

Choose a reason for hiding this comment

dbessono Apr 10, 2021

Choose a reason for hiding this comment

prsadhuk Apr 10, 2021

Choose a reason for hiding this comment

prsadhuk commented Apr 13, 2021

openjdk bot commented Apr 15, 2021 • edited Loading

prsadhuk commented May 5, 2021

openjdk bot commented May 5, 2021

8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color #3305

prsadhuk commented Apr 1, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 1, 2021 •

edited

Loading

openjdk bot commented Apr 15, 2021 •

edited

Loading