[Poppler-bugs] [Bug 36653] Missing functions for setting document's properties

[bugzilla-daemon at freedesktop.org]

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

--- Comment #47 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 124570
  --> https://bugs.freedesktop.org/attachment.cgi?id=124570
[PATCH 7/7] glib: Added document property setters & simplified getters

Review of attachment 124570:
-----------------------------------------------------------------

Same here, I have a few more comments, but I'll push a modified patch.

::: glib/poppler-document.cc
@@ +754,3 @@
>  
> +  gchar *utf16 = g_convert (src, -1, "UTF-16BE", "UTF-8", NULL, &outlen, NULL);
> +  GooString *result = new GooString (utf16, outlen);

g_convert returns NULL if the conversion fails for whatever reason, we should
handle that case.

@@ +758,2 @@
>  
> +  assert(result->getLength() != 0);

And then this assert is not needed, I would say.

@@ +764,3 @@
>    }
>  
> +  assert((*result).getLength() != 0);

Nor this redundant one.

@@ +886,5 @@
> + * poppler_document_set_title:
> + * @document: A #PopplerDocument
> + * @title: A new title
> + *
> + * Sets the document's title. If @title is NULL or its length is 0, Title entry

NULL -> %NULL
We don't normally use empty strings in glib APIs, so better to only handle the
case of NULL explicitly.

@@ +896,5 @@
> +poppler_document_set_title (PopplerDocument *document, const gchar *title)
> +{
> +  g_return_if_fail (POPPLER_IS_DOCUMENT (document));
> +  GooString *goo_title = _poppler_goo_string_from_utf8(title);
> +  document->doc->setDocInfoTitle(goo_title);

We don't want to remove the entry if the conversion fails, so we should handle
the case of NULL given by the user separately from the the case of
_poppler_goo_string_from_utf8 returning NULL. These comments apply to all
setters.

@@ +1122,4 @@
>  time_t
>  poppler_document_get_creation_date (PopplerDocument *document)
>  {
> +  g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), -1);

I think we should always cast (time_t)-1

@@ +2831,5 @@
>    return NULL;
>  }
>  
> +GooString *
> +_poppler_convert_gtime_to_pdf_date (time_t gdate) {

We don't need to add a new function for this, there's timeToDateString() in the
core.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/poppler-bugs/attachments/20160703/caf079f8/attachment-0001.html>