[Poppler-bugs] [Bug 33269] Expose text attributes in glib interface

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Carlos Garcia Campos <carlosgc at gnome.org> 2011-08-22 04:45:03 PDT ---
Review of attachment 50406:
 --> (https://bugs.freedesktop.org/review?bug=33269&attachment=50406)

Patch looks good to me, I have a few comments, though. Since we are in a hurry
to have this new api in 0.18 I've modified the patch and pushed to git master.
Thanks!

::: glib/poppler-page.cc
@@ +1485,3 @@
+               poppler_text_info_copy,
+               poppler_text_info_free)
+

The name of the struct is inconsistent with the api
poppler_page_get_text_attributes(), I think the struct name should be
PopplerTextAttributes.

@@ +1493,3 @@
+ * Returns: a new #PopplerTextInfo with
+ * poppler_text_info_free() to free it
+ */

Since: 0.18 tag is missing here.

@@ +1498,3 @@
+{
+  PopplerTextInfo *tinfo = g_slice_new0 (PopplerTextInfo);
+  tinfo->color = poppler_color_new ();

Since the color is never null, we can allocate it in the stack instead.

@@ +1538,3 @@
+    fontname++;
+  }
+

This code to get the font name can be factored out, also you are iterating the
string twice to look for +, if there's a subset, i is already the index of + in
the string.

@@ +2030,3 @@
+poppler_page_free_text_attributes (GList *list)
+{
+  g_list_free_full (list, (GDestroyNotify)poppler_text_info_free);

This is new api added in glib 2.28, poppler depends on 2.18

@@ +2077,3 @@
+
+      // each char of the word has the same attributes
+      textinfo2 = poppler_text_info_new_from_word (word);

Instead of always creating a new text info, we can compare TextWord objects to
decide whether to create a new text info or not, so that we avoid unneeded
create/destroy.

@@ +2084,3 @@
+          textinfo2->color->red != textinfo->color->red ||
+          textinfo2->color->green != textinfo->color->green ||
+          textinfo2->color->blue != textinfo->color->blue)

This can be factored out into a method that compares attrs of two words

@@ +2090,3 @@
+          textinfo = textinfo2;
+          textinfo->start = offset;
+          attributes = g_list_append (attributes, textinfo);

Using g_list_prepend() + g_list_reverse() is more efficient than
g_list_append()

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.