[poppler] Poppler 0.12 release schedule

[Albert Astals Cid]

A Divendres, 15 de maig de 2009, Hal V. Engel va escriure:
> On Thursday 14 May 2009 11:22:14 am Albert Astals Cid wrote:
> > 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.
>
> Thanks for the tip.  I have not worked with git before this.  So I am on a
> learning curve.
>
> > > 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.
>
> Meaning poppler-qt4.h or are you saying that this should also apply to
> GfxState.h?

I mean poppler-qt4.h, GFxState is not really public API even some people use 
it.
>
> > > Especially that including lcms.h would break all the poppler-qt4
> > > library users which don't have LCMS available.
>
> Which is almost no one now days but it is still a valid point.
>
> > > 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.
>
> I agree that using void* is more general and it would make it easier to
> abstract the code into a general purpose library for use by all tool sets
> rather than just Qt.
>
> I am thinking of pulling the functions to get the system supplied monitor
> profile(s) into a separate library.  I think I can find a place to host
> this code so that it is available to a wide range of projects.  But it will
> take some time to do that.  In the mean time working out the details in the
> demo viewer seems like the way to go.
>
> > > 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.
>
> USE_CMS or HAVE_LCMS?  At some point it might be desirable to abstract the
> CMS support so that other CMS's like ColorSysc (OS/X), the Windows CMS,
> ArgyllCMS,  AdobeAce ... can be used instead.  The more general solution is
> to use USE_CMS for this so as not to tie this to a specific CMS.
>
> I now have the CMS related functions in the poppler-qt4 library and the
> demo viewer setup so that LCMS specifics are not visible in the header
> files and these functions will always be built.  But if USE_CMS is not
> defined they will simply return NULL or do nothing.  I will look at doing
> the same type of thing in GfxState.* but perhaps Koji should have a look at
> this.
>
> I added
>
> bool GfxColorSpace::cmsAvailable() and bool Document::cmsAvailable()
>
> to make this test available for user apps.
>
> On the other hand for poppler to meet the current PDF specification it
> really needs to have CMS support.  So in general not using a CMS should be
> discouraged and in the longer term it should become a very rare situation
> for users to not have a CMS installed since most users with up to date
> distros will have lcms installed by default because may other apps now
> require it.

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
>
> This is really more in the scope of OpenICC.  So I will look into it.
>
> > > > 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.
>
> Yes I now have code blocks for X11, Windows and OS/X for this OS specific
> code.  Each OS does this in completely different ways with APIs that are
> very different.  So it is necessary to abstract this in a non-OS specific
> API.  I pulled most of the Windows and OS/X code I am using from stuff I
> had in LProf for dealing with video gamma tables since the logic involved
> in getting the handle for the specific monitor is the same as needed here. 
> I also found the rest of the Windows code in a note that had been sent to
> the lcms email list by Marti Maria (the author of lcms). So most of the
> code has been tested in other apps but I am still trying to understand some
> details in the OS/X API and I have not located the documentation for this
> yet.
>
> > > 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)

Documentation on poppler-qt4.h is weird, mentions functions that don't exist 
and we also have a commented function, can you clean it up? It would be good 
if you kept doxygen style comments, that is one comment per function. Also 
config.h is not needed there anymore. 

> or setDocumentProfile().  And it appears that this is how it works although
> I am not sure I understand why this happens.

It works if you call the functions before GfxColorSpace::setupColorProfiles() 
is called.

Also i think 

void GfxColorSpace::setDisplayProfile(void *displayProfileA)
void GfxColorSpace::setDisplayProfileName(GooString *name)

should take care of deleting old contents of the variables they assign to, 
what do you think?

>
> > > -) API: in Document::setDocumentProfileName(), QStringToGooString()
> > > creates a new GooString, which should be delete'd.
>
> Done.  But I don't think it is necessary since this is a local variable
> that is created on the stack and it is gone as soon as the call returns.

No, that means you get a leak each time because a newed pointer goes out of 
scope.

The rest seems oka-ish to me.

Albert

>
> > About naming, should setDocumentProfileName be setOutputProfileName? I
> > mean should i set there the profile of my monitor (output) or of the
> > document (input)
>
> This is a good question.  This is really a wrapper function for
> GfxColorSpace::setOutputProfileName().  But it might make things clearer if
> these methods were overloaded and my code now does this.
>
> > Albert
> >
> > > -) Qt4 demo: you don't need to create and hold a new QDesktopWidget,
> > > QApplication::desktop() will return the global one.
>
> Done
>
> > > -) Qt4 demo: getProfileAtom() should be just colorProfile(), returning
> > > a Qt::HANDLE and being always defined.
>
> Actually a more meaningful name would be getMonitorProfile(...) since this
> is actually what it does.  I have changed this in my current code.  Also
> Windows and OS/X require additional information to get the correct screen
> handle so I have set this up to allow that data to be passed in.  In X11
> with Qt these are just dummy parms that don't do anything.  But it will
> make the API the same in every OS.  I should add that the reason that these
> same parms are not needed is that it can be handled via a
> QX11info::display() call so doing this in a generic X11 way (IE. not using
> Qt calls) will likely also require the same parms as Windows and OS/X.  Qt
> does not have a similar API for Windows or OS/X.
>
> > > -) Qt4 demo: the general coding style there is much like
> > > http://qt.gitorious.org/qt/pages/QtCodingStyle, so please follow it,
> > > whenever possible.
>
> I had a look and my code is now in the correct style at least for the most
> part.
>
> I have created a new patch with these changes and it is attached here.  The
> getMonitorProfile() code for Windows and OS/X is completely untested and I
> don't even know if it compiles at this point.  But it is mostly correct and
> there is perhaps a 50% chance that the Windows code is OK or at most only
> needs minor tweaks to work.
>
> Hal