<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 Jun 11 05:11:52 UTC 2012
Hi Pavel,
Thanks for review
Charles has helped to commit the patch and I've verified it.
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/b57167b71169
Best regards
Jonathan
On 06/09/2012 09:35 PM, Pavel Porvatov wrote:
> Hi Jonathan,
>
> The fix looks good for me.
>
> Regards, Pavel
>> Hi Pavel,
>>
>> I realized the problem and refined the patch, could you please take
>> another look?
>> http://cr.openjdk.java.net/~luchsh/7155887_5/
>>
>> Thanks!
>> - Jonathan
>>
>> On 06/08/2012 01:19 AM, Pavel Porvatov wrote:
>>> 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