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 619421 - Margin preview icon in Page setup dialog is not generated using the available space for it
Margin preview icon in Page setup dialog is not generated using the available...
Status: RESOLVED FIXED
Product: Gnumeric
Classification: Applications
Component: GUI
1.10.x
Other Linux
: Normal normal
: ---
Assigned To: Jody Goldberg
Jody Goldberg
Depends on:
Blocks:
 
 
Reported: 2010-05-23 11:09 UTC by Sameer Morar
Modified: 2010-05-26 07:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch to fix this (8.66 KB, patch)
2010-05-23 11:12 UTC, Sameer Morar
needs-work Details | Review
Revised patch (8.58 KB, patch)
2010-05-23 23:45 UTC, Sameer Morar
needs-work Details | Review
Revised patch #2 (8.38 KB, patch)
2010-05-24 12:07 UTC, Sameer Morar
none Details | Review
Patch #2 (8.40 KB, patch)
2010-05-24 12:16 UTC, Sameer Morar
committed Details | Review
Patch to glade file that deals with items moving apart + clipping bug (1.57 KB, patch)
2010-05-26 02:25 UTC, Sameer Morar
committed Details | Review

Description Sameer Morar 2010-05-23 11:09:06 UTC
The size values that the margin preview icon is generated from is hard coded.
As font size, and other factors change relating to the surrounding widget sizes, this icon will not scale appropriately.
Comment 1 Sameer Morar 2010-05-23 11:12:29 UTC
Created attachment 161774 [details] [review]
Proposed patch to fix this
Comment 2 Andreas J. Guelzow 2010-05-23 18:30:16 UTC
Review of attachment 161774 [details] [review]:

+	guint widths[4];
+	for (i = 0; i < table->ncols; i++){
+		widths[i] = 0;
+	}

While at this time table->ncols may be 4 or less this invites potential trouble later on. 

similarly or heights[9].


GValue is an opaque structure (and GTK has been known to change things that weren't even so marked.) So 
+	GValue *top_att, *bottom_att, *left_att, *right_att;
...
+		if ((uint) left_att + 1 == (uint) right_att){

is completely unacceptable. You need to use g_value_get_uint ().

Personally I would prefer  margin_preview_page_available_size to return void and take an MarginPreviewPageAvailableSize* argument.
Comment 3 Sameer Morar 2010-05-23 23:45:21 UTC
Created attachment 161817 [details] [review]
Revised patch

I have made the following changes to deal with the points above:

1) guint widths and heights arrays are now 20 items long.

2) No longer use GValue pointers to access container properties.

3) margin_preview_page_available_size now returns void and takes an MarginPreviewPageAvailableSize* argument.
Comment 4 Andreas J. Guelzow 2010-05-24 00:28:20 UTC
Review of attachment 161817 [details] [review]:

1) guint widths and heights arrays are now 20 items long.

the same argument applies. While it may be less likely to run out of space, you should really allocate exactly as much space as is really needed. (g_new)

2) No longer use GValue pointers to access container properties.
It hadn't even occurred to me that you were effectively casting gint to GValue * and then back to guint...
"top-attach" and friends properties are gint not guint, you should use the right type.
Comment 5 Sameer Morar 2010-05-24 12:07:54 UTC
Created attachment 161856 [details] [review]
Revised patch #2

1) Done. I use g_new0 so that I don't have to initialize the array to zeros, and I clean up with g_free.

2) According to: http://library.gnome.org/devel/gtk/2.20/GtkTable.html "top-attach" and friends properties are guint.
Comment 6 Sameer Morar 2010-05-24 12:16:19 UTC
Created attachment 161857 [details] [review]
Patch #2

Botched upload - trying again.
Comment 7 Andreas J. Guelzow 2010-05-25 06:55:09 UTC
You are of course right that "top-attach" and friends properties are guint. When I looked them up I ended up at the child properties of gtk_menu that have the same name.
Comment 8 Andreas J. Guelzow 2010-05-25 07:10:04 UTC
When I enlarge the page setup dialog the portrait/landscape/reverse landscape, etc items move apart. Since you made sure that the margin spin buttons did not move apart it would be nice if you could do the same for the orientation/center items.
Comment 9 Andreas J. Guelzow 2010-05-25 07:16:54 UTC
Review of attachment 161857 [details] [review]:

nice work (but there are still many other little things to improve with the page setup dialog)
Comment 10 Andreas J. Guelzow 2010-05-25 07:17:13 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Comment 11 Morten Welinder 2010-05-26 01:08:12 UTC
...
dialog-printer-setup.c:84: error: expected specifier-qualifier-list before 'uint'
dialog-printer-setup.c: In function 'margin_preview_page_available_size':
dialog-printer-setup.c:412: error: 'MarginPreviewPageAvailableSize' has no member named 'width'
dialog-printer-setup.c:413: error: 'MarginPreviewPageAvailableSize' has no member named 'height'
dialog-printer-setup.c:445: error: 'uint' undeclared (first use in this function)
dialog-printer-setup.c:445: error: (Each undeclared identifier is reported only once
...
Comment 12 Andreas J. Guelzow 2010-05-26 01:34:27 UTC
Hmm, I don't see that.
Comment 13 Sameer Morar 2010-05-26 01:40:28 UTC
Nor do I. Morten, does your compiler support 'uint' at all? The only thing that I can see that may be a problem is that i used uint instead of guint.

I'm busy putting together a patch that replaces uint with guint, and fixes the items from comment 8.
Comment 14 Andreas J. Guelzow 2010-05-26 01:46:41 UTC
I have changed the uint to guint which might fix what you are seeing (and which it should have been in the first place). Please check again.
Comment 15 Andreas J. Guelzow 2010-05-26 01:48:28 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Comment 16 Andreas J. Guelzow 2010-05-26 01:49:48 UTC
Sameer, Mingw32 does not support uint, so we should not be using it.
Comment 17 Sameer Morar 2010-05-26 02:25:49 UTC
Created attachment 161987 [details] [review]
Patch to glade file that deals with items moving apart + clipping bug

1) As per comment 8, this patch disables the portrait/landscape/reverse landscape,etc items from moving apart.

2) Work around a weird clipping bug with an embedded canvas widget in a table cell whose child's packing properties is not set to fill. This seems to be triggered when the margin preview widget draws the landscape page preview.
Comment 18 Andreas J. Guelzow 2010-05-26 07:56:06 UTC
Review of attachment 161987 [details] [review]:

committed as is
Comment 19 Andreas J. Guelzow 2010-05-26 07:56:22 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.