<div id="geary-body" dir="auto"><div>Thanks for the review.</div><div><br></div><div>I've created an issue on GitLab (<a href="https://gitlab.freedesktop.org/cairo/cairo/issues/336">https://gitlab.freedesktop.org/cairo/cairo/issues/336</a>) with a new patch and some explanation.</div><div><br></div><div>I'm not sure that I can easily add tests for now. I don't understand (yet) how it works and how to launch tests, I feel far from adding one. Maybe Adrian could help?</div></div><div id="geary-signature" dir="auto"><div style="white-space: pre-wrap;">--
Guillaume</div></div><div id="geary-quote" dir="auto"><br>Le ven. 24 août 2018 à 20:24, Bryce Harrington <bryce@bryceharrington.org> a écrit :<br><blockquote type="cite"><div class="plaintext" style="white-space: pre-wrap;">[Adrian - would be great to get your feedback on this patch.]
On Tue, Aug 21, 2018 at 03:55:35PM +0200, Guillaume Ayoub wrote:
<blockquote> As explained on the mailing list[1], links are broken when their targets are on
a page that has not been created yet.
This commit fixes the problem by using indirect references for /Dest objects,
and by creating the references when the file has been totally drawn.
I'm not sure whether the "links" property should be in "surface" or in
"interchange". I may also have added some memory leaks even if I did my best to
avoid them, as I'm not a regular C developer.
[1] <a href="https://lists.cairographics.org/archives/cairo/2018-August/028715.html">https://lists.cairographics.org/archives/cairo/2018-August/028715.html</a>
---
src/cairo-pdf-interchange.c | 100 +++++++++++++++++++++++---------
src/cairo-pdf-surface-private.h | 9 +++
src/cairo-pdf-surface.c | 2 +
3 files changed, 84 insertions(+), 27 deletions(-)
diff --git a/src/cairo-pdf-interchange.c b/src/cairo-pdf-interchange.c
index 9aa6934dc..d9b1e19fc 100644
--- a/src/cairo-pdf-interchange.c
+++ b/src/cairo-pdf-interchange.c
@@ -327,29 +327,34 @@ _named_dest_pluck (void *entry, void *closure)
static cairo_int_status_t
cairo_pdf_interchange_write_explicit_dest (cairo_pdf_surface_t *surface,
- int page,
- cairo_bool_t has_pos,
- double x,
- double y)
+ cairo_pdf_link_t *link)
{
cairo_pdf_resource_t res;
double height;
- if (page < 1 || page > (int)_cairo_array_num_elements (&surface->pages))
+ if (link->page < 1 || link->page > (int)_cairo_array_num_elements (&surface->pages))
return CAIRO_INT_STATUS_TAG_ERROR;
- _cairo_array_copy_element (&surface->page_heights, page - 1, &height);
- _cairo_array_copy_element (&surface->pages, page - 1, &res);
- if (has_pos) {
- _cairo_output_stream_printf (surface->output,
- " /Dest [%d 0 R /XYZ %f %f 0]\n",
- res.id,
- x,
- height - y);
+ _cairo_pdf_surface_update_object (surface, link->res);
+
+ _cairo_array_copy_element (&surface->page_heights, link->page - 1, &height);
+ _cairo_array_copy_element (&surface->pages, link->page - 1, &res);
+ if (link->has_pos) {
+ _cairo_output_stream_printf (surface->output,
+ "%d 0 obj\n"
+ " [%d 0 R /XYZ %f %f 0]\n"
+ "endobj",
+ link->res.id,
+ res.id,
+ link->x,
+ height - link->y);
} else {
- _cairo_output_stream_printf (surface->output,
- " /Dest [%d 0 R /XYZ null null 0]\n",
- res.id);
+ _cairo_output_stream_printf (surface->output,
+ "%d 0 obj\n"
+ " [%d 0 R /XYZ null null 0]\n"
+ "endobj",
+ link->res.id,
+ res.id);
}
return CAIRO_STATUS_SUCCESS;
@@ -376,6 +381,10 @@ cairo_pdf_interchange_write_dest (cairo_pdf_surface_t *surface,
* reference instead of the name */
double x = 0;
double y = 0;
+ cairo_pdf_resource_t res;
+ cairo_pdf_link_t *link;
+
+ link = _cairo_malloc (sizeof (cairo_pdf_link_t));
</blockquote>
link can be NULL here if malloc fails, so error handling for that might
be appropriate here.
<blockquote> if (named_dest->extents.valid) {
x = named_dest->extents.extents.x;
@@ -388,10 +397,16 @@ cairo_pdf_interchange_write_dest (cairo_pdf_surface_t *surface,
if (named_dest->attrs.y_valid)
y = named_dest->attrs.y;
- status = cairo_pdf_interchange_write_explicit_dest (surface,
- named_dest->page,
- TRUE,
- x, y);
+ res = _cairo_pdf_surface_new_object (surface);
+ _cairo_output_stream_printf (surface->output,
+ " /Dest %d 0 R\n",
+ res.id);
+ link->res = res;
+ link->page = named_dest->page;
+ link->has_pos = TRUE;
+ link->x = x;
+ link->y = y;
+ status = _cairo_array_append (&surface->links, link);
</blockquote>
I'm not sure I follow the reason for the above change, can you explain?
<blockquote> return status;
}
}
@@ -406,11 +421,24 @@ cairo_pdf_interchange_write_dest (cairo_pdf_surface_t *surface,
dest);
free (dest);
} else {
- status = cairo_pdf_interchange_write_explicit_dest (surface,
- link_attrs->page,
- link_attrs->has_pos,
- link_attrs->pos.x,
- link_attrs->pos.y);
+ cairo_pdf_resource_t res;
+ cairo_pdf_link_t *link;
+
+ link = _cairo_malloc (sizeof (cairo_pdf_link_t));
+ res = _cairo_pdf_surface_new_object (surface);
+ _cairo_output_stream_printf (surface->output,
+ " /Dest %d 0 R\n",
+ res.id);
+ link->res = res;
+ link->page = link_attrs->page;
+ link->has_pos = link_attrs->has_pos;
+ if (link->has_pos) {
+ link->x = link_attrs->pos.x;
+ link->y = link_attrs->pos.y;
+ }
+ status = _cairo_array_append (&surface->links, link);
+ if (unlikely (status))
+ return status;
</blockquote>
An explanation of the above would be appreciated as well. Make sure to
also check link for NULL, and _cairo_pdf_surface_new_object() also has
an error condition to check for, which can be detected here if res.id == 0
<blockquote> }
return CAIRO_STATUS_SUCCESS;
@@ -855,6 +883,20 @@ strcmp_null (const char *s1, const char *s2)
return FALSE;
}
+static cairo_int_status_t
+cairo_pdf_interchange_write_links (cairo_pdf_surface_t *surface)
+{
+ int num_elems, i;
+ cairo_pdf_link_t link;
+
+ num_elems = _cairo_array_num_elements (&surface->links);
+ for (i = 0; i < num_elems; i++) {
+ _cairo_array_copy_element (&surface->links, i, &link);
+ cairo_pdf_interchange_write_explicit_dest (surface, &link);
+ }
+ return CAIRO_STATUS_SUCCESS;
+}
</blockquote>
I assume if there are zero links (i.e. num_elems == 0) that calling this
should still result in success; if it shouldn't then might want to check
and return a different status in that case.
<blockquote> +
static cairo_int_status_t
cairo_pdf_interchange_write_page_labels (cairo_pdf_surface_t *surface)
{
@@ -1357,10 +1399,10 @@ _cairo_pdf_interchange_write_page_objects (cairo_pdf_surface_t *surface)
cairo_int_status_t status;
status = cairo_pdf_interchange_write_page_annots (surface);
- if (unlikely (status))
+ if (unlikely (status))
return status;
- cairo_pdf_interchange_clear_annotations (surface);
+ cairo_pdf_interchange_clear_annotations (surface);
return cairo_pdf_interchange_write_page_parent_elems (surface);
}
@@ -1395,6 +1437,10 @@ _cairo_pdf_interchange_write_document_objects (cairo_pdf_surface_t *surface)
if (unlikely (status))
return status;
+ status = cairo_pdf_interchange_write_links (surface);
+ if (unlikely (status))
+ return status;
+
status = cairo_pdf_interchange_write_names_dict (surface);
if (unlikely (status))
return status;
diff --git a/src/cairo-pdf-surface-private.h b/src/cairo-pdf-surface-private.h
index 3793332ce..64dd300f6 100644
--- a/src/cairo-pdf-surface-private.h
+++ b/src/cairo-pdf-surface-private.h
@@ -208,6 +208,14 @@ typedef struct _cairo_pdf_outline_entry {
int count;
} cairo_pdf_outline_entry_t;
+typedef struct _cairo_pdf_link {
+ cairo_pdf_resource_t res;
+ int page;
+ cairo_bool_t has_pos;
+ double x;
+ double y;
+} cairo_pdf_link_t;
+
struct docinfo {
char *title;
char *author;
@@ -327,6 +335,7 @@ struct _cairo_pdf_surface {
cairo_pdf_interchange_t interchange;
int page_parent_tree; /* -1 if not used */
cairo_array_t page_annots;
+ cairo_array_t links;
cairo_bool_t tagged;
char *current_page_label;
cairo_array_t page_labels;
diff --git a/src/cairo-pdf-surface.c b/src/cairo-pdf-surface.c
index ab6781340..9fd48265d 100644
--- a/src/cairo-pdf-surface.c
+++ b/src/cairo-pdf-surface.c
@@ -493,6 +493,7 @@ _cairo_pdf_surface_create_for_stream_internal (cairo_output_stream_t *output,
surface->page_parent_tree = -1;
_cairo_array_init (&surface->page_annots, sizeof (cairo_pdf_resource_t));
+ _cairo_array_init (&surface->links, sizeof (cairo_pdf_link_t));
surface->tagged = FALSE;
surface->current_page_label = NULL;
_cairo_array_init (&surface->page_labels, sizeof (char *));
@@ -2296,6 +2297,7 @@ _cairo_pdf_surface_finish (void *abstract_surface)
_cairo_array_fini (&surface->fonts);
_cairo_array_fini (&surface->knockout_group);
_cairo_array_fini (&surface->page_annots);
+ _cairo_array_fini (&surface->links);
if (surface->font_subsets) {
_cairo_scaled_font_subsets_destroy (surface->font_subsets);
</blockquote>
Thanks for working on this. You might want to file a ticket in Bugzilla
if there's not one already, so it can be referenced from this patch.
(This may be handy with doing bugfix backporting, for ex.)
Make sure to also look at how to get some test coverage on this. Would
something squeezed into create-for-stream.c make sense? Or, maybe
better to craft a new test case that checks for this particular
situation?
Beyond that, I'd love if Adrian can jump in with comments on it as well,
before we look at landing it.
Bryce
</div></blockquote></div>