Quantcast

<Swing Dev> Review request for JDK-6490753 JComboBox doesn't look as native combobox in different states of component

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

<Swing Dev> Review request for JDK-6490753 JComboBox doesn't look as native combobox in different states of component

Martin M
bug: https://bugs.openjdk.java.net/browse/JDK-6490753
webrev: http://cr.openjdk.java.net/~alexsch/mraz.martin/6490753/webrev.00

Problem description:
Swing JComboBox looks different than native one in states like e.g. focused state or mouse rollover state and so on.

The fix solves following issues:
- Editable combobox border is regular dark rectangle in all states now. After the fix border will have light gray color in normal state, light blue in rollover or hot state and dark blue in pressed or focused state.
- Editable combobox button is painted almost properly, but not animated as it should be. The fix will correct animation of the button.
- popup with list of choices has black border. The fix will correct the border to proper colors.
- All texts were rendered very close to left side of borders. The fix shifts texts few screen points to the left. 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <Swing Dev> Review request for JDK-6490753 JComboBox doesn't look as native combobox in different states of component

Alexandr Scherbatiy
On 2/1/2017 3:41 PM, Martin M wrote:
bug: https://bugs.openjdk.java.net/browse/JDK-6490753
webrev: http://cr.openjdk.java.net/~alexsch/mraz.martin/6490753/webrev.00

Problem description:
Swing JComboBox looks different than native one in states like e.g. focused state or mouse rollover state and so on.

The fix solves following issues:
- Editable combobox border is regular dark rectangle in all states now. After the fix border will have light gray color in normal state, light blue in rollover or hot state and dark blue in pressed or focused state.
- Editable combobox button is painted almost properly, but not animated as it should be. The fix will correct animation of the button.
- popup with list of choices has black border. The fix will correct the border to proper colors.
- All texts were rendered very close to left side of borders. The fix shifts texts few screen points to the left.

AnimationController.java
 386                 if (skin.haveToSwitchStates()) {
 387                     skin.paintSkinRaw(g, dx, dy, dw, dh, state);
 388                     g.setComposite(AlphaComposite.SrcOver.derive(1 - progress));
 389                     skin.paintSkinRaw(g, dx, dy, dw, dh, startState);
 390                 } else {

- Could the '1 - progress' value in AlphaComposite.SrcOver.derive(1 - progress) be out of range?

WindowsComboBoxUI.java
 221             if (this.borderChecked == false) // check border of component
 222                 replaceBorder();

It is better to use 'if (!this.borderChecked) {replaceBorder();}'.
There are Java Style Guidelines [1] which we need to follow to.


 454             if (comboBox.isPopupVisible())
 455                 getModel().setPressed(true);
 456             else
 457                 getModel().setPressed(false);

This can be simplified to getModel().setPressed(comboBox.isPopupVisible()).


 508     @Deprecated
 509     @SuppressWarnings("serial") // Superclass is not serializable across versions
 510     protected class WindowsComboPopup extends BasicComboPopup {
 511
 512         private class comboPopUpBorder extends EmptyBorder {

The class WindowsComboPopup is marked as deprecated with comments '* This class is now obsolete and doesn't do anything.'
Is it possible to move the popup border class outside and do not use the WindowsComboPopup at all?
The comboPopUpBorder class name should start from capital char.


 566             Border border = (Border)UIManager.get("ComboBox.editorBorder");
 567
 568             // correct space on the left side of text (for editable combobox)
 569             Insets i = border.getBorderInsets(editor);
 570             border = BorderFactory.createEmptyBorder(i.top, 4, i.bottom, i.right);
 571
 572             if (border != null) {
 573                 editor.setBorder(border);
 574             }

- The border.getBorderInsets(editor) is called before checking the border to null.
- It seems that if a user sets a custom "ComboBox.editorBorder" it should not be changed.
Is it possible just to update the dafult "ComboBox.editorBorder" in the com.sun.java.swing.plaf.windows.WindowsLookAndFeel class?


XPStyle.java
 517         public boolean haveToSwitchStates() {
 518             return switchStates;
 519         }
 520
 521         public void switchStates(boolean b) {
 522             switchStates = b;
 523         }

Could you change the methods access from the public to package? Making some API public, even in com.sun.* package may require some additional process.

[1] http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html

Thanks,
Alexandr.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <Swing Dev> Review request for JDK-6490753 JComboBox doesn't look as native combobox in different states of component

Martin M
AnimationController.java
 386                 if (skin.haveToSwitchStates()) {
 387                     skin.paintSkinRaw(g, dx, dy, dw, dh, state);
 388                     g.setComposite(AlphaComposite.SrcOver.derive(1 - progress));
 389                     skin.paintSkinRaw(g, dx, dy, dw, dh, startState);
 390                 } else {

- Could the '1 - progress' value in AlphaComposite.SrcOver.derive(1 - progress) be out of range?

Value 'progress' is checked in method 'private void updateProgress()' so it always is between 0 and 1.


WindowsComboBoxUI.java
 221             if (this.borderChecked == false) // check border of component
 222                 replaceBorder();

It is better to use 'if (!this.borderChecked) {replaceBorder();}'.
There are Java Style Guidelines [1] which we need to follow to. 


I removed this part and replaced it with one line into method 'public void installUI( JComponent c )'. It has the same effect.
WindowsComboBoxUI.java
159        // set empty border as default to see vista animated border
160        comboBox.setBorder(new EmptyBorder(0,0,0,0)); 


454             if (comboBox.isPopupVisible())
455                 getModel().setPressed(true);
456             else
457                 getModel().setPressed(false);

This can be simplified to getModel().setPressed(comboBox.isPopupVisible()).


Corrected. this was lame :(


 508     @Deprecated
 509     @SuppressWarnings("serial") // Superclass is not serializable across versions
 510     protected class WindowsComboPopup extends BasicComboPopup {
 511 
 512         private class comboPopUpBorder extends EmptyBorder {

The class WindowsComboPopup is marked as deprecated with comments '* This class is now obsolete and doesn't do anything.'
Is it possible to move the popup border class outside and do not use the WindowsComboPopup at all?
The comboPopUpBorder class name should start from capital char.

I removed comboPopUpBorder class because painting of this border was hardcoded and it looked the same in win7 and win10. 
The border is now painted with system skin and looks properly in both windows versions.
And I am not sure if it is ok, but I created new class WinComboPopUp which extends BasicComboPopup
 to avoid using deprecated WindowsComboPopup. But deprecated class also extends BasicComboPopup. So....


566             Border border = (Border)UIManager.get("ComboBox.editorBorder");
 567 
 568             // correct space on the left side of text (for editable combobox)
 569             Insets i = border.getBorderInsets(editor);
 570             border = BorderFactory.createEmptyBorder(i.top, 4, i.bottom, i.right);
 571 
 572             if (border != null) {
 573                 editor.setBorder(border);
 574             }

- The border.getBorderInsets(editor) is called before checking the border to null.
- It seems that if a user sets a custom "ComboBox.editorBorder" it should not be changed.
Is it possible just to update the dafult "ComboBox.editorBorder" in the com.sun.java.swing.plaf.windows.WindowsLookAndFeel class? 

I updated default ComboBox.editorBorder.


XPStyle.java
 517         public boolean haveToSwitchStates() {
 518             return switchStates;
 519         }
 520 
 521         public void switchStates(boolean b) {
 522             switchStates = b;
 523         }

Could you change the methods access from the public to package? Making some API public, even in com.sun.* package may require some additional process. 

I changed access modifier from public to package.

And I also decreased shifting of list items to right from 3px to 2px. 
If I see correctly native comboBox items have 2 px intendation from left side of the border, when system font is used.
WindowsComboBoxUI.java
 600 return new Insets(0,2,0,0);                            (previous value was 3)
 609 setBorder(new EmptyBorder(0, 2, 0, i.right));  (previous value was 3 as well)

br
Martin



2017-02-03 13:31 GMT+01:00 Alexandr Scherbatiy <[hidden email]>:
On 2/1/2017 3:41 PM, Martin M wrote:
bug: https://bugs.openjdk.java.net/browse/JDK-6490753
webrev: http://cr.openjdk.java.net/~alexsch/mraz.martin/6490753/webrev.00

Problem description:
Swing JComboBox looks different than native one in states like e.g. focused state or mouse rollover state and so on.

The fix solves following issues:
- Editable combobox border is regular dark rectangle in all states now. After the fix border will have light gray color in normal state, light blue in rollover or hot state and dark blue in pressed or focused state.
- Editable combobox button is painted almost properly, but not animated as it should be. The fix will correct animation of the button.
- popup with list of choices has black border. The fix will correct the border to proper colors.
- All texts were rendered very close to left side of borders. The fix shifts texts few screen points to the left.

AnimationController.java
 386                 if (skin.haveToSwitchStates()) {
 387                     skin.paintSkinRaw(g, dx, dy, dw, dh, state);
 388                     g.setComposite(AlphaComposite.SrcOver.derive(1 - progress));
 389                     skin.paintSkinRaw(g, dx, dy, dw, dh, startState);
 390                 } else {

- Could the '1 - progress' value in AlphaComposite.SrcOver.derive(1 - progress) be out of range?

WindowsComboBoxUI.java
 221             if (this.borderChecked == false) // check border of component
 222                 replaceBorder();

It is better to use 'if (!this.borderChecked) {replaceBorder();}'.
There are Java Style Guidelines [1] which we need to follow to.


 454             if (comboBox.isPopupVisible())
 455                 getModel().setPressed(true);
 456             else
 457                 getModel().setPressed(false);

This can be simplified to getModel().setPressed(comboBox.isPopupVisible()).


 508     @Deprecated
 509     @SuppressWarnings("serial") // Superclass is not serializable across versions
 510     protected class WindowsComboPopup extends BasicComboPopup {
 511
 512         private class comboPopUpBorder extends EmptyBorder {

The class WindowsComboPopup is marked as deprecated with comments '* This class is now obsolete and doesn't do anything.'
Is it possible to move the popup border class outside and do not use the WindowsComboPopup at all?
The comboPopUpBorder class name should start from capital char.


 566             Border border = (Border)UIManager.get("ComboBox.editorBorder");
 567
 568             // correct space on the left side of text (for editable combobox)
 569             Insets i = border.getBorderInsets(editor);
 570             border = BorderFactory.createEmptyBorder(i.top, 4, i.bottom, i.right);
 571
 572             if (border != null) {
 573                 editor.setBorder(border);
 574             }

- The border.getBorderInsets(editor) is called before checking the border to null.
- It seems that if a user sets a custom "ComboBox.editorBorder" it should not be changed.
Is it possible just to update the dafult "ComboBox.editorBorder" in the com.sun.java.swing.plaf.windows.WindowsLookAndFeel class?


XPStyle.java
 517         public boolean haveToSwitchStates() {
 518             return switchStates;
 519         }
 520
 521         public void switchStates(boolean b) {
 522             switchStates = b;
 523         }

Could you change the methods access from the public to package? Making some API public, even in com.sun.* package may require some additional process.

[1] http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html

Thanks,
Alexandr.





winComboBox.diff.txt (17K) Download Attachment
Loading...