<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F

[Pavel Porvatov]

Hi Jonathan,

What do you think about another solution: can we set component state as 
SynthConstants.FOCUSED before paintTextBackground is invoked. Another 
solution is to set state as "focused" for ComboBox renderer like the 
following:

         if ("ComboBox.renderer".equals(c.getName())) {
             for (Component comboBoxParent = c.getParent(); 
comboBoxParent != null; comboBoxParent = comboBoxParent.getParent()) {
                 if (comboBoxParent instanceof JComboBox){
                     if(comboBoxParent.hasFocus()){
                         state |= SynthConstants.FOCUSED;
                     }
                     break;
                 }
             }
         }
without other changes in GTKPainter.java (actually there is some problem 
with "interiorFocus", but it could be resolved....)

See also my answers below.
> Hi Pavel,
>
> Thanks for review, here's the new patch and my answers are inlined.
> http://cr.openjdk.java.net/~luchsh/7155887_2/
>
> On 03/22/2012 10:24 PM, Pavel Porvatov wrote:
>> Hi Jonathan,
>>> Hi Swing-dev,
>>>
>>> ComboBox on linux GTK L&F does not works as gtk native applications, 
>>> when get focused, the apperance of Java ComboBox remains unchanged 
>>> but native GTK ComboBox control will have a outline to indicate it 
>>> has got focused.
>>>
>>> The problem seems similar to bug
>>> 6947671 ( http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6947671),
>>> except that I did not reproduced the problem on Nimbus L&F, so 
>>> another bug
>>> 7155887 (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7155887)
>>> was created for this issue,
>>>
>>> And here's the proposed patch to fix this problem,
>>> http://cr.openjdk.java.net/~luchsh/7155887/
>>>
>>> Could anybody please help to take a look?
>> I have several comments about the patch:
>>
>> 1. "c.getName().equals("ComboBox.renderer")": I think we can get NPE 
>> here
> Yes, I've changed it to
>
> "ComboBox.renderer".equals(c.getName())
>
>>
>> 2.
>> +            for (Component comboBoxParent = c.getParent(); 
>> comboBoxParent != null; comboBoxParent = comboBoxParent
>> +                    .getParent()) {
>> +                if (comboBoxParent instanceof JComboBox
>> + && comboBoxParent.hasFocus()) {
>> +                    comboBoxFocused = true;
>> +                }
>> +            }
>>
>> I'm not sure we should do such deep parent investigation. Why don't 
>> you check first parent only?
>>
> javax.swing.CellRendererPane is inserted between the component and 
> renderer, so if check only the first parent, it will retrieve a 
> CellRendererPane object instead of JComboBox component. In the new 
> patch, I added a break when JComboBox is encounterred so to make the 
> effect similar to the first-parent-only approach.
I found out the following code (see 
com.sun.java.swing.plaf.gtk.GTKPainter#paintLabelBackground):
         if (c instanceof ListCellRenderer &&
                  container != null &&
                  container.getParent() instanceof JComboBox ) {
             ...
         }
I think we should use the same pattern
>
>> 3. "if (ENGINE.paintCachedImage(g, x, y, w, h, id, state) && 
>> !comboBoxFocused)"
>> If you are going to ignore ENGINE.paintCachedImage when 
>> comboBoxFocused, then there is no need to invoke it at all
>>
> yes, in the new patch I've changed the order of these two checks.
>> 4. "if (comboBoxFocused || focusSize > 0)"
>> I'm not sure we should paint focus if focusSize == 0
>>
> I think there's no need to paint the focus if focusSize ==0, since the 
> focus width and height arguements passed to JNI method 
> native_paint_focus() will both be zero.
That's what I meant! (may be my phrase was not clear enough)
Your condition "if (comboBoxFocused || focusSize > 0)" allows to paint 
focus even if focusSize == 0...

Regards, Pavel