GNOME Bugzilla – Bug 724044
Can not print PostScript files as PDF
Last modified: 2014-03-27 14:53:07 UTC
Created attachment 268687 [details] Screenshot It appears that we can not print PostScript files as PDF (see attached screenshot). So, maybe we should not offer the PDF option in the dialog.
Created attachment 268688 [details] [review] ps: Don't advertize CAN_GENERATE_PDF
Review of attachment 268688 [details] [review]: ::: backend/ps/ev-spectre.c @@ +411,3 @@ EV_FILE_EXPORTER_CAN_COLLATE | EV_FILE_EXPORTER_CAN_REVERSE | + EV_FILE_EXPORTER_CAN_GENERATE_PS; This is not correct. These are the capabilities of the backend, and the PS backend can generate PDF files. Another thing is what the actual printer supports. I wonder why you got that error when printing a PS file as PDF. It's definitely another bug somewhere else.
Created attachment 270819 [details] [review] Allow printing to all formats supported by backend Hi, I've looked at the problem and it seems to me that the "if (!gtk_printer_accepts_ps (export->printer))" in the ev-print-operation.c is not correct any more. We should check whether requested file format is supported by the current backend. The attached patch implements this (I'm not sure about the warning message so propose something else if needed). Regards Marek
Review of attachment 270819 [details] [review]: Looks good to me in general, but I have some comments. Thanks ::: libview/ev-print-operation.c @@ +1178,3 @@ + else + format = EV_FILE_FORMAT_PS; + } I would move this to a helper function, ev_file_exporter_get_format_from_print_settings() or something like that. @@ +1187,3 @@ + gtk_printer_accepts_pdf (export->printer) && + ev_file_exporter_get_capabilities (EV_FILE_EXPORTER (op->document)) & + EV_FILE_EXPORTER_CAN_GENERATE_PDF))) { This expression is too complex. I don't think we need to check the backend capabilities here, because the options shown in the dialog are based on that already, so if we get PDF here is because either it was selected by the user in the dialog or fell back in previous expression that already checks backend capabilities. So, here we should only make sure that the actual printer supports the format selected. @@ +1193,3 @@ GTK_PRINT_ERROR, GTK_PRINT_ERROR_GENERAL, + _("Requested format is not supported by this printer or backend.")); We can't change strings now. We could move this change to a different patch. I don't think we should expose the backend error, because it shouldn't happen, if the backend doesn't support the format, it won't be exposed in the dialog.
Created attachment 272561 [details] [review] Allow printing to all formats supported by backend Thank you for the review. (In reply to comment #4) > Review of attachment 270819 [details] [review]: > > Looks good to me in general, but I have some comments. Thanks > > ::: libview/ev-print-operation.c > @@ +1178,3 @@ > + else > + format = EV_FILE_FORMAT_PS; > + } > > I would move this to a helper function, > ev_file_exporter_get_format_from_print_settings() or something like that. I've moved it to static _get_output_format() function. Moving it to ev-file-exporter.h would mean to add GTKUNIXPRINT_CFLAGS and GTKUNIXPRINT_LIBS to all parts which include the header file. But if you want to move it there I will do the move. > @@ +1187,3 @@ > + gtk_printer_accepts_pdf (export->printer) && > + ev_file_exporter_get_capabilities (EV_FILE_EXPORTER (op->document)) > & > + > EV_FILE_EXPORTER_CAN_GENERATE_PDF))) { > > This expression is too complex. I don't think we need to check the backend > capabilities here, because the options shown in the dialog are based on that > already, so if we get PDF here is because either it was selected by the user in > the dialog or fell back in previous expression that already checks backend > capabilities. > > So, here we should only make sure that the actual printer supports the format > selected. Done. > @@ +1193,3 @@ > GTK_PRINT_ERROR, > GTK_PRINT_ERROR_GENERAL, > + _("Requested format is not supported by > this printer or backend.")); > > We can't change strings now. We could move this change to a different patch. I > don't think we should expose the backend error, because it shouldn't happen, if > the backend doesn't support the format, it won't be exposed in the dialog. I've split the patch to 2 patches and removed the note about the backend.
Created attachment 272562 [details] [review] The warning change
(In reply to comment #5) > Created an attachment (id=272561) [details] [review] > Allow printing to all formats supported by backend > > Thank you for the review. > > (In reply to comment #4) > > Review of attachment 270819 [details] [review] [details]: > > > > Looks good to me in general, but I have some comments. Thanks > > > > ::: libview/ev-print-operation.c > > @@ +1178,3 @@ > > + else > > + format = EV_FILE_FORMAT_PS; > > + } > > > > I would move this to a helper function, > > ev_file_exporter_get_format_from_print_settings() or something like that. > > I've moved it to static _get_output_format() function. Moving it to > ev-file-exporter.h would mean to add GTKUNIXPRINT_CFLAGS and GTKUNIXPRINT_LIBS > to all parts which include the header file. But if you want to move it there I > will do the move. Sure, I meant to use a static function. > > > @@ +1187,3 @@ > > + gtk_printer_accepts_pdf (export->printer) && > > + ev_file_exporter_get_capabilities (EV_FILE_EXPORTER (op->document)) > > & > > + > > EV_FILE_EXPORTER_CAN_GENERATE_PDF))) { > > > > This expression is too complex. I don't think we need to check the backend > > capabilities here, because the options shown in the dialog are based on that > > already, so if we get PDF here is because either it was selected by the user in > > the dialog or fell back in previous expression that already checks backend > > capabilities. > > > > So, here we should only make sure that the actual printer supports the format > > selected. > > Done. > > > > @@ +1193,3 @@ > > GTK_PRINT_ERROR, > > GTK_PRINT_ERROR_GENERAL, > > + _("Requested format is not supported by > > this printer or backend.")); > > > > We can't change strings now. We could move this change to a different patch. I > > don't think we should expose the backend error, because it shouldn't happen, if > > the backend doesn't support the format, it won't be exposed in the dialog. > > I've split the patch to 2 patches and removed the note about the backend. Thanks!
Review of attachment 272561 [details] [review]: Thanks, please see my review commits and push after freeze. ::: libview/ev-print-operation.c @@ +1143,3 @@ +static EvFileExporterFormat +_get_output_format (EvFileExporter *exporter, We use the _ prefix only for private methods defined in headers, not for static methods. Use ev_file_exportrr_get_output_format() or just get_file_exporter_format() @@ +1206,3 @@ + + if (!((format == EV_FILE_FORMAT_PS && gtk_printer_accepts_ps (export->printer)) || + (format == EV_FILE_FORMAT_PDF && gtk_printer_accepts_pdf (export->printer)))) { I think it's easier to read something like if ((format == EV_FILE_FORMAT_PS && !gtk_printer_accepts_ps (export->printer)) || (format == EV_FILE_FORMAT_PDF && !gtk_printer_accepts_pdf (export->printer)) {
Review of attachment 272562 [details] [review]: Push it only to master after the freeze or ask for a string freeze break if you want to land it to stable too
Thank you for the review, I'll push it to master after freeze then.
I already branched, so you can push both patches to master now, but to stable only after the release.
Comment on attachment 272561 [details] [review] Allow printing to all formats supported by backend Thank you for the review. I've pushed this patch to master and gnome-3-12.
Comment on attachment 272562 [details] [review] The warning change I've pushed this one to master. Thank you for your help. Regards Marek