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

[Jonathan Lu]

Hi Pavel,

For the native_paint_focus imlementation, the underlying GTK 
implementation will just ignore the condition
|width <= 0 || height <= 0||

See |
||http://git.gnome.org/browse/gtk+/tree/gtk/gtkstylecontext.c:4133

So the excluding of focusSize == 0 seems reasonable to me.

Cheers!
- Jonathan

On 03/26/2012 04:29 PM, Jonathan Lu wrote:
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120326/80d02f94/attachment.html>