<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Pavel Porvatov
pavel.porvatov at oracle.com
Thu Jun 7 17:19:48 UTC 2012
Hi Jonathan,
> 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.
Yes, I understand that. But why don't you write something like
x += focusSize + 2
? (that's just for example)
>
> 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.
I didn't found the same code in other text field classes. Could you
please explain what did you mean? Why do you increase "x" on "3 *
focusSize", but "y" on "2 * focusSize"& It should produce bad result
when focusSize is greater then 5...
Regards, Pavel
>
> 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