<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Jonathan Lu
luchsh at linux.vnet.ibm.com
Wed Jun 6 09:11:36 UTC 2012
Hello Alan,
On 05/24/2012 11:29 PM, Pavel Porvatov wrote:
> Hi Jonathan,
>
> Now focused JComboBox looks good. But it's not clear for me where did
> you find the following constants:
>
> + x += 3 * focusSize;
> + y += 2 * focusSize;
> + w -= 3 * focusSize;
> + h -= 4 * focusSize;
>
> ?
>
These constants are making the focus rectangle of JComboBox under GTK
L&F a little smaller to show itself by shrinking the size and shifting
by one focusSize.
This piece of painting code is also used by text field classes but the
actual effect is that the focus line will be covered by the bound lines
and cannot be seen by users. My patch just keep the old approach of
other text field classes and will not change the appearance of them, but
resize the focus rectangle of JComboBox to be smaller than the bound
rectangle.
Thanks
- Jonathan
> I took a look at the
> http://svn.gnome.org/viewvc/gtk%2B/trunk/docs/widget_geometry.txt?view=markup
> page but didn't find such information.
>
> Regards, Pavel
>> Hi Pavel,
>>
>> Here's the updated patch,
>> http://cr.openjdk.java.net/~luchsh/7155887_4/
>>
>> The shifting pixel problem was fix following the widget geometry
>> documents from
>> http://svn.gnome.org/viewvc/gtk%2B/trunk/docs/widget_geometry.txt?view=markup
>>
>>
>> Could you please take a look?
>>
>> Thanks!
>> - Jonathan
>>
>> On 05/21/2012 09:55 PM, Pavel Porvatov wrote:
>>> Hi Jonathan,
>>>
>>> Sorry for the delay, I was on vacation...
>>>
>>> I applied your patch and got the following results (see attached
>>> screenshot). If you magnify the image you will see that focus
>>> rectangle is moved to left on 1 pixel. Is it possible to fix that?
>>>
>>> All other changes looks good for me (except unnecessary
>>> "containerParent != null" checking, as I said before)
>>>
>>> Regards, Pavel
>>>
>>>> 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