<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Jonathan Lu
luchsh at linux.vnet.ibm.com
Tue Apr 24 01:51:27 UTC 2012
Hi Pavel,
What do you think of this issue?
Regards!
- Jonathan
On 04/06/2012 03:55 PM, Jonathan Lu wrote:
> Hi Pavel
>
> On 04/03/2012 07:45 PM, Pavel Porvatov wrote:
>> Hi Jonathan,
>>
>> I have several comments:
>> 1. containerParent instanceof JComboBox && containerParent != null
>> it is not necessary to check "containerParent != null" here
>>
>
> Yes, the null checking is not necessary 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
>>
>
> There is indeed some changes, please refer to following link of raw
> text patch,
> http://cr.openjdk.java.net/~luchsh/7154030_3/jdk.patch
>
> The statement of retrieving focus-line-width property has been moved up.
>
>> 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 found the explanation of GTK geometry from following link
> http://svn.gnome.org/viewvc/gtk%2B/trunk/docs/widget_geometry.txt?view=markup
>
>
> And above piece of code is trying to render interior focus separately
> from exterior focus in the same way as
> GTKPainter.paintButtonBackgroundImpl(), (GTKPainter.java: 363).
>
>>
>> I applied the patch and observe that focused JComboBox looks strange
>> (see attachments):
>> a. Native focus uses solid line
> The patch only changed the size and position of focus rectangle not
> the style, so it is using the unchanged focus style.
>> b. Native focused JComboBox paints focus rectangle across whole
>> JComboBox
> It depends on the GTK version and ComboBox's 'style'.
>
> I made a simple native program for questions b.
>
> /*
> * Copyright (c) 2012 Oracle and/or its affiliates. All rights reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> * under the terms of the GNU General Public License version 2 only, as
> * published by the Free Software Foundation.
> *
> * This code is distributed in the hope that it will be useful, but
> WITHOUT
> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> * version 2 for more details (a copy is included in the LICENSE file
> that
> * accompanied this code).
> *
> * You should have received a copy of the GNU General Public License
> version
> * 2 along with this work; if not, write to the Free Software Foundation,
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> *
> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065
> USA
> * or visit www.oracle.com if you need additional information or have any
> * questions.
> */
>
> /*
> * Portions Copyright (c) 2012 IBM Corporation
> */
>
> #include <gtk/gtk.h>
>
> static gboolean delete_event(GtkWidget *widget, GdkEvent *event,
> gpointer data ) {
> return FALSE;
> }
>
> static void destroy(GtkWidget *widget, gpointer data ) {
> gtk_main_quit ();
> }
>
> int main(int argc, char *argv[]) {
> GtkWidget *window;
> GtkWidget *comboText;
> GtkWidget *fixed;
> GtkWidget *button;
> GtkWidget *combo;
> GtkWidget *comboEntry;
> GList *glist = NULL;
>
> gtk_init (&argc, &argv);
>
> // init
> window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
> g_signal_connect (window, "delete-event", G_CALLBACK
> (delete_event), NULL);
> g_signal_connect (window, "destroy", G_CALLBACK (destroy), NULL);
> gtk_container_set_border_width (GTK_CONTAINER (window), 10);
>
> fixed = gtk_fixed_new();
> gtk_container_add (GTK_CONTAINER (window), fixed);
>
> //Button
> button = gtk_button_new_with_label("GTK button");
>
> // ComboBox using combo box text new()
> comboText = gtk_combo_box_text_new();
> gtk_combo_box_append_text(GTK_COMBO_BOX(comboText), "aaaaa");
> gtk_combo_box_append_text(GTK_COMBO_BOX(comboText),
> "GtkComboBoxText");
> gtk_combo_box_set_active(GTK_COMBO_BOX(comboText), TRUE);
>
> // ComboBox using deprecated combo
> combo = gtk_combo_new();
> glist = g_list_append (glist, "GtkCombo");
> glist = g_list_append (glist, "GTK");
> glist = g_list_append (glist, "theme");
> gtk_combo_set_popdown_strings (GTK_COMBO (combo), glist);
>
> // ComboBox using combo box entry new()
> GtkTreeIter iter;
> GtkTreeModel *list_store = gtk_list_store_new(1, G_TYPE_STRING);
> gtk_list_store_clear(list_store);
> gtk_list_store_append(GTK_LIST_STORE(list_store), &iter);
> gtk_list_store_set(GTK_LIST_STORE(list_store), &iter,
> G_TYPE_STRING, "123", -1);
> comboEntry = gtk_combo_box_entry_new_with_model(list_store, 0);
> gtk_combo_box_set_active(GTK_COMBO_BOX(comboEntry), TRUE);
>
> // pack & show
> gtk_fixed_put(GTK_FIXED(fixed), button, 0, 0);
> gtk_fixed_put(GTK_FIXED(fixed), comboText, 0, 30);
> gtk_fixed_put(GTK_FIXED(fixed), comboEntry, 0, 60);
> gtk_fixed_put(GTK_FIXED(fixed), combo, 0, 90);
>
> gtk_widget_show(button);
> gtk_widget_show(comboText);
> gtk_widget_show(combo);
> gtk_widget_show(comboEntry);
>
> gtk_widget_show (fixed);
> gtk_widget_show (window);
> gtk_main ();
>
> // clean up
> g_list_free(glist);
>
> return 0;
> }
>
> Compile above code using following command and run the binary,
> gcc -Wall comboboxtest.c -o comboboxtest `pkg-config --cflags
> gtk+-2.0` `pkg-config --libs gtk+-2.0`
>
> So far as I see from the docs and above demo, there're several ways
> for native GTK application to create a ComboBox component,
> 1), gtk_combo_box_text_new() way will draw focus rectangle accross the
> whole ComboBox component;
> 2), gtk_combo_box_entry_new_with_model() way will have two focus
> points and draw the rectangle for each of them.
> 3), gtk_combo_new() way will draw focus rectangle on the text area
> only not on the whole ComboBox.
>
> I think current swing's ComboBox is trying to follow condition 3),
> right? if so, the focus size problem may not be a inconsistency
> problem, does that make sense?
>
>> 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
>>>
>>
>
> Thanks and best regards!
>
> - Jonathan
>
More information about the swing-dev
mailing list