<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Pavel Porvatov
pavel.porvatov at oracle.com
Sat Jun 9 13:35:42 UTC 2012
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