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

[Jonathan Lu]

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.

> 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.
> Regards, Pavel

Regards
- Jonathan