A Divendres, 29 d'octubre de 2010, Benjamin Adler va escriure: > On 10/28/2010 10:16 PM, Albert Astals Cid wrote: > > A Dijous, 28 d'octubre de 2010, Benjamin Adler va escriure: > >> [...] > >> Please tell me what you think! > >> > > * Use capital first letter for the enum values. > > Done. > > > * Don't call the variables cropBox since it seems it contains the > > "cropBox", > > > > call them boxToCrop or something like that > > I opted for "pageBox" and hope thats ok. > > > * It also would be cool if you detected -cropbox and -crop MediaBox > > together > > > > and told the user he is wrong and needs to fix his parameters. > > Done. > > I still don't know whether it was ok to change Page::textList() from > > m_page->parentDoc->doc->displayPageSlice( > output_dev, m_page->index + 1, 72, 72, rotation, > false, false, false, -1, -1, -1, -1); > > into > > m_page->parentDoc->doc->displayPageSlice( > output_dev, m_page->index + 1, 72, 72, rotation, > > ::Page::MediaBox, false, -1, -1, -1, -1); > > because I don't really know what the desired behaviour is when > "!useMediaBox && !crop" is passed.
I think false, true and false, false actually create a very similar if nto the same behaviour. But that leads me to the fact that (even if noone that we know was using it), your current patch makes it impossible to get the same behaviour of having useMediaBox = true and crop = true. Since that gave you a page as big as the mediabox but the contents cropped to the cropbox, so i really think the display methods should have two parameters Page::PageBox box, Page::PageBox cropBox that mimic exactly what the Gfx constructor parameters do, and we'd probably need another value for the PageBox enum that would be something like NoBox so you can pass it to boxToCrop if you don't want any cropping (passing a null to Gfx). What do you think? > > Also, I created the two methods > > static Page::PageBox getPageBoxFromString(const char *pageBoxName, > GBool* success) and > > PDFRectangle* getPDFRectangleOfPageBox(const Page::PageBox pageBox), > > using the first in the utils and the second in Page itself. The latter > is not private yet, should it be? > > I personally feel that it'd be more beautiful to replace > > Page::getMediaWidth() > Page::getMediaHeight() > Page::getCropWidth() > Page::getCropHeight() > > with > > double Page::getPageBoxWidth(Page::PageBox) > double Page::getPageBoxHeight(Page::PageBox) > > and modify callers accordingly. Do you agree? Please don't do any change to existing code unless it's totally necessary. > > Should I add my name to the files I changed (mostly Page.cc/h)? You don't need to worry about that i'll take care of it when commiting. > > cheers, > ben > > P.S: Sorry for making this a two-patch-mess, I need to learn git. That makes two of us ;-.) Albert _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
