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 521442 - x/y thickness is being overriden by the combobox realize function
x/y thickness is being overriden by the combobox realize function
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
2.12.x
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 461805
 
 
Reported: 2008-03-09 19:50 UTC by Alberto Ruiz
Modified: 2008-03-16 02:17 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22


Attachments
Proposed fix (556 bytes, patch)
2008-03-09 19:52 UTC, Alberto Ruiz
committed Details | Review
Combobox entry before and after the patch. (11.56 KB, image/png)
2008-03-10 09:56 UTC, Alberto Ruiz
  Details
fixes thickness awareness for the child and the frame within the combobox (1.23 KB, patch)
2008-03-12 08:27 UTC, Alberto Ruiz
none Details | Review
This patch covers the GtkContainer::border-width property case (2.64 KB, patch)
2008-03-12 11:24 UTC, Alberto Ruiz
needs-work Details | Review
Second take of the patch (2.64 KB, patch)
2008-03-13 10:04 UTC, Alberto Ruiz
none Details | Review
Third take of the patch (2.64 KB, patch)
2008-03-14 01:15 UTC, Alberto Ruiz
accepted-commit_now Details | Review

Description Alberto Ruiz 2008-03-09 19:50:04 UTC
Please describe the problem:
The combobox realize function ignores thickness, for some reason this bug is not exposed in the most common engines, but on the windows ones it makes combo_box_entry to draw on top of the shadow.

Steps to reproduce:


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Alberto Ruiz 2008-03-09 19:52:02 UTC
Created attachment 106927 [details] [review]
Proposed fix
Comment 2 Matthias Clasen 2008-03-10 03:11:05 UTC
Hmm, hard to say if this is correct. I don't see any visible difference when using this patch with testcombo and large x/ythickness.

Can you show a screenshot of the difference ?
Comment 3 Alberto Ruiz 2008-03-10 09:56:49 UTC
Created attachment 106958 [details]
Combobox entry before and after the patch.
Comment 4 Matthias Clasen 2008-03-10 14:03:42 UTC
Thanks, looks right then. 

Please commit to both branches then.
Comment 5 Matthias Clasen 2008-03-12 02:19:36 UTC
2008-03-11  Alberto Ruiz  <aruiz@gnome.org>

        * gtk/gtkcombobox.c (gtk_combo_box_size_allocate):
        Take thickness into account in the size allocation of the child widgets in
        list mode.
        Fixes #521442
Comment 6 Alberto Ruiz 2008-03-12 08:25:19 UTC
It turns out that the patch didn't fixed the whole problem:

There are two different scenarios depending on the "has-frame" value.

   * If it has frame, the child should be aware of the thickness of the frame. And the frame should be aware of the thickness of the combobox.

   * If it has no frame, the child should ignore the thickness of the frame and use the one from the combobox.

The current patch totally ignores the first scenario and allways adds the combobox thickness directly.

I'm going to propose a patch that fully fixes all my issues, but I think that there are open questions:

The allocation assumes that if there is no shadow for the combobox, thickness is equals to 0, is that a correct assumption generally speaking?

We're ignoring the ComboBox border at the moment (and in the patch that I'm going to attach I'm ignoring the frame border as well), my gut feeling is that I'm doing it wrong, but need some input.
Comment 7 Alberto Ruiz 2008-03-12 08:27:26 UTC
Created attachment 107124 [details] [review]
fixes thickness awareness for the child and the frame within the combobox
Comment 8 Alberto Ruiz 2008-03-12 11:24:34 UTC
Created attachment 107136 [details] [review]
This patch covers the GtkContainer::border-width property case

I would say that this looks as the real patch.
I tried to use gtk_container_set_border_width and it just does the right thing.
Comment 9 Matthias Clasen 2008-03-12 15:45:15 UTC
+          child.width = MAX (1, child.width) - delta_x * 2;
+          child.height = MAX (1, child.height) - delta_y * 2;

This cannot be right, it defeats the point of doing the MAX in the first place.
Probably more like

MAX(1, child.width - delta_x * 2)
Comment 10 Alberto Ruiz 2008-03-13 10:04:56 UTC
Created attachment 107205 [details] [review]
Second take of the patch
Comment 11 Benjamin Berg 2008-03-13 11:10:04 UTC
Alberto, you probably did not mean to write:
 MAX (1, child.height - delta_y) * 2;
did you? :-)
Comment 12 Alberto Ruiz 2008-03-13 12:13:02 UTC
Benjamin,
yes I did, that's why the patch I submited right before your comment was fixing that :P
Comment 13 Alberto Ruiz 2008-03-14 01:15:25 UTC
Created attachment 107261 [details] [review]
Third take of the patch

Benjamin, you were right, I wasn't meaning that.
Comment 14 Matthias Clasen 2008-03-15 01:20:08 UTC
I assume these later assignments:

+              child.width -= delta_x * 2;
+              child.height -= delta_y * 2;

probably also need the MAX(1, ...) treatment ?
Comment 15 Alberto Ruiz 2008-03-15 02:18:12 UTC
The max thing is already in place, look at the very end of the patch text.

The idea is not to call MAX everytime, but just at the end where it's really needed.
Comment 16 Matthias Clasen 2008-03-15 02:25:32 UTC
Ah, right. 

Then it is probably fine.
Comment 17 Alberto Ruiz 2008-03-16 01:43:15 UTC
Revision 521442:

2008-16-03  Alberto Ruiz <aruiz@gnome.org>

        * gtl/gtkcombobox.c: (gtk_combo_box_size_allocate) The child is now aware of
        both the combobox and frame (if has-frame is set) thickness and border. (bug #521442)

Comment 18 Alberto Ruiz 2008-03-16 02:17:49 UTC
I'll send an email to gnome-themes-list to make sure that there are no regresions out of this fix. If everything seems fine, I'll backport it to 2.12.