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 <[EMAIL PROTECTED]> > > > 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 <[EMAIL PROTECTED]> > > > 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 [EMAIL PROTECTED] [EMAIL PROTECTED] http://carlosgc.linups.org PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
signature.asc
Description: Esta parte del mensaje está firmada digitalmente
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
