<Swing Dev> Review request for JDK-6490753 JComboBox doesn't look as native combobox in different states of component
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Mar 6 09:50:21 UTC 2017
The fix looks good to me.
Thanks,
Alexandr.
On 3/3/2017 8:55 PM, Alexandr Scherbatiy wrote:
>
> I have uploaded the webrev to the http://cr.openjdk.java.net:
> http://cr.openjdk.java.net/~alexsch/mraz.martin/6490753/webrev.01
>
> Thanks,
> Alexandr.
>
> On 2/17/2017 2:41 PM, Martin M wrote:
>> 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
>> <alexandr.scherbatiy at oracle.com <mailto:alexandr.scherbatiy at oracle.com>>:
>>
>> On 2/1/2017 3:41 PM, Martin M wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-6490753
>>> <https://bugs.openjdk.java.net/browse/JDK-6490753>
>>> webrev:
>>> http://cr.openjdk.java.net/~alexsch/mraz.martin/6490753/webrev.00
>>> <http://cr.openjdk.java.net/%7Ealexsch/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
>> <http://cr.openjdk.java.net/%7Ealundblad/styleguide/index-v6.html>
>>
>> Thanks,
>> Alexandr.
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20170306/2745e48c/attachment.html>
More information about the swing-dev
mailing list