GNOME Bugzilla – Bug 583429
patch against evince to fix multipage even odd printing issues
Last modified: 2018-05-22 13:32:03 UTC
Please describe the problem: Note, this patch is against the git master branch but I couldn't test it with a running copy of the master branch because this is a production system and i can't run the dev version of gnome here. I have tested it on ubuntu against evince_2.26.1-0ubuntu1 and submitted a downstream patch on launchpad. (https://bugs.launchpad.net/bugs/378976) This patch fixes the bugs people are experiencing when printing: * multiple pages per sheet (lp107025) * even and odd sheets (lp107025) * multiple collated copies + multiple pages per sheet (lp282659) In this context it is important to know that it behaves likes lpr with n-up and even/odd options passed: if you have a document with 5 pages and want to print 2 pages per sheet, MANUAL duplex print odd sheets: {1,2} , {5} flip sheets and print even sheets: {3,4} IE: you need to make sure to remove the sheet with page 5 from the stack, evince will not produce empty sheets to fully automate the process. This would be nice, but it would mess with other things. Besides, it is the way multipage even & odd printing works on other platforms and the native GtkPrintOperation. ------------------ It does NOT solve bugs related to: * multiple pages per sheet + reverse order (lp377102) * the weird ordering of pages when pages_per_sheet = 6 (related to: lp328143) as that would require a complete rethinking of the way the output is assembled for printing. ------------------ It MAY solve: * evince prints multiple (collated) copies as one job * weird stapling behaviour with multiple copies ie: lp258364 I'm not sure about this because as far as I can tell the multiple copies are still sent as one print job, but by solving lp282659 we introduce blank spaces on sheets so the two copies are clearly separated, it depends on how the driver treats this. ---------------- Steps to reproduce: / Actual results: / Expected results: / Does this happen every time? / Other information: It would be great if a developer could take a look at it and comment. It would also be great if someone could enlighten me if the changes going into EvPrintOperation to transition to GtkPrintOperation will solve these bugs in the future. The reverse multipage printing functionality bug is especially nasty.
Created attachment 135078 [details] [review] patch to fix a number of multipage printing issues
I just realized that my printer (samsung ml-2010) has a duplex functionality (for instance: collated, copies=2, print all pages, long edge, pages per sheet=2) where the printer itself splits the job in two and waits for the user to reinsert the pages and push a button. I can confirm that this functionality is NOT broken by the patch and would therefore assume that fully automatic duplex printers are not broken either.
Comment on attachment 135078 [details] [review] patch to fix a number of multipage printing issues Hi Bartek, thank you very much for the patch. This patch would also fix bug #397225, right? Some comments about the patch below, >+ >+ // when printing multiple collated copies & multiple pages per sheet we want to >+ // prevent the next copy starting at the end of the previous one >+ // we therefore check whether we've reached the last page in a document >+ // if that is the case and the given sheet is not filled with pages, we introduce a few blank pages >+ // to finish off the sheet >+ // **export->skipped should not be included in this check!!** please, do not use C++ comments, use /* */ instead. >+ >+ // commented out because of evince bug http://bugzilla.gnome.org/show_bug.cgi?id=583388 >+ // I believe this end_page is extraneous, it causes blank pages to appear at the end >+ // of a document once the bug is fixed >+ /* >+ if (export->pages_per_sheet > 1 && >+ (export->total - 1 + export->skipped) % export->pages_per_sheet == 0) { >+ ev_file_exporter_end_page (EV_FILE_EXPORTER (op->document)); >+ >+ } // */ so, fixing bug #583388 just by adding the missing parantheses, together with this patch (with this code uncommented) would fix everything, right? > static void > ev_print_operation_export_init (EvPrintOperationExport *export) > { >+ export->skipped = 0; > } You don't need this, the whole EvPrintOperationExport structure is initialized to 0 when allocated.
Hi Carlos, thanks for your reply. Sorry about the comment style. I only included the comments to clarify what I did to the code. I think they should be more succinct anyway. When I fix bug 583388, in addition to my changes OR just on its own, I get 1 blank sheet added to the end of the printout when more than one page per sheet and an EVEN number of copies are printed. From the many tests I've done it appears that it is simply unnecessary. (I believe it is unnecessary because there is an end_page in export_job_finished which is called constantly) Initialization to zero: thanks, good to know. I'll clean it up a bit and rename skipped to blank I think, makes more sense.
Oh, about bug 397225, yes, that's the equivalent of lp107025, which in turn was the reason I wanted to fix this in the first place. I then realized that there we still things going awry with multiple multipage copies bleeding into eachother. I made the patch specifically for ubuntu at first because there's little chance that we'll see a new version using GtkPrintOperation added to 9.04
Created attachment 135238 [details] [review] patch to fix a number of multipage printing issues v2 I'm happy with this now, although the comment in the code is still quite lengthy.
Thank you very much, I've just applied the patch to both master and gnome-2-26 branches.
*** Bug 397225 has been marked as a duplicate of this bug. ***
Hi Carlos, I think we need to consider retracting the patch. I made some really silly thought mistakes and the patch fails to fix the following combination of settings: "custom ranges, multiple pages per sheet, multiple collated copies". I forgot to test ranges which didn't include the last page... I still think it improves upon the previous situation though. I'm not sure how to fix the problems short of making a few changes to the way the pages are assembled for printout. Essentially I'd like to add a guint to the export struct which tracks the sheet number. Sorry about this... I'll provide a proposition ASAP.
(In reply to comment #9) > Hi Carlos, > > I think we need to consider retracting the patch. I made some really silly > thought mistakes and the patch fails to fix the following combination of > settings: "custom ranges, multiple pages per sheet, multiple collated copies". > I forgot to test ranges which didn't include the last page... I didn't test ranges either... > I still think it improves upon the previous situation though. yes, absolutely. > I'm not sure how to fix the problems short of making a few changes to the way > the pages are assembled for printout. Essentially I'd like to add a guint to > the export struct which tracks the sheet number. take a look at gtk+ code, we might use most of the current gtk+ implementation http://git.gnome.org/cgit/gtk+/tree/gtk/gtkprintoperation.c > Sorry about this... no problem > I'll provide a proposition ASAP. > great, thank you very much.
Created attachment 135381 [details] [review] new attempt to fix even / odd multipage issues How about this? It leaves the structure of ev-print-operation largely intact and introduces only two new variables to the struct (while removing the blank counter). On the flipside, the added logic is UGLY. It uses the fact that all pages of all ranges are traversed anyway (in inc_page) and counts the total page number (those that are in the current sheet set, those that are not and blanks) to do end_page and begin_page and odd/even selection. It relies only on being provided with correct ranges, which is ensured by clamp_ranges.
With reference to the ugly logic in the patch: What speaks against an implementation which builds a model of ALL sheets before starting exportation? ( dynamically allocate a GPtrArray for the printout with GArrays of gints for the individual sheets, blank pages introduced at the end of a copy are set to "-1" ) This way all the jumping around to select the correct page for inclusion into the printout becomes unnecessary. (as all the logic is in one place) The actual exporting reduces to traversing the model by (odd/even/all) sheet, and each sheet by page. No special checks for when to do "begin_page" and "end_page" are necessary. The logic fits into two chunks ( one for collated, one for uncollated ) of about 60 lines each in a "build_sheets" function which is called after "clamp_ranges" in the dialog_response callback. ( I've done that, but I haven't implemented the actual export yet ) This approach would remove export_print_inc_page and bring all the "begin_page" and "end_page" into export_print_page. Of course, it would still export one page at a time to be non-blocking.
Again, can we have a unit test?
Hi Nickolay, I'll try to cook something up using dogtail and two test documents (one with an odd number of pages, the other one with an even number of pages). Will probably be useful for the future. I've never used it or written anything in python, so it might take a while. -Bartek
Hi Bartek. Actually there is no need to use either dogtail or python. It would be nice just to have a C test that will be linked with evince files, call function and check results. Though if it could be done with python easily it would be even greater. I just think that it would be very useful to fix the desired behaviour and check it, otherwise we'll never approach it.
Thanks for amazing contribution btw! :)
I see, hmm, what about the signalling which happens here? I can certainly build a custom "export" struct and imitate the callback to the dialog response signal in starting the export operation, but I don't know how to supply it with content. (In reply to comment #15) > Hi Bartek. > > Actually there is no need to use either dogtail or python. It would be nice > just to have a C test that will be linked with evince files, call function and > check results. Though if it could be done with python easily it would be even > greater. > > I just think that it would be very useful to fix the desired behaviour and > check it, otherwise we'll never approach it. >
Ok, this shouldn't be too hard using dogtail. However, there are 288 combinations in these couple of print options: copies = [1,2,3,4] collate = [0, 1] reverse = [0, 1] pages_per_side = [1,2,4,6,9,16] only_print = ["All sheets","Even sheets","Odd sheets"] Looking through those will be a lot of work! Keeping in mind that there will actually be 2x288 because proper testing needs to include a document with an even number of pages and one with an odd number of pages. I don't see any other way of testing this other than actually generating all the output because the placement of end_page and begin_page must be just right (otherwise you get blank sheets where there shouldn't be any, or pages print on top of eachother, and so on). If you know a way to automate it throw me a hint please. -B.
Hm, you certainly don't need to test all combinations, just a few common ones. Like collate 0 1 copies 1 3 pages 1 4 even/all that would be only 16 variants. Probably some of them aren't useful as well. Anyhow it was just an idea, there is no strict preference.
No you need to test more combinations. But I agree, testing a subset is sufficient if the test files and range-sets are chosen wisely. I've opened bug 583976 for inclusion of my test suite/case into git.
(In reply to comment #11) > Created an attachment (id=135381) [edit] > new attempt to fix even / odd multipage issues > Sorry for the late reply, I've been quite busy these days. A few comments on your patch: - There are still coding style issues, for example: + if( export->collate ) { should be + if (export->collate) { do not leave spaces there, please. - Commented code: + /* in the way this currently works, there's not harm in an extra begin_page */ + //ev_file_exporter_begin_page (EV_FILE_EXPORTER (op->document)); is this intentional or did you forget it? should it be uncommented or just removed? Thanks for the patch :-)
Hi Carlos, Don't worry about the timeframes, I'm well aware of the effort developer-maintainers like you put into keeping a project running! Commented stuff: Indeed, I forgot to remove that. Coding style: Is there a document describing proper gnome coding style? (or evince coding style for that matter) The documents I found on developer.gnome.org didn't mention specifics about spacing. As for the patch, I was wondering what you and others thought about revamping ev-print-operation to dynamically build a model of the forward printout before starting exportation. That way, all the ugly conditionals are kept in one place and the only conditional to be added would be in inc_page. This would add a short delay but the non-blocking behaviour of the actual export would not be affected. Another question I had (probably the wrong place to ask) is whether you were aware of there are any plans upstream to rename the enums for GTK_PAGE_SET_XXX into GTK_SHEET_SET_XXX to be consistent with the actual meaning and the user interface? I'm currently quite busy with some other projects so it might take me a week or so to apply the coding style changes.
Created attachment 136740 [details] [review] new attempt to fix even / odd multipage issues V2 Ok, I hope I've caught all style problems. This comment is more verbose than I had hoped. The patch seems to work alright judging by the test-case, except for printing in reverse where it produces blanks (as it inserts blank pages when the last "page range to be printed" is reached) Note that if the pages were provided in the correct order for multipage reverse printing, it would work properly. See the next attachment for an example of what I mean. Another (very) minor issue is that if one is printing say a 4-page document with 4 pages per sheet and one chooses to print "even sheets only", a blank page will be printed. This is a user interface problem as the UI should not allow "even sheets" to be selected if there are none. (the same is true if you print "even sheets" in a single-page document at one-page per sheet) Again, both problems would have an easy fix by building a model of the printout: the UI could then be informed about the options which are to be disabled and reverse printing would work "for free".
Created attachment 136741 [details] broken reverse multipage which is NOT fixed by this patch This is the example referred to in the above comment.
Did someone have the time to check whether http://bugzilla.gnome.org/attachment.cgi?id=136740 is OK to go in, then? -Bartek
I've applied it to git master, I'll leave the bug open for the minor pending issues. Thank you very much Bartek.
*** Bug 590084 has been marked as a duplicate of this bug. ***
Answer to bug 590084: "2.27.4 Try to print using the FILE printer, and save a PDF with multiple pages per sheet enabled (using 2 or 4 always triggers the issue, I didn't try more). The result file has some empty pages between the normal pages. " Oops, that's my fault I'm afraid, I must've messed something up! (although I'm surprised because I did some extensive testing...) Nicolò, does it happen with every PDF or just a particular one? -Bartek
Bartek, yes, it happens with all files I try to print
Hmm, are you printing in reverse? I know that causes blanks, printing forward shouldn't but I can't be sure. I'm building the git branch to check the problem out, but it might take a while.
I'm normally printing forward. this is my evince version: 2.27.4-0ubuntu2 Do you need a PDF input and the output?
I'm sorry I can't confirm the problem, pdf output works correctly for me. Maybe we have a problem with terminology. (the difference between sheets and pages) It would be nice if you could either attach the output here or send it to my e-mail address. -B.
Hi Nicolò, thanks for the documents. The bug appears to have been introduced via Gtk+ (or some other library used by evince). I can confirm that both the ubuntu package (run as usual) and the git master branch (run via LD_LIBRARY_PATH=/usr/local/lib /usr/local/bin/evince ) suffer from the bug you describe with output completely garbled and pages missing. As pointed out above (comment #32), the git master branch produces correct output on jaunty. My guess is - and a someone more familiar with the subject matter will probably have to correct me - that the GtkPrintOperation in karmic supports printing to file (and advertises itself as such to evince) but it's simply broken, hence the broken output. I currently don't know how to fix this I'm afraid and don't have time right now to look into it more deeply. -Bartek
I think I should open a bug to GTK so
Ok, this is the second to last comment from me for today. My guess was wrong. I compiled a version of evince from before I started working on this (2.26.0) on karmic and that prints fine for Nicolò's specific usage scenario, but it obviously has the problems concerning the combination of multiple pages per sheet and odd/even. Something appears to be introducing more blanks than necessary, unfortunately I don't know whether it's my code, gtk+ or one of the other components in evince. I'll try a gdb run and see when the relevant functions are called.
It appears as though my initial guess was somewhat right after all ;) None of the export_* functions in EvPrintOperation are being called during printing, and those are the only ones that I've modified. It seems that the whole printing is done via gtk+. Interestingly, it also broke the progress updater which remains at "Preparing to print..." during the whole operation. (ev_print_operation_get_progress is called only twice during the whole print operation)
(In reply to comment #36) > It appears as though my initial guess was somewhat right after all ;) None of > the export_* functions in EvPrintOperation are being called during printing, > and those are the only ones that I've modified. It seems that the whole > printing is done via gtk+. when GTK+ version is >= 2.17.1 we use GtkPrinOperation because it's supposed to fix all of these issues. So the rule is: gtk+ < 2.17.1 evince bug, otherwise GTK+ bug. > Interestingly, it also broke the progress updater which remains at "Preparing > to print..." during the whole operation. (ev_print_operation_get_progress is > called only twice during the whole print operation) > yes, see bug #582964.
(In reply to comment #37) > when GTK+ version is >= 2.17.1 we use GtkPrinOperation because it's supposed to > fix all of these issues. So the rule is: gtk+ < 2.17.1 evince bug, otherwise > GTK+ bug. Ah I see, thanks Carlos. The #ifdef did not exist in 2.26.0 which is why that prints fine.
Created attachment 139998 [details] [review] EVINCE_2_26_2 tag branch backport Hi Carlos & co. I completely forgot to backport the patch to 2.26.x which is the version most affected by this bug. If you want I can make one for 2.24.x too. (Don't know whether that will work though, haven't looked at the differences yet!)
Thanks Bartek, but there won't be more releases on the stable branch this cycle, so I don't think it's worth to backport it.
Ok, thanks for your response. Maybe distributions which continue to package 2.26 will happen across this bug and pick up the patch.
This seems to fix a bug for me in 2.26 where printing with: - pages-per-side set to 2 (possibly any value > 1) and - 'Only print' set to odd would print (where numbers refer to original pages): {1,3},{5,7} rather than the desired: {1,2},{5,6} I.e. the "Only print odd pages" was being applied /before/ the pages-per-side, when the desired outcome is that "only odd" should apply to /post/ pages-per-side. Same story with it set to even, and guess similar happens with higher values of pages-per-side. Thanks v much!
Comment on attachment 139998 [details] [review] EVINCE_2_26_2 tag branch backport Marking the patch as rejected. It won't be applied to an old branch.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/87.