<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Jonathan Lu
luchsh at linux.vnet.ibm.com
Fri Jun 8 07:36:14 UTC 2012
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