GNOME Bugzilla – Bug 508563
Migrate from libgnomeprint to gtk-print
Last modified: 2011-08-04 08:11:46 UTC
libgnomeprint is dead, gtk-print (in gtk as of (I think) 2.10) is the new way (supported and also much nicer user experience). This is an enhancement request to migrate ghex from libgnomeprint to gtk-print. See http://www.gnome.org/~alexl/presentations/guadec2006-printing.pdf for details of this new API.
*** Bug 590993 has been marked as a duplicate of this bug. ***
Created attachment 152942 [details] [review] patch removing custom gnomeprint widget
Hi Rafał, can you please provide a Git formatted patch, as described at http://live.gnome.org/Git/Developers ? Thanks in advance!
Created attachment 153034 [details] [review] drop custom gnome-print widget I didn't noticed not being CC'ed. I'd say that former one was git-formatted enough, but :roll:... First one is the old one.
Created attachment 153035 [details] [review] move to gtk print This one drops gnome-print altogether, still needs polish and probably leaks a bit, but print preview works (I don't have a real printer) and I'm slowly starting to understand gtkprint (still unsure how exactly pango units and GTK_UNIT_POINTS relate). The one in between simply removes src/gnome-print-font-dialog.{c,h}.
After branching (for gnome-2-32?), surely this can just go into master? It can't be worse than the current use of gnome-print, which makes it near useless in GTK+3-land.
I agree, but it still depends on bonobo so it's not going to build in 3.0 before that's been fixed.
Review of attachment 153034 [details] [review]: Looks good, except for some minor issues. Can you git rm gnome-print-font-dialog.c and gnome-print-font-dialog.h files and also remove these from po/POTFILES.in? >+ pui->df_button = gtk_font_button_new_with_font("Monospace 14"); Should use def_font_name instead of hardcoding "Monospace 14" >+ pui->hf_button = gtk_font_button_new_with_font("Monospace 14"); Likewise. >@@ -382,12 +373,12 @@ void set_current_prefs(PropertyUI *pui) { > } > > if(header_font_name) >- gnome_print_font_picker_set_font_name >- (GNOME_PRINT_FONT_PICKER(pui->hf_button), >+ gtk_font_button_set_font_name >+ (GTK_FONT_BUTTON(pui->hf_button), > header_font_name); > if(data_font_name) >- gnome_print_font_picker_set_font_name >- (GNOME_PRINT_FONT_PICKER(pui->df_button), >+ gtk_font_button_set_font_name >+ (GTK_FONT_BUTTON(pui->df_button), > data_font_name); > if(def_font_name) > gtk_font_button_set_font_name(GTK_FONT_BUTTON(pui->font_button), Please indent the two new gtk_font_button_set_font_name() calls the same way like the 3rd, already existing gtk_font_button_set_font_name() call.
The catch is def_font_name is "Monospace 12" and I didn't want to change the original size.
It doesn't really matter much if the default is "Monospace 12" or "Monospace 14"; it's just a fallback. The actual font is instead taken from from gconf: <schema> <key>/schemas/apps/ghex2/default-data-font</key> <applyto>/apps/ghex2/datafont</applyto> <owner>ghex2</owner> <type>string</type> <locale name="C"> <default>Courier 10</default> <short>Default Data Font</short> <long>Default Data Font</long> </locale> </schema> <schema> <key>/schemas/apps/ghex2/default-header-font</key> <applyto>/apps/ghex2/headerfont</applyto> <owner>ghex2</owner> <type>string</type> <locale name="C"> <default>Helvetica 12</default> <short>Default Header Font</short> <long>Default Header Font</long> </locale> </schema>
Created attachment 192368 [details] [review] old patch updated to master Never mind, seems I misunderstood the point of that call, back when I wrote that patch.
Review of attachment 192368 [details] [review]: Thanks, pushed to master as 69b2279.
Review of attachment 153035 [details] [review]: This looks like a very nice patch. I haven't really taken a close look at it yet, but a quick glance revealed some commented out lines in print.c and a block of code disabled with #if 0 in ui.c. Is it something you wanted to work on later but didn't get to yet? Do you think you could clean that up and also rebase the patch to current master? ::: configure.in @@ +42,3 @@ libbonoboui-2.0 \ libgnomeui-2.0 >= 2.6.0 \ + gtk+-unix-print-2.0) Is the dependency on gtk+-unix-print-2.0 really necessary? I can't see any unix-only code in the patch. ::: src/print.h @@ +26,3 @@ +#include <gtk/gtk.h> +#include <gtk/gtkunixprint.h> Same here, is including gtkunixprint.h necessary? I would have assumed that <gtk/gtk.h> is enough for printing functionality.
Actually, you're right here - gtk+-unix-print-2.0 isn't necessary. As for the '#if 0' block: the answer is "yes and no". Due to design differences between gnome-print and gtk-print, most of those functions were either unnecessary or were merged into the remaining ones - solitary exception was ghex_print_progress which was on my TODO list, but I've never got to it. Now, print.c is a bit longer story: g_print are obviously my personal debug, progress_func is both a leftover from gnome-print and the other reminder about ghex_print_progress, box_adjustment_* and print_verify_fonts are just something I forgot to clean up. I'll clean it up and rebase it shortly - though I don't think I'll get the ghex_print_progress done yet.
Created attachment 192946 [details] [review] move to gtk_print, rebased Cleaned it up a bit (still left the one '#if 0...#endif' block, though I'm beginning to suspect it's actually not needed, as I think I saw a native progress dialog during test). Fixed a minor issue with page number position in the process. Slipped in 'foreign' parameter to automake (after all, most gnome packages use it). On that note: is there some way to make fairly sure, that the font selected for view/print is monospaced ? After all, variable width doesn't really make sense here. On an unrelated note, I'm beginning to wonder just why exactly evince can't be fixed in non-continuous view to handle PageUp/PageDown consistently after all these years.
Review of attachment 192946 [details] [review]: First of all I'd like to thank you for the patch. The original GHex printing code is hairy and your patch makes it much nicer. > still left the one '#if 0...#endif' block, though I'm > beginning to suspect it's actually not needed, as I think I saw a native > progress dialog during test Yes, gtk_print_operation_set_show_progress() affects whether GTK+ shows the progress dialog. So it would appear you can get rid of the custom progress dialog code, unless of course you want to do a prettier dialog than GTK+ has. > Slipped in 'foreign' parameter to automake (after all, > most gnome packages use it). What good does it do like that? I would understand adding it if you wanted to remove the empty NEWS file and the outdated ChangeLog file -- in that case 'foreign' would be needed to make automake stop complaining about missing files. But currently it doesn't seem to have any effect. In any case, make it a separate patch please. Anybody doing git history digging will appreciate small, self-contained patches. Some nitpicking about code style. In the patch, some of the new code has a space before the opening parentheses and some of the code does not. I would prefer if new code in GHex consistently used a space before the left parentheses, as this is what the rest of the GNOME is doing. Just running the patch through sed -i -e '/^+/ s/\b(/ (/' patchname.patch should fix it. ::: src/print.c @@ +45,3 @@ gchar *data_font_name, *header_font_name; gint shaded_box_size; +static PangoLayout *layout; Why is it static global variable? I would prefer if it was a local variable declared at the local function scope. @@ +57,3 @@ guint max_row); static void print_shaded_box( GHexPrintJobInfo *pji, guint row, guint rows); +//static gboolean print_verify_fonts (void); The function is gone so please remove the prototype too. @@ +61,3 @@ static void print_header(GHexPrintJobInfo *pji, unsigned int page) { + cairo_t* cr = gtk_print_context_get_cairo_context(pji->pc); Some nitpicking: the rest of the GHex uses * before the variable name, so instead of 'cairo_t* cr', could you use 'cairo_t *cr' ? @@ +69,3 @@ + gint height, length; + + layout = gtk_print_context_create_pango_layout (pji->pc); The 'layout' created here isn't released anywhere. @@ +78,2 @@ /* Print the file name */ + layout = gtk_print_context_create_pango_layout (pji->pc); The 'layout' created here isn't released anywhere. @@ +82,3 @@ + pango_layout_set_indent (layout, 0); + pango_layout_get_pixel_size(layout, &length, &height); + y = height; // + pji->margin_top The commented out '+ pji->margin_top' can probably be removed. @@ +89,2 @@ /* Print the page/pages */ + layout = gtk_print_context_create_pango_layout (pji->pc); The 'layout' created here isn't released anywhere. @@ +206,3 @@ + gdouble box_bottom = 0; + gdouble box_right = 0; + gdouble box_left = 0; Setting all the variables to 0 and then setting them all over directly below seems unnecessary and a bit confusing; I'd just skip the zeroing here. @@ +219,3 @@ + cairo_line_to(cr, box_left, box_bottom); + cairo_line_to(cr, box_right, box_bottom); + cairo_line_to(cr, box_right, box_top); Instead of doing cairo_move_to and 4x cairo_line_to, I'd suggest using cairo_rectangle. @@ +221,3 @@ + cairo_line_to(cr, box_right, box_top); + cairo_stroke(cr); + cairo_fill(cr); cairo_stroke() directly before cairo_fill() makes the latter a no-op. If you are drawing shaded boxes, then the cairo_stroke() call can probably just be removed. @@ +222,3 @@ + cairo_stroke(cr); + cairo_fill(cr); + cairo_set_source_rgb(cr, 0.0, 0.0, 0.0); This function having a side effect of setting colour to black can be confusing. Instead of: cairo_set_source_rgb (cr, 0.90, 0.90, 0.90); ... cairo_set_source_rgb (cr, 0.0, 0.0, 0.0); I would use cairo_save/cairo_restore pairs to restore cr in the state that the caller had: cairo_save (cr); cairo_set_source_rgb (cr, 0.90, 0.90, 0.90); ... cairo_restore (cr); @@ +320,3 @@ + g_object_unref (layout); + + setup = gtk_page_setup_new(); 'setup' created here isn't released anywhere. @@ +323,3 @@ + gtk_page_setup_set_paper_size_and_default_margins(setup, gtk_print_settings_get_paper_size(pji->config)); + pji->printable_height = gtk_page_setup_get_page_height(setup, GTK_UNIT_POINTS); + pji->printable_width = gtk_page_setup_get_page_width(setup, GTK_UNIT_POINTS); There's a much easier way of getting the width and height: gtk_print_context_get_width (pji->pc) gtk_print_context_get_height (pji->pc) Could even get rid of pji->printable_width and pji->printable_height and just call gtk_print_context_get_* directly in the functions where you need to use them. @@ +329,3 @@ + pji->font_char_width)) * pji->gt + /((3*pji->gt + 1) * + pji->font_char_width); I don't really understand the calculation here. Was it copied from earlier code? If it's something new and you understand what it does, then it would be nice to split it up a bit and use descriptive variable names for subresults. e.g. '((3*pji->gt + 1) * pji->font_char_width)' could be assigned to a separate variable. At the very least, could you fix the indentation in the calculation? I'd suggest using 4 spaces in new functions instead of tabs so that the diffs look better. (Sorry for nitpicking about indentation again.) @@ -425,3 @@ g_return_if_fail(pji->pc != NULL); - for(i = pji->page_first; i <= pji->page_last; i++) { If you remove the loop, please also fix indentation down below. ::: src/print.h @@ +36,3 @@ + GtkPrintOperation *master; + GtkPrintContext *pc; + GtkPrintSettings *config; Perhaps rename config -> settings, now that it points to GtkPrintSettings instead of GnomePrintConfig. ::: src/ui.c @@ +762,1 @@ + if (result == GTK_PRINT_OPERATION_RESULT_ERROR) g_print("%s\n", error->message); This is leaking GError, should call g_error_free (error) here.
I'm working on an updated patch. In the meanwhile: - 'foreign' i.e. stops warnings from automake about gnome-doc-utils.make (probably the very reason gnome packages add it) - 'cairo_t* cr': will change it, but you should say "most of the rest" - that style was in the functions I modified - it actually leaks less than I thought - as noted, the original patch was more a proof-of-concept, than anything else - as for pji->bytes_per_row value, it's nothing new, it was simply in a different function earlier - honestly, I think I've simplified it from the previous form, though I need to think for awhile, to recall what I figured out it to be
Created attachment 193211 [details] [review] move to gtk-print, corrected I think I've addressed most of the issues - right now I'm checking if all three of operation, context and settings really need to be contained in the _GHexPrintJobInfo struct, chances are not. Feel free to ignore the 'foreign' block, if you're not convinced. This time, I've decided to enable the page size part of the dialog - seems to work nicely.
(In reply to comment #17) > - 'foreign' i.e. stops warnings from automake about gnome-doc-utils.make > (probably the very reason gnome packages add it) I have been under the impression that GNOME packages have added 'foreign' to stop automake from complaining about missing ChangeLog and NEWS files: http://www.openismus.com/documents/linux/automake/automake To check your theory about gnome-doc-utils warning, I ran two different builds (./autogen.sh && make) from fresh git checkout, one without 'foreign' and 2nd one with 'foreign'. The only difference in these two runs was a line about installing 'INSTALL'. Am I missing something obvious here? @@ -87,7 +87,6 @@ configure.ac:8: installing `./install-sh' configure.ac:8: installing `./missing' src/Makefile.am: installing `./depcomp' -Makefile.am: installing `./INSTALL' Running ./configure --enable-maintainer-mode ... checking for a BSD-compatible install... /usr/bin/install -c checking whether build environment is sane... yes (In reply to comment #18) > Feel free to ignore the 'foreign' block, if you're not convinced. I am sure of one thing -- it should be a separate commit, not mixed up with the print stuff.
Review of attachment 193211 [details] [review]: Looks good, pushed to master as 8594b560.
(In reply to comment #18) > I think I've addressed most of the issues - right now I'm checking if all three > of operation, context and settings really need to be contained in the > _GHexPrintJobInfo struct, chances are not. I bet there are a lot of nice clean up opportunities in the rest of the printing code. Just post follow-up patches please. Closing the ticket. Big thanks to Rafał again!