Request for review: 7124530 What is background color of AWT component? (And foreground, for that matter)

[Sergey Bylokhov]

Hi Alexander
29.12.2011 22:32, Alexander Potochkin wrote:
> Hello Sergey
>> Hi Everyone,
>> This is a fix for some glitches in the code for 
>> background/foreground/font properties.
>> 1. SystemColor.window was changed to color which is used by default 
>> in swing l&f(Aqua). Changes in AquaImageFactory.java and 
>> CSystemColors.m. Now most of the awt components use this color as 
>> default.
>> 2. set** methods were moved from LWWindowPeer to LWComponentPeer, 
>> because there is an issues when these methods has no effect, because 
>> repaint of the component does not happen.For example:
>>  - call Label.setbackground which set component background property
>>  - call Peer.setBackground
>>  - call Delegate.setBackgound
>>  - compare passed color with color from Label property, which was set 
>> in the first step: changes in 
>> LWWindowPeer,LWContainerPeer,LWComponentPeer.
> could you give more details on that?
By default delegates uses colors from DelegateContainer which take color 
from targets. So if we set color for target(Label) -> then set color for 
peer ->then set color for delegete. this code does not repaint our 
component:
     public void setBackground(Color bg) {
         Color oldBg = getBackground();
         super.setBackground(bg);
         if ((oldBg != null) ? !oldBg.equals(bg) : ((bg != null) && 
!bg.equals(oldBg))) {
             // background already bound in AWT1.2
             repaint();
         }
     }
Because bg and oldBg will be the same color.
>
> the pattern where every peer has its delegate which is responsible for 
> storing/repainting for colors and fonts
> looks more laconic and consistent to me.
This is true if all our components had delegates. But we has LWWindow 
which has no delegate and LWCanvas which the delegate serves only for 
storage of colors(By default LWComponentPeer has not delegates so we 
cannot simplify old code and eliminate null checks).Probably delegate 
from LWPanelPeer can be removed too.
And i think it would be better not to have similar code in different 
classes.
For example I believe this is better

protected final Color getBackground() {
     synchronized (getStateLock()) {
         return background;
     }
}

Than 2 different methods in different classes:

protected Color getBackground() {
     synchronized (getDelegateLock()) {
          D delegate = getDelegate();
          if (delegate != null) {
             return delegate.getBackground();
          }
     }
     return null;
}

protected Color getBackground() {
     synchronized (getStateLock()) {
         return background;
     }
}

same for setters
public void setBackground(final Color c) {
     final Color oldBg = getBackground();
     if (oldBg == c || (oldBg != null&&  oldBg.equals(c))) {
         return;
     }
     synchronized (getStateLock()) {
         background = c;
     }
     final D delegate = getDelegate();
     if (delegate != null) {
         synchronized (getDelegateLock()) {
             // delegate will repaint the target
             delegate.setBackground(c);
         }
     } else {
         repaintPeer();
     }
}

against:

public void setBackground(Color c) {
     final boolean changed;
     synchronized (getStateLock()) {
         changed = ((background != null) ? !background.equals(c) :
                   ((c != null)&&  !c.equals(background)));
         background = c;
     }
     if (changed) {
        repaintPeer();
     }
}

public void setBackground(Color c) {
     synchronized (getDelegateLock()) {
         D delegate = getDelegate();
         if (delegate != null) {
             // delegate will repaint the target
             delegate.setBackground(c);
         }
     }
}

>
>> 3. List default color was changed to SystemColor.text: changes in 
>> LWListPeer.java.
>
> LWListPeer set the font and colors to its view component,
> with the proposed fix it seems to update the scrollPane instead
But in resetColorsAndFont() we clear default colors for view and it use 
colors from its parent ScrollableJList.
>
>
>> 4. LWWIndow target color initialization was moved from CPlatformIndow 
>> to LWWindowPeer, so it can be reused later by CPlatformEmbeddedFrame .
>> 5. unnecessary peers set** methods and unnecessary delegate in 
>> LWCanvasPeer were removed: changes in LWCanvasPeer.java, 
>> LWListPeer.java, LWPanelPeer.java.
>>
>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7124530
>> Webrev can be found at: 
>> http://cr.openjdk.java.net/~serb/7124530/webrev.00/
>>
>
> Thanks
> alexp


-- 
Best regards, Sergey.