<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 Apr 6 07:55:41 UTC 2012
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