<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Jonathan Lu
luchsh at linux.vnet.ibm.com
Mon May 21 06:36:49 UTC 2012
Hello Pavel,
Any more comments on this topic?
Regards, Jonathan
On 04/24/2012 04:31 PM, Pavel Porvatov wrote:
> Hi Jonathan,
>
> I remember about that and I need some additional time for the review.
>
> Regards, Pavel
>> 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