<Swing Dev> Request for review 7155887: ComboBox does not paint focus correctly on GTK L&F
Pavel Porvatov
pavel.porvatov at oracle.com
Wed Jun 13 15:28:54 UTC 2012
Hi Jonathan,
> 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
Thanks for the info, I've marked the bug as fixed.
Regards, Pavel
>
> 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