[poppler] Releasing RC1 one week later

[Carlos Garcia Campos]

El sáb, 08-03-2008 a las 21:20 +1100, Brad Hards escribió:
> On Saturday 08 March 2008 08:29:03 pm Carlos Garcia Campos wrote:
> > El vie, 07-03-2008 a las 20:03 +1100, Brad Hards escribió:
> > > On Monday 03 March 2008 09:25:10 pm Carlos Garcia Campos wrote:
> > > > I've started to look at the optional content stuff and I've realized
> > > > there are some things  implemented in the qt4 frontend that should be
> > > > in the core, like the order array and radio buttons stuff. I've
> > > > changed/fixed other minor issues. Bradh, could you have a look at these
> > > > patches, please?
> > > >
> > > > http://people.freedesktop.org/~carlosgc/poppler-optcontent.diff
> > >
> > > I haven't tried / tested any of them, but I'll still offer my opinion,
> > > based on initial review.
> >
> > we can just keep both interfaces, getOCGs() returns the
> > optionalContentGroups and getOrderArray() the array object, and
> > getOrderedOCGs or something like that would return the list of optional
> > content already ordered.
> Please don't do this. Just use the existing interface. Then people who don't 
> want to present the UI part don't pay for parsing.

They won't, since they can still use the getOCGs instead of
getOrderedOCGs. I don't see the problem. 

> If you do both, then the original argument for common code doesn't hold 
> anyway.
> 
> > > The foo() -> getFoo() rename is fine (not standard for Qt, but looks more
> > > standard for poppler),
> >
> > yes, that's consistent with poppler core code.
> >
> > >  but please don't put the implementation in the header.
> >
> > ok, no problem, but just out of curiosity, why?
> Because it limits ability to fix things later, and because it is ugly. I 
> shouldn't have done any, but almost certainly did. It isn't that big a deal.
> 
> > > From: Carlos Garcia Campos <carlosgc at gnome.org>
> > > Date: Sun, 2 Mar 2008 19:43:15 +0100
> > > Subject: [PATCH] Use an array instead of a list for optional content
> > > groups Sorry, I don't understand what you are trying to accomplish with
> > > this. Seems gratuitous. Can you explain more?
> >
> > We don't really need a list when we already know the size of the list.
> > An array it's much more efficient than a list and it's also more
> > consistent with the poppler code.
> Did you do any measurements of the impact?

Well, accessing a list is O(n) while an array is O(1) and you avoid a
lot allocation/deallocations of memory. Looking at the GooList
implementation for n <= 8 it's almost the same than using a list. 

> I really don't care to much, but make sure you don't break the Qt4 bindings.

It's not a huge change, changes needed in the qt4 side would be quite
trivial. 

> > > >From 157d763c3e0e5432cce4da391fab99d0854a63f5 Mon Sep 17 00:00:00 2001
> > >
> > > From: Carlos Garcia Campos <carlosgc at gnome.org>
> > > Date: Sun, 2 Mar 2008 19:52:37 +0100
> > > Subject: [PATCH] Fix memory leaks
> > > I don't see the leaks. Again, can you explain why this change is needed?
> >
> > m_orderArray and m_rBGroupsArray are never freed. By using objects
> > instead of pointers you avoid allocations/deallocations too.
> Seems like an excessive change. Why not just free it in the destructor?
> 

Again, because you avoid alloc/dealloc more than  needed. 
-- 
Carlos Garcia Campos
   elkalmail at yahoo.es
   carlosgc at gnome.org
   http://carlosgc.linups.org
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada
	digitalmente
Url : http://lists.freedesktop.org/archives/poppler/attachments/20080308/2647c56d/attachment.pgp