<Swing Dev> RFR: 8263496: MetalHighContrastTheme.getControlHighlight cleanup

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> RFR: 8263496: MetalHighContrastTheme.getControlHighlight cleanup

Prasanta Sadhukhan-2
SonarCloud reports the potential issue with MetalHighContrastTheme.getControlHighlight where `controlHighlight ` field is not used and the getControlHighlight() uses secondary field.
public ColorUIResource getControlHighlight() {
        // This was super.getSecondary3();
        return secondary2;
    }
Removed the unused field.

-------------

Commit messages:
 - 8263496: MetalHighContrastTheme.getControlHighlight cleanup

Changes: https://git.openjdk.java.net/jdk/pull/3149/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3149&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263496
  Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3149.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3149/head:pull/3149

PR: https://git.openjdk.java.net/jdk/pull/3149
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8263496: MetalHighContrastTheme.getControlHighlight cleanup

Alexander Zvegintsev-2
On Tue, 23 Mar 2021 10:26:42 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> SonarCloud reports the potential issue with MetalHighContrastTheme.getControlHighlight where `controlHighlight ` field is not used and the getControlHighlight() uses secondary field.
> public ColorUIResource getControlHighlight() {
>         // This was super.getSecondary3();
>         return secondary2;
>     }
> Removed the unused field.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalHighContrastTheme.java line 55:

> 53:                               255, 255, 255);
> 54:     private static final ColorUIResource controlHighlight = new
> 55:                               ColorUIResource(102, 102, 102);

I didn't check it on Windows(AFAICS it is used for Windows only), but I've done a quick check how it looks on Linux with both colors.

Using `controlHighlight`  in `getControlHighlight()`:
![using controlHighlight](https://user-images.githubusercontent.com/77687766/112357602-bf00dd80-8ce0-11eb-9a8d-0faeb6d1940b.png)

Using `secondary2`  in `getControlHighlight()`:
![using secondary2](https://user-images.githubusercontent.com/77687766/112357736-db9d1580-8ce0-11eb-8e1a-527ec327874f.png)

And it seems that the option with unused `controlHighlight` color is more fits the definition of high contrast.

Have you checked why its usage was removed?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3149
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8263496: MetalHighContrastTheme.getControlHighlight cleanup

Prasanta Sadhukhan-2
On Wed, 24 Mar 2021 17:51:44 GMT, Alexander Zvegintsev <[hidden email]> wrote:

>> SonarCloud reports the potential issue with MetalHighContrastTheme.getControlHighlight where `controlHighlight ` field is not used and the getControlHighlight() uses secondary field.
>> public ColorUIResource getControlHighlight() {
>>         // This was super.getSecondary3();
>>         return secondary2;
>>     }
>> Removed the unused field.
>
> src/java.desktop/share/classes/javax/swing/plaf/metal/MetalHighContrastTheme.java line 55:
>
>> 53:                               255, 255, 255);
>> 54:     private static final ColorUIResource controlHighlight = new
>> 55:                               ColorUIResource(102, 102, 102);
>
> I didn't check it on Windows(AFAICS it is used for Windows only), but I've done a quick check how it looks on Linux with both colors.
>
> Using `controlHighlight`  in `getControlHighlight()`:
> ![using controlHighlight](https://user-images.githubusercontent.com/77687766/112357602-bf00dd80-8ce0-11eb-9a8d-0faeb6d1940b.png)
>
> Using `secondary2`  in `getControlHighlight()`:
> ![using secondary2](https://user-images.githubusercontent.com/77687766/112357736-db9d1580-8ce0-11eb-8e1a-527ec327874f.png)
>
> And it seems that the option with unused `controlHighlight` color is more fits the definition of high contrast.
>
> Have you checked why its usage was removed?

As far I see in the history of this file, it was `secondary2` that was used from beginning, so I kept it and removed the unused.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3149
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8263496: MetalHighContrastTheme.getControlHighlight cleanup

Alexander Zvegintsev-2
In reply to this post by Prasanta Sadhukhan-2
On Tue, 23 Mar 2021 10:26:42 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> SonarCloud reports the potential issue with MetalHighContrastTheme.getControlHighlight where `controlHighlight ` field is not used and the getControlHighlight() uses secondary field.
> public ColorUIResource getControlHighlight() {
>         // This was super.getSecondary3();
>         return secondary2;
>     }
> Removed the unused field.

Marked as reviewed by azvegint (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3149
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> Integrated: 8263496: MetalHighContrastTheme.getControlHighlight cleanup

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
On Tue, 23 Mar 2021 10:26:42 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> SonarCloud reports the potential issue with MetalHighContrastTheme.getControlHighlight where `controlHighlight ` field is not used and the getControlHighlight() uses secondary field.
> public ColorUIResource getControlHighlight() {
>         // This was super.getSecondary3();
>         return secondary2;
>     }
> Removed the unused field.

This pull request has now been integrated.

Changeset: 0696fd0e
Author:    Prasanta Sadhukhan <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/0696fd0e
Stats:     2 lines in 1 file changed: 0 ins; 2 del; 0 mod

8263496: MetalHighContrastTheme.getControlHighlight cleanup

Reviewed-by: azvegint

-------------

PR: https://git.openjdk.java.net/jdk/pull/3149