<html>
<head>
<base href="https://bugs.freedesktop.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Missing functions for setting document's properties"
href="https://bugs.freedesktop.org/show_bug.cgi?id=36653#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - 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=107369" name="attach_107369" title="core document info dict API rev 2">attachment 107369</a> <a href="attachment.cgi?id=107369&action=edit" title="core document info dict API rev 2">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=36653&attachment=107369'>[review]</a>
core document info dict API rev 2
Review of <span class=""><a href="attachment.cgi?id=107369" name="attach_107369" title="core document info dict API rev 2">attachment 107369</a> <a href="attachment.cgi?id=107369&action=edit" title="core document info dict API rev 2">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=36653&attachment=107369'>[review]</a>:
-----------------------------------------------------------------
::: glib/poppler-document.cc
@@ +778,5 @@
<span class="quote">> }
>
> +gchar *
> +poppler_document_info_get_string (PopplerDocument *document,
> + const gchar *key)</span >
Instead of adding API to extend the info dict with new metadata, I would start
adding support for modifying the well known entries, like Rodrigo's patch did.
I think we could split this patch, first adding the core part and the glib API,
so that tthe glib api doesn't block the core part that could be used by qt.
::: poppler/PDFDoc.cc
@@ +538,5 @@
<span class="quote">>
> +// Set/get doc info string entries
> +GooString *PDFDoc::getDocInfoStringEntry(const char *key)
> +{
> + Object info_obj;</span >
I agree we are not always consistent, but in the core we usually prefer camel
case instead of underscores, so this would be infoObj, instead of info_obj, and
the same for all other cases.
@@ +546,5 @@
<span class="quote">> + if (info_obj.isDict()) {
> + Object result_obj;
> +
> + if (info_obj.dictLookup(key, &result_obj, 0)->isString()) {
> + result = new GooString(result_obj.getString());</span >
You can do result = resultObj.takeString() to avoid a memory allocation.
@@ +571,5 @@
<span class="quote">> +
> +GBool PDFDoc::setDocInfoStringEntry(const char *key, GooString *value)
> +{
> + Object info_obj;
> + GBool success = gFalse;</span >
I'm not sure this could fail, the getter can fail because the entry might not
be present, but here we are setting a new entry, how can that fail?
@@ +578,5 @@
<span class="quote">> +
> + if (info_obj.isDict()) {
> + Object goo_str_obj;
> +
> + if (value) {</span >
The spec says we shouldn't store entry values, but omit the entries, so we
should also check that the string is not empty when not NULL.
@@ +593,5 @@
<span class="quote">> +
> + info_obj.dictSet(key, &goo_str_obj);
> + setInfoModified(this, &info_obj);
> + success = gTrue;
> + }</span >
If the document doesn't have an info dict we should create one instead of
failing.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>