A Dijous, 14 de maig de 2009, Pino Toscano va escriure: > Alle martedì 12 maggio 2009, Hal V. Engel ha scritto: > > Here it is. This patch applies and builds on git head as of this > > afternoon. > > Nice! > Just as a general comment: there are many whitespace-only changes (like a > tab that becomes four spaces); if you could avoid them, and produce a patch > in unified format (`git diff -U` should be enough), it would be slightly > more readable. > > Now some comments about the Qt4 related parts: > > 1. I am not a cmake expert so I had to hack on qt4/src/CMakeLists.txt so > > that USE_CMS was defined for the qt4 stuff. Someone with more cmake > > expertise almost for sure needs to fix my hack. > > I don't like much the "have/don't-have LCMS" exposed in the public API. > Especially that including lcms.h would break all the poppler-qt4 library > users which don't have LCMS available. > > The only solution I have in mind is the following: given the only LCMS bit > exposed is cmsHPROFILE, which internally is a void*, it could be passed as > Qt::HANDLE, like this: > void setColorProfile(Qt::HANDLE cmsProfile); > void setColorProfileName(const QString &name); > Qt::HANDLE colorRGBProfile() const; > Qt::HANDLE colorProfile() const; > and then doing 'cmsHPROFILE profile = (cmsHPROFILE)cmsProfile;' to cast the > profile back and forth. Of course the apidox for the function would need to > specify that the Qt::HANDLE refers to a cmsHPROFILE, and in case there's no > color management that those handles are NULL.
If we are going to pass void * just pass void *, it's as ugly as a Qt::HANDLE and shows what you're doing more clearly imho. > Also, the function would always be compiled, and the actual implementations > making nothing when HAVE_LCMS is not defined. For this, adding a > static bool canManageColorProfiles(); > would help users to know whether they can actually expect the function to > do something. Agreed. > > > 4. Someone with Windows and/or Mac expertise needs to write versions of > > qt4/demos/viewer.cpp cmsHPROFILE PdfViewer::getProfileAtom () for those > > other OS's for this to work on Windows and on the Mac. It might also be > > useful to move these functions to a support library so that they are > > available to anyone who uses the library. Yeah, but i don't really think that library belongs into the poppler scope, so let's just use that code until someone creates that lib :D > > > > 5. I am not a gkt programmer so I made no attempt to update any of the > > gtk stuff to support CM. But the Qt4 code should provide enough clues > > for a gtk programmer to do the same basic stuff as I did with the Qt4 > > support library and the demo viewer. > > Then the X11 specific code should be grouped inside #ifdef Q_WS_X11 blocks. > > Other notes: > > -) API: not sure whether it is necessary to expose 'int > setupDocumentColorProfiles()', it seems more worth just making sure it is > properly intialized when needed, either in the poppler core or when > creating a new SplashOutputDev? In my opinion it's not needed at all, should be automatic on creation and on calls that need to change something (like i guess setDocumentProfileName) > > -) API: in Document::setDocumentProfileName(), QStringToGooString() creates > a new GooString, which should be delete'd. About naming, should setDocumentProfileName be setOutputProfileName? I mean should i set there the profile of my monitor (output) or of the document (input) Albert > > -) Qt4 demo: you don't need to create and hold a new QDesktopWidget, > QApplication::desktop() will return the global one. > > -) Qt4 demo: getProfileAtom() should be just colorProfile(), returning a > Qt::HANDLE and being always defined. > > -) Qt4 demo: the general coding style there is much like > http://qt.gitorious.org/qt/pages/QtCodingStyle, so please follow it, > whenever possible. _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
