[Poppler-bugs] [Bug 56843] [PATCH] Feature : extract media in html format

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=56843

--- Comment #11 from Albert Astals Cid <tsdgeos at terra.es> ---
(In reply to comment #8)
> > Also can you explain the code change in poppler/FileSpec.cc?
> 
> I made ​​the change in the file "FileSpec.cc" because of all the test files
> I used the old method to read the filename does not work.
> Looking at the struture of PDF, I noticed that the tag 'F' was still present.

Why it wouldn't work, it first checks for UF and if it's not there uses F,
besides the specification clearly documents that F is deprecated and that UF
should be used if both are present

Also i'm going to guess this is one of your first open source contributions and
you are making the mistake of dumping everything in the same patch, i.e. 
   [PATCH 6/7] Remove "
does not have anything to do with "Feature : extract media in html format" and
you should submit it separately because dumping everything in the same patch
makes it harder to review.


-  if (ignore||(complexMode && !xml)) {
+  if (ignore|| !(complexMode && withMedia)) {

Why the !xml was removed?

delete assets; is not enough, check how other goolists are deleted

I'm also a bit confused with 

+  type = richMediaContentType;
   configurations = new GooList();
   assets = new GooList();
   //Type name (Optional; ExtensionLevel 3)
   if (dict->lookup("Type", &obj1)->isName()) {
-    type = obj1.getName();
+    const char *name = obj1.getName();
+    if(!strcmp("RichMediaContent", name)){
+      type = richMediaContentType;
+    }
   }

if the type will always be richMediaContentType, why bother asking the dict?

Also it would be cool if you could squash patch 7 into patch 3, this would make
reviewing easier

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20121121/eab0ff87/attachment.html>