After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 435471 - small GtkComboBox cleanup
small GtkComboBox cleanup
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkComboBox
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-05-03 10:27 UTC by Carlos Garnacho
Modified: 2007-06-13 23:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
cleanup patch (4.89 KB, patch)
2007-05-03 10:28 UTC, Carlos Garnacho
none Details | Review
updated patch (5.48 KB, patch)
2007-05-21 15:55 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2007-05-03 10:27:02 UTC
When the GtkComboBox::appears-as-list style property is TRUE, GtkComboBox creates a widget hierarchy for the menu like window->frame->scrolled_window->treeview, here the frame is borderless and with no shadow, so I'm attaching a patch to get rid of it
Comment 1 Carlos Garnacho 2007-05-03 10:28:08 UTC
Created attachment 87439 [details] [review]
cleanup patch
Comment 2 Murray Cumming 2007-05-04 20:46:48 UTC
So this has no effect on the appearance? Sounds good to me. Permission to apply?
Comment 3 Matthias Clasen 2007-05-18 17:21:07 UTC
yeah, looks ok to me. I can see no reason to keep the frame. 
Comment 4 Tim Janik 2007-05-21 15:11:09 UTC
the hierarchy change can theoretically confuse third-party code that tries to mess with the popup window and has hard coded expectations about widget parents/children.
from an API perspective, the window is considered private though, so the chances for such breakage are small and should be considered a third-party bug.
other possible breakage are gtkrc files, which aim to theme the popup window and have "GtkFrame" in their path pattern. the expected breakage here is mild though and can easily be fixed in third-party themes.

the patch itself looks good, so please:
a) apply the patch to trunk;
b) outline the need to remove "GtkFrame" from rc-file paths intended to theme combobox popups in the NEWS file.
Comment 5 Carlos Garnacho 2007-05-21 15:55:25 UTC
Created attachment 88553 [details] [review]
updated patch

Hmm, I think I found a situation where it could make a visual difference, when using high contrast themes (which also use a quite visible xthickness/ythickness), the frame can be seen as a border in the popup window. I'm not sure whether the frame was intended to achieve this, but I think it's better to set shadow in the scrolled window.

This new patch also modifies gtk_combo_box_list_position, which seemed to align the contents of popup_frame with cell_view_frame (I noticed this setting exagerated xthickness), this shouldn't be needed anymore.
Comment 6 Carlos Garnacho 2007-05-21 16:45:46 UTC
Gosh, thanks for both reviews btw :), I was also pointed out that it'd be good to check whether this patch makes any visual difference with the ms-windows theme.
Comment 7 Cody Russell 2007-05-21 18:50:59 UTC
It looks good on ms-windows to me.
Comment 8 Behdad Esfahbod 2007-06-12 05:37:27 UTC
Updated patch needs review.
Comment 9 Tim Janik 2007-06-13 13:40:26 UTC
(In reply to comment #5)
> Created an attachment (id=88553) [edit]
> updated patch
> This new patch also modifies gtk_combo_box_list_position, which seemed to align
> the contents of popup_frame with cell_view_frame (I noticed this setting
> exagerated xthickness), this shouldn't be needed anymore.

ok, patch still looks good. but lacks the Changes/NEWS rc-file documentation that i pointed out earlier.

(In reply to comment #7)
> It looks good on ms-windows to me.

ok, thanks Cody.

Carlos, can you please add the missing docs and then apply?
Comment 10 Carlos Garnacho 2007-06-13 23:19:13 UTC
Thanks :), I've just committed the patch and updated NEWS,

2007-06-14  Carlos Garnacho  <carlos@imendio.com>

        * gtk/gtkcombobox.c: removed unused and hardly visible GtkFrame from
        the menu widget hierarchy when ::appears-as-list is TRUE. (#435471)
        * NEWS: add a note about repercussions of this change to RC files.