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

[Pavel Porvatov]

Hi Jonathan,

I have several comments:
1. containerParent instanceof JComboBox && containerParent != null
it is not necessary to check "containerParent != null" here

2.
-            if (!interiorFocus && (state & SynthConstants.FOCUSED) != 0) {
+
                  focusSize = style.getClassSpecificIntValue(context,
                          "focus-line-width",1);
+            if (!interiorFocus && (state & SynthConstants.FOCUSED) != 0) {
I don't see here any changes

3. Could you please explain the following changes?
-            if (focusSize > 0) {
+            if (focusSize > 0 && (state & SynthConstants.FOCUSED) != 0) {
+                if (interiorFocus) {
+                    x += focusSize;
+                    y += focusSize;
+                    w -= 2 * focusSize;
+                    h -= 2 * focusSize;
+                } else {


I applied the patch and observe that focused JComboBox looks strange 
(see attachments):
a. Native focus uses solid line
b. Native focused JComboBox paints focus rectangle across whole JComboBox

Regards, Pavel
> Hi Pavel,
>
> Here's the updated patch, including proposed solution for interior focus.
> http://cr.openjdk.java.net/~luchsh/7155887_3/
>
> In the orginal code as I understood, focus is only paint when 
> !interiorFocus, in which case the background shadow and flatBox will 
> shrink a bit to corperate with the outer focus whose size is same as 
> the original component. My proposed solution is to shirink focus line 
> for interior focus, but keep the same way of !interorFocus.
>
> could you pls take a look?
>
> On 03/27/2012 10:46 PM, Pavel Porvatov wrote:
>> 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...
>>
> Oh, sorry for my misunderstanding, the previous patch indeed got such 
> a problem, but it may not be in the new patch.
>> Regards, Pavel
>
> Thanks and best regards!
>
> - Jonathan
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ver3.png
Type: image/png
Size: 6286 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120403/69838650/ver3.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: native.png
Type: image/png
Size: 5986 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120403/69838650/native.png>