<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Jonathan Lu
luchsh at linux.vnet.ibm.com
Tue May 22 13:38:56 UTC 2012
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