GNOME Bugzilla – Bug 356947
[eog-ng] GtkPrint support
Last modified: 2007-01-05 03:28:40 UTC
I have cooked a small patch to remove from the eog-ng branch the unused dependence on libgnomeprint[ui] and use instead of it the high level GTK+ Printing API. It still needs some work, but I would like it to be reviewed so I can continue work on the eog-ng branch. Some issues: - There is no way for the user to resize the image nor move it in the page. Resizing the printing job is not going to work properly for the former. I guess I would need to add a dialog to be shown before the actual GtkPrintUnixDialog is shown, so the user can set there his preferences. Current implementation in HEAD and previous EOG versions sucks regarding this, as there is no way to adjust these settings. - I made the dependence on gtk+-unix-print-2.0 hard. As this is included in GTK+ 2.10 and EOG already depends on this version of the toolkit, I saw no reason for making printing support optional. Please comment on the code, so we can improve it and have printing support soon in this branch.
Created attachment 73102 [details] [review] use GtkPrint API
(In reply to comment #0) > - There is no way for the user to resize the image nor move it in the page. > Resizing the printing job is not going to work properly for the former. I > guess I would need to add a dialog to be shown before the actual > GtkPrintUnixDialog is shown, so the user can set there his preferences. How about making it THE dialog? This would make it possible to add a page preview and positioning options (something like Irfanview's printing dialog comes to my mind). This could possibly also avoid the additional "Page Setup" menu item in the File menu. > - I made the dependence on gtk+-unix-print-2.0 hard. As this is included in > GTK+ > 2.10 and EOG already depends on this version of the toolkit, I saw no reason > for making printing support optional. > Looks okay to me, but EOG should probably respect the lockdown keys as it does for saving.
Created attachment 74657 [details] [review] Use GtkPrint API and respect lockdown configuration Ok, this patch follows the lockdown configuration.
(In reply to comment #2) > How about making it THE dialog? This would make it possible to add a page > preview and positioning options (something like Irfanview's printing dialog > comes to my mind). > This could possibly also avoid the additional "Page Setup" menu item in the > File menu. > I was thinking of that too, but it seems to me that customizing the printing dialog would stop us to use the high level printing API, which makes the patch pretty simple. I will play a bit more with the API to see if it's possible without having to play more with low level features.
Ok, it is possible. Will play with this and post a updated patch when I have it.
Created attachment 74895 [details] [review] use a custom widget to edit the image printing properties This patch creates a custom widget which allows the user to define the printing size and position of the image. It will be added in a tab ("Image Settings") for the Printing dialog. I am planning to write a preview widget for this, where the user can drag the image easily and put it in the place he wants. Notes, as usual: - I added API to GtkImage, that I needed to use properly the cairo API to create printable surfaces. This is what I added: /** This function will transform to pixels in the format requested by Cairo, that is, ARGB32 with pre-multiplied alpha channel and stored native-endian. We use it for the printing support. Inspired on the The Gimp's printing code. The returned pixels should be freed once they are no longer needed. */ guchar* eog_image_get_pixels (EogImage *img, int *width, int *height, int *rowstride); - I haven't tested exhaustively if this code works with weird image formats. I would appreciate feedback about this (I already tested with gif, images with transparency, without it, photos). - About the wording and the dialog itself, I would appreciate comments, because there may be a lot of things that can be improved. - I am not sure about the organization of the new code, do you think it is ok?
Created attachment 74979 [details] [review] same patch but improved This is the same patch, but it factorizes some functions that were pretty similar. In the previous comment, when I said GtkImage, I wanted to say EogImage.
I just had a quick look at it and it seems to to a good job already :). Some ideas/comments: - We should not hardcode the unit to Inches. A GtkComboBox where the user can select between let's say Inch, cm and px (?) should do and also serves as an indicator of which unit is in use. - Make the label next to the scaling bar a GtkEntry (or even a Spinbutton?) too. This would allow a finer adjustment. - The automatic horizontal centering of the image doesn't seem to work correctly. It looks like it centers the left border of the image intead of the middle. But maybe that's just a problem with me :). - Maybe it's possible to put the printing itself into an EogJob so the the UI doesn't freeze while printing. That should be all I found for the moment. :)
(In reply to comment #8) > - The automatic horizontal centering of the image doesn't seem to work > correctly. It looks like it centers the left border of the image intead of the > middle. But maybe that's just a problem with me :). Yes, I realized of that yesterday. I will attach a patch that fixes this.
Created attachment 75011 [details] [review] fix for centering problems
(In reply to comment #10) > Created an attachment (id=75011) [edit] > fix for centering problems > No, it's now working yet. But I don't have an idea currently why it does, as the horzontal centering code is pretty much identical to the vertical one. Maybe it's because I'm currently using only PDF-printing to test (I should try a real printout).
On Thu, 2006-10-19 at 10:17 +0000, EOG (bugzilla.gnome.org) wrote: > We should not hardcode the unit to Inches. A GtkComboBox where the user can > select between let's say Inch, cm and px (?) should do and also serves as an > indicator of which unit is in use. Agreed, we definitively can't force every user to use inches. Would a combo box or radio buttons look better? > - Make the label next to the scaling bar a GtkEntry (or even a Spinbutton?) > too. This would allow a finer adjustment. > I don't think it's necessary. The precision level is already at 1 percent of the image size, what I think is enough for a simple image viewer as EOG. People who wants more precision, should either modify the printing size directly or use a more professional image editor, like gimp. Moreover, I wouldn't like to overload the UI, EOG is just an image viewer. >- Maybe it's possible to put the printing itself into an EogJob so the the UI >doesn't freeze while printing. Sounds interesting, though I think GTK+ itself provides asynchronous printing. Will play with it and see what results. I wonder if I may commit this code to the branch. Adding Lucas to the cc list.
(In reply to comment #11) > No, it's now working yet. But I don't have an idea currently why it does, as > the horzontal centering code is pretty much identical to the vertical one. > Maybe it's because I'm currently using only PDF-printing to test (I should try > a real printout). I double checked now if it works properly, and it does for me. The problem with attachment 74979 [details] [review] can be sumarized with this diff of the two latest patches: @@ -438,7 +438,7 @@ diff -u -p -r1.189.2.28 eog-window.c + dpi_y = gtk_print_context_get_dpi_y (context); + + x0 = left_margin * dpi_x; -+ y0 = left_margin * dpi_y; ++ y0 = top_margin * dpi_y; + + pixels = eog_image_get_pixels (image, + &pixbuf_width, &pixbuf_height, I am only doing previews and printing to PDF files, because I don't have a real printer, but results shouldn't be really different, right?
(In reply to comment #13) > I am only doing previews and printing to PDF files, because I don't have a real > printer, but results shouldn't be really different, right? > No, there shouldn't be. Interestingly it worked when I tried to make a PDF with a smaller image to upload here. It seems to happen under certain conditions only. Strange. (In reply to comment #12) > On Thu, 2006-10-19 at 10:17 +0000, EOG (bugzilla.gnome.org) wrote: > > We should not hardcode the unit to Inches. A GtkComboBox where the user can > > select between let's say Inch, cm and px (?) should do and also serves as an > > indicator of which unit is in use. > > Agreed, we definitively can't force every user to use inches. Would a combo box > or radio buttons look better? I think it depends on how many options we want to offer. If it's only for inch + mm (GTK_UNIT_PIXEL is afaik not working yet and GTK_UNIT_POINTS is not really useful for printing I think(?)) radio buttons should be sufficient. Otherwise a simple ComboBox would be better and easier to extend if that's ever needed. > > - Make the label next to the scaling bar a GtkEntry (or even a Spinbutton?) > > too. This would allow a finer adjustment. > > > > I don't think it's necessary. The precision level is already at 1 percent of > the image size, what I think is enough for a simple image viewer as EOG. People > who wants more precision, should either modify the printing size directly or > use a more professional image editor, like gimp. > > Moreover, I wouldn't like to overload the UI, EOG is just an image viewer. > Yes thats a point, but I find it easier to enter the relation by hand than by setting it with the mouse especially when printing several equally large images at once. (I guess I'm a little spoiled there) :)
This is wrong: >+ caption = eog_image_get_caption (window->priv->image); >+ gtk_print_operation_set_job_name (print, caption); >+ g_free (caption); Because eog_image_get_caption () returns a const *gchar that shouldn't be freed. This was producing memory corruption. I already fixed it in my local copy, but will send an updated patch when I finish working in the units switching code.
Created attachment 75323 [details] [review] allow to use millimeters or inches + several fixes This adds a combobox that allows the user to switch to and from millimeters. I also fixed what I described in previous comment and some other small things.
I'm still investigating why I'm unable to really center an image, but I noticed some other thing. The image size is limited to the page size. This makes the scaling slider look really broken when you use large images (I need to pull it down to ~40% before seeing any reaction of the numbers). The user should be allowed to print the image larger than the page size (which I think would theoretically make it possible to span an image over several pages and assemble it to a large poster). Maybe a button which sets the image back to page size would be an option? (at least until the preview widget is ready)
I'm having the feeling that the behavior I described in comment 17 is the reason why I can't really center the image on the page. The problem is that it somewhat mixes up width and height. Example: I want to print a 2304x1728px image on an A4 (210x297mm) page. The print dialog lets me scale down the image but the aspect ratio is wrong (width < height although it should be larger). I hope it's understandable what I mean. :)
Okay, I think I got it. The following change in eog-print-image-setup.c (I don't get a good diff currently; cvs doesn't take the new files into account) avoids setting the range of the height and width fields to the page_width: update_image_pos_ranges (self, page_width, page_height, width, height); gtk_spin_button_set_range (GTK_SPIN_BUTTON (priv->width), - 0, MIN (page_width, width)); + 0, width); gtk_spin_button_set_range (GTK_SPIN_BUTTON (priv->height), - 0, MIN (page_height, height)); + 0, height); Now I can properly center the image on the page :) I'm not yet sure if we need to adjust the range of the other fields too.
Created attachment 75739 [details] [review] updated patch The patch containing the little change I mentioned in comment 19
Created attachment 76086 [details] [review] Fixes in eog_image_get_pixels () This includes Felix's suggestions and fixes some issues with certain image sizes when getting their pixels in ARGB format. Now eog_image_get_pixels () should be sound.
Hmm, isn't it possible to use gdk_cairo_set_source_pixbuf in eog_window_print_draw_page()? It looks like it does the same as eog_image_get_pixels and directly adds the surface to the cairo context (which is currently done by hand).
Hey guys! Good job! I think the patch is already good enough to be commited in CVS. Polishing work should continue on repository. Some remaining issues: 1. Investigate issue commented (#22) by Felix; 2. Spin button width are variating when changing "Scaling". 3. Some of settings in Page Setup dialog seem to overlap with the ones in Page Setup tab in Print dialog. Is this separated page setup really needed? Did I miss something? Todo: visual inline editing and preview of image print settings.
Created attachment 76559 [details] [review] another updated patch This should contain a fix for the resizing spin buttons. It simply sets their size to 6 characters. That should be sufficient as we are limited ~32768 pixel (~410in or ~10000mm) wide images by GdkPixbuf (there is a bug report around). I hope I didn't miss a file in the patch. :)
Created attachment 76578 [details] [review] simple printing patch This patch adds printing support in a pretty simple and 'Just Works (tm)' way: If the image is bigger than the page, it scales it down. Otherwise, it simply centers it. I will commit if noone have issues with it, so we can work on improving the user experience within CVS. 45KBs patches are becoming unmaintainable.
(In reply to comment #22) > Hmm, isn't it possible to use gdk_cairo_set_source_pixbuf in > eog_window_print_draw_page()? > It looks like it does the same as eog_image_get_pixels and directly adds the > surface to the cairo context (which is currently done by hand). By the way, yes. You are right. I rewrote the cairo drawing part to use gdk_cairo_set_source_pixbuf (). The commited patch uses it.
Created attachment 78602 [details] [review] updated patch + EogPrintPreview This patch adds a EogPrintPreview widget which allows the user to set the image position by dragging it in the preview or using the arrow keys. I also fixed a lot of issues in the custom widget I found while writing the preview. Also, I changed printing behavior and now it's only allowed to print an image at most at page size. If an user tries to print an image whose natural size is bigger than the page, this will be scaled down to fit the page and the user won't be allowed to print it bigger. I think it is pretty clean, but please test and give me your feedback. There are probably many optimizations that can be done, but I would like to work on them in CVS, if this qualifies :-).
I just took a quick look and it looks good so far. The only thing I noticed that everything behaves a little odd when i use the scaler bar while the unit is set to mm.
Created attachment 78847 [details] [review] updated patch Thanks Felix for the pointer. I fixed that and also did some other minor improvements. Here is the latest version.
In a unrelated issue, I was thinking that maybe we should move the printing callbacks out from eog-window.c to another file, say eog-print-callbacks.c, to organize a bit better the printing code and avoid overloading eog-window.c, which is already huge. Comments?
Hi Claudio, very nice work! I'll move all eog_window_cmd_* from eog-window.c to eog-commands.c to solve this problem. Maybe it will solve this eog-window.c overloading problem. Also, eog-print-callbacks.c is a terrible name. :-) The patch is very good, I have just a few issues: Found those bugs: 1. When you move the image inside the page and the select any of the centering optins from the combo, "None" remains selected, not the one I chose. Even though the image is centered correctly. 2. Use stock_print-setup icon from g-i-t for the "Page Setup..." menu item (unrelated but would to fix it :-) ). Minor code issues: 1. No need to use libeog-marshal.list to define such marshaling code. Just use the default one from glib; 2. GObject style issue: G_DEFINE_TYPE call just after EOG_PRINT_IMAGE_SETUP_GET_PRIVATE definition just like other new EOG classes; 3. GObject style issue: Declare class structs before defining class constants; 4. Typo in eog-print-image-setup "The information for the page where the image will be print". "print" -> "printed"; 5. Code style issue: indent function parameters like other new EOG classes in eog-print-image-setup.h; 6. Same GObject style issues in eog-print-preview.c. Organize the exactly the same way of other EOG classes.
(In reply to comment #31) > Hi Claudio, very nice work! I'll move all eog_window_cmd_* from eog-window.c to > eog-commands.c to solve this problem. Maybe it will solve this eog-window.c > overloading problem. Also, eog-print-callbacks.c is a terrible name. :-) > > The patch is very good, I have just a few issues: > > Found those bugs: > 1. When you move the image inside the page and the select any of the centering > optins from the combo, "None" remains selected, not the one I chose. Even > though the image is centered correctly. Agreed. I will fix this. > 2. Use stock_print-setup icon from g-i-t for the "Page Setup..." menu item > (unrelated but would to fix it :-) ). This can be orthogonally fixed, so I opened bug #389314 for tracking it (I'm lazy today, and besides it's a good gnome-love bug). > > Minor code issues: > 1. No need to use libeog-marshal.list to define such marshaling code. Just use > the default one from glib; > 2. GObject style issue: G_DEFINE_TYPE call just after > EOG_PRINT_IMAGE_SETUP_GET_PRIVATE definition just like other new EOG classes; > 3. GObject style issue: Declare class structs before defining class constants; > 4. Typo in eog-print-image-setup "The information for the page where the image > will be print". "print" -> "printed"; > 5. Code style issue: indent function parameters like other new EOG classes in > eog-print-image-setup.h; > 6. Same GObject style issues in eog-print-preview.c. Organize the exactly the > same way of other EOG classes. I've locally fixed all this. Lucas, I wonder if this could go into HEAD. I would really love to see this in EOG 2.18, because I don't really see any reason for delaying it and leaving it only for eog-ng. What do you think? If you agree, then I would suggest we commit it and I take care of fixing the issue (1) in CVS (or SVN), to make sure all the new strings get translated, and to allow documenters to update related documentation before upcoming freezes.
Created attachment 79120 [details] [review] fix style issues and centering combobox This fixes all the issues mentioned by Lucas. The "printing-setup" icon is already added in SVN.
I fixed some small issues, and finally commited to trunk and eog-ng. Thanks to everyone for your feedback, testing, and coding! If you find issues, please do not hesitate to open new bugs for them. Yay! 2007-01-05 Claudio Saavedra <csaavedra@alumnos.utalca.cl> * src/Makefile.am: Add new files. * src/eog-print-image-setup.c: * src/eog-print-image-setup.h: New custom widget for the GtkPrint UI to set the image position and size in a printed page. * src/eog-print-preview.c: * src/eog-print-preview.h: Widget to interactively preview the printing of an image. * src/eog-window.c: (eog_window_print_draw_page), (eog_window_print_create_custom_widget), (eog_window_print_custom_widget_apply), (eog_window_print_end_print), (eog_window_print), (verb_HelpAbout_cb): Use the above widgets to set the image position and size when printing instead of hardcoding its size. Fixes bug #356947.