<html>
<head>
<base href="https://bugs.freedesktop.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_ASSIGNED "
title="ASSIGNED - Missing functions for setting document's properties"
href="https://bugs.freedesktop.org/show_bug.cgi?id=36653#c47">Comment # 47</a>
on <a class="bz_bug_link
bz_status_ASSIGNED "
title="ASSIGNED - Missing functions for setting document's properties"
href="https://bugs.freedesktop.org/show_bug.cgi?id=36653">bug 36653</a>
from <span class="vcard"><a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=124570" name="attach_124570" title="[PATCH 7/7] glib: Added document property setters & simplified getters">attachment 124570</a> <a href="attachment.cgi?id=124570&action=edit" title="[PATCH 7/7] glib: Added document property setters & simplified getters">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=36653&attachment=124570'>[review]</a>
[PATCH 7/7] glib: Added document property setters & simplified getters
Review of <span class=""><a href="attachment.cgi?id=124570" name="attach_124570" title="[PATCH 7/7] glib: Added document property setters & simplified getters">attachment 124570</a> <a href="attachment.cgi?id=124570&action=edit" title="[PATCH 7/7] glib: Added document property setters & simplified getters">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=36653&attachment=124570'>[review]</a>:
-----------------------------------------------------------------
Same here, I have a few more comments, but I'll push a modified patch.
::: glib/poppler-document.cc
@@ +754,3 @@
<span class="quote">>
> + gchar *utf16 = g_convert (src, -1, "UTF-16BE", "UTF-8", NULL, &outlen, NULL);
> + GooString *result = new GooString (utf16, outlen);</span >
g_convert returns NULL if the conversion fails for whatever reason, we should
handle that case.
@@ +758,2 @@
<span class="quote">>
> + assert(result->getLength() != 0);</span >
And then this assert is not needed, I would say.
@@ +764,3 @@
<span class="quote">> }
>
> + assert((*result).getLength() != 0);</span >
Nor this redundant one.
@@ +886,5 @@
<span class="quote">> + * 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</span >
NULL -> %NULL
We don't normally use empty strings in glib APIs, so better to only handle the
case of NULL explicitly.
@@ +896,5 @@
<span class="quote">> +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);</span >
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 @@
<span class="quote">> time_t
> poppler_document_get_creation_date (PopplerDocument *document)
> {
> + g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), -1);</span >
I think we should always cast (time_t)-1
@@ +2831,5 @@
<span class="quote">> return NULL;
> }
>
> +GooString *
> +_poppler_convert_gtime_to_pdf_date (time_t gdate) {</span >
We don't need to add a new function for this, there's timeToDateString() in the
core.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are on the CC list for the bug.</li>
</ul>
</body>
</html>