Re: printing page ranges patch

[Martin Kretzschmar <m_kretzschmar gmx net>]

Hi Amaury,

Thanks for the patch. Comments inline.

Am Dienstag, den 22.02.2005, 10:32 +0100 schrieb Amaury Jacquot:
>This patch allows printing either the whole document, or a particular 
>range of pages


>Index: ChangeLog
>===================================================================
>RCS file: /cvs/gnome/evince/ChangeLog,v
>retrieving revision 1.194
>diff -u -r1.194 ChangeLog
>--- ChangeLog   18 Feb 2005 10:01:33 -0000      1.194
>+++ ChangeLog   22 Feb 2005 08:43:09 -0000
>@@ -1,3 +1,12 @@
>+2005-02-18  Amaury Jacquot  <sxpert esitcom org>
>+       
>+       * ev-print-job.c: (_EvPrintJob),
>+       (ev_print_job_use_print_dialog_settings), (idle_print_handler):
>+       * ev-window.c: (ev_window_print):
>+
>+       added the required bits to chose between printing the whole document
>+       or a particular range of pages
>+
> 2005-02-18  Tommi Vainikainen  <thv iki fi>
> 
>        * configure.ac (ALL_LINGUAS): Added "fi" (Finnish).
>Index: shell/ev-print-job.c
>===================================================================
>RCS file: /cvs/gnome/evince/shell/ev-print-job.c,v
>retrieving revision 1.2
>diff -u -r1.2 ev-print-job.c
>--- shell/ev-print-job.c        8 Jan 2005 19:02:48 -0000       1.2
>+++ shell/ev-print-job.c        22 Feb 2005 08:43:11 -0000
>@@ -49,7 +49,11 @@
>        gboolean duplex; /* FIXME unused */
>        int copies; /* FIXME unused */
>        int collate; /* FIXME unsued */
>-
>+       /* range printing */
>+       GnomePrintRangeType range_type;
>+       int first_page;
>+       int last_page;
>+       
>        int fd;
>        char *temp_file;
>        guint idle_id;
>@@ -215,7 +219,7 @@
> ev_print_job_use_print_dialog_settings (EvPrintJob *job, GnomePrintDialog *dialog)
> {
>        GnomePrintConfig *print_config;
>-
>+       
>        g_return_if_fail (EV_IS_PRINT_JOB (job));

No whitespace changes please.

>        g_return_if_fail (GNOME_IS_PRINT_DIALOG (dialog));
> 
>@@ -225,6 +229,12 @@
>                                          &job->width, &job->height);
>        gnome_print_config_get_boolean (print_config,
>                                        GNOME_PRINT_KEY_DUPLEX, &job->duplex);
>+       
>+       /* get the printing ranges */
>+       job->range_type = gnome_print_dialog_get_range (dialog);
>+       if (job->range_type == GNOME_PRINT_RANGE_RANGE)
>+               gnome_print_dialog_get_range_page (dialog, &(job->first_page), &(job->last_page));
>+       
>

Please move the handling for PRINT_RANGE_ALL to this function and set
first_page to 1, last_page to ev_document_get_n_pages. That way the
switch statement below can be removed and all you have to do there is
set next_page. And you can remove priv->range_type completely.

>        gnome_print_config_unref (print_config);
> }
> 
>@@ -234,12 +244,23 @@
>        if (!job->printing) {
>                ev_ps_exporter_begin (EV_PS_EXPORTER (job->document),
>                                      job->temp_file);
>-               job->next_page = 1; /* FIXME use 0-based page numbering? */
>+               switch (job->range_type) {
>+               case GNOME_PRINT_RANGE_ALL:
>+                       job->next_page = 1; /* FIXME use 0-based page numbering? */
>+                       job->last_page = ev_document_get_n_pages (job->document);
>+                       break;
>+               case GNOME_PRINT_RANGE_RANGE:
>+                       job->next_page = job->first_page;
>+                       break;
>+               default: 
>+                       /* do nothing, these are not supported */
>+                       break;
>+               }
>                job->printing = TRUE;
>                return TRUE;
>        }
> 
>-       if (job->next_page <= ev_document_get_n_pages (job->document)) {
>+       if (job->next_page <= job->last_page) {
> #if 0
>                g_printerr ("Printing page %d\n", job->next_page);
> #endif
>Index: shell/ev-window.c
>===================================================================
>RCS file: /cvs/gnome/evince/shell/ev-window.c,v
>retrieving revision 1.69
>diff -u -r1.69 ev-window.c
>--- shell/ev-window.c   16 Feb 2005 19:06:40 -0000      1.69
>+++ shell/ev-window.c   22 Feb 2005 08:43:13 -0000
>@@ -853,6 +853,18 @@
>        print_dialog = gnome_print_dialog_new (job, _("Print"),
>                                               (GNOME_PRINT_DIALOG_RANGE |
>                                                GNOME_PRINT_DIALOG_COPIES));
>+
>+       /* FIXME: should be able to print the "current page". however, 
>+        * the "all" option should be selected by default. Haven't found how
>+        * to achieve that */
>+       gnome_print_dialog_construct_range_page (GNOME_PRINT_DIALOG(print_dialog), 
>+                                                (GNOME_PRINT_RANGE_ALL |
>+                                                 GNOME_PRINT_RANGE_RANGE),
>+                                                1,
>+                                                ev_document_get_n_pages (ev_window->priv->document),
>+                                                _("Current Page"), 
>+                                                _("Pages Range"));
>+                                               
>        gtk_dialog_set_response_sensitive (GTK_DIALOG (print_dialog),
>                                           GNOME_PRINT_DIALOG_RESPONSE_PREVIEW,
>                                           FALSE);

As we're not using "current page" yet, we could save some translator
work by not marking "Current Page" for translation.

Where you have "Pages Range", gpdf has just "Pages", that way the line
reads "Pages From [  ] To [  ]" I guess that's a matter of taste. Bryan?

The dialog layout in the print dialog is a little crowded (little room
between page range label and "From" label) (Did I ever file that bug?).

Therefore gpdf appends a " " to the "Pages" label (but with g_strconcat,
so that translators don't drop the " " accidentally). Please add it
here, too.

Regards,

Martin