After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 635705 - API Annotation work for EvinceDocument and EvinceView
API Annotation work for EvinceDocument and EvinceView
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Evince Maintainers
Evince Maintainers
: 662709 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-11-24 16:18 UTC by José Aliste
Modified: 2017-09-26 20:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use G_DEFINE_BOXED instead of custom macro. (3.18 KB, patch)
2010-11-24 16:19 UTC, José Aliste
committed Details | Review
Make EvSourceLink boxed (2.82 KB, patch)
2010-11-24 16:20 UTC, José Aliste
needs-work Details | Review
Some annotations for libdocument (12.31 KB, patch)
2011-05-30 10:48 UTC, Lucas Stadler
rejected Details | Review
libdocument: document EvAnnotation. (5.42 KB, patch)
2011-11-03 11:54 UTC, José Aliste
needs-work Details | Review
libdocument: Document EvAnnotation (5.10 KB, patch)
2011-11-03 17:07 UTC, José Aliste
none Details | Review
libdocument: Document EvAnnotation (6.15 KB, patch)
2011-11-03 22:21 UTC, José Aliste
needs-work Details | Review
libdocument: Make EvSourceLink a Boxed Type. (3.90 KB, patch)
2011-11-04 01:13 UTC, José Aliste
needs-work Details | Review
libdocument: Add missing annotation to ev_document_factory_get_document. (1.02 KB, patch)
2011-11-06 02:11 UTC, José Aliste
committed Details | Review
libdocument: Make EvSourceLink boxed. (3.41 KB, patch)
2011-11-15 00:00 UTC, José Aliste
committed Details | Review
libdocument: Document EvAnnotation. (6.00 KB, patch)
2011-11-15 00:00 UTC, José Aliste
committed Details | Review
libview: Use ev_source_link_free instead of g_free. (812 bytes, patch)
2011-11-15 00:00 UTC, José Aliste
committed Details | Review

Description José Aliste 2010-11-24 16:18:02 UTC
Currently, they are several annotation lacking in libview and libdocument. Moreover, we need to make some struct boxed so they can be detected by gobject-introspection. 

I will attach patches to make libview and libdocument gi happy.
Comment 1 José Aliste 2010-11-24 16:19:43 UTC
Created attachment 175175 [details] [review]
Use G_DEFINE_BOXED instead of custom macro.
Comment 2 José Aliste 2010-11-24 16:20:13 UTC
Created attachment 175176 [details] [review]
Make EvSourceLink boxed
Comment 3 Christian Persch 2010-11-25 15:05:58 UTC
+	g_free (link->filename);

but:

+struct _EvSourceLink
+{
+        const gchar *filename;

That gives a warning, doesn't it? You probably need to cast in the g_free().

Other than that, the patches look ok to me.
Comment 4 Lucas Stadler 2011-05-30 10:47:09 UTC
I added a few annotations for evolution-document, but I'm not at all sure if they are correct.

I *think* that everything that comes from `priv->something`  or similar has to be annotated with (transfer none) and most other things with (transfer full), but I don't have that much experience with  it.

So, the diff is attached, would you have a look at it and correct me where I'm wrong?
Comment 5 Lucas Stadler 2011-05-30 10:48:50 UTC
Created attachment 188875 [details] [review]
Some annotations for libdocument

Really not sure if it is correct.
Comment 6 Gonzalo Odiard 2011-11-02 13:14:18 UTC
I am trying these patches.

Now I can do:

>>> doc = EvinceDocument.Document.factory_get_document('file:///home/gonzalo/Desktop/AjedrezyLeyendas-Postmortem.pdf')
Traceback (most recent call last):
  • File "<stdin>", line 1 in <module>
  • File "/usr/lib64/python2.7/site-packages/gi/types.py", line 43 in function
    return info.invoke(*args, **kwargs)
gi._glib.GError: File type documento PDF (application/pdf) is not supported

Any idea about what this mean?

I have compiled from git, applying all these 3 patches.
Comment 7 Gonzalo Odiard 2011-11-02 13:49:56 UTC
forget the previous comment.

I needed initialize before. 

Now I can open and show a pdf file.
Comment 8 Gonzalo Odiard 2011-11-02 14:05:19 UTC
*** Bug 662709 has been marked as a duplicate of this bug. ***
Comment 9 Carlos Garcia Campos 2011-11-02 17:47:03 UTC
Review of attachment 175175 [details] [review]:

This is unrelated to this bug, please next time file another bug report.
Comment 10 Carlos Garcia Campos 2011-11-02 18:00:11 UTC
Review of attachment 175176 [details] [review]:

This makes the struct a boxed type, but it's not used, you should update ev_document_synctex_backward_search() to use ev_source_link_new() and where the struct is freed to use ev_source_link_free()

::: libdocument/ev-document.c
@@ +744,3 @@
+ev_source_link_new (const gchar *filename, gint line, gint col) 
+{
+	EvSourceLink *link = g_new (EvSourceLink, 1);

Use g_slice instead of g_free

@@ +746,3 @@
+	EvSourceLink *link = g_new (EvSourceLink, 1);
+	
+	link->filename = g_strdup (filename);

Why duplicating it? we don't do that currently, I think this is supposed to be valid while the syntex scanner is alive. If you think we should duplicated it, don't declare the filename as const gchar *

@@ +760,3 @@
+	g_return_val_if_fail (link != NULL, NULL);
+	
+	copy = g_new (EvSourceLink, 1);

Use g_slice

::: libdocument/ev-document.h
@@ +199,3 @@
 };
 
+#define EV_SOURCE_LINK (ev_source_link ())

that should be 

#define EV_TYPE_SOURCE_LINK (ev_source_link_get_type())

@@ +208,3 @@
+
+GType          ev_source_link_get_type (void) G_GNUC_CONST;
+EvSourceLink  *ev_source_link_new      (const gchar *filename, gint line, gint col);

use a new line for every parameter

EvSourceLink  *ev_source_link_new      (const gchar *filename,
                                                                  gint               line, 
                                                                  gint               col);
Comment 11 Carlos Garcia Campos 2011-11-02 18:16:07 UTC
Review of attachment 188875 [details] [review]:

The documentation added is vague, I've reviewed some of the files, but the problems are the same in all other files. Feel free to ask on IRC or mailing list if you need more information about what the API does.

::: libdocument/ev-annotation.c
@@ +261,3 @@
+/**
+ * ev_annotation_get_page:
+ *

parameter docs are missing here

@@ +262,3 @@
+ * ev_annotation_get_page:
+ *
+ * Get the #EvPage of an #EvAnnotation.

Please explain here that it's the page where the annotation is.

@@ +264,3 @@
+ * Get the #EvPage of an #EvAnnotation.
+ *
+ * Returns: (transfer none):

Text it missing here after Returns:

@@ +985,3 @@
+/**
+ * ev_annotation_attachment_get_attachment:
+ *

parameter docs are missing here

@@ +986,3 @@
+ * ev_annotation_attachment_get_attachment:
+ *
+ * Get an attachment of an annotation.

Get the #EvAttachment of @annot

::: libdocument/ev-backends-manager.c
@@ +223,3 @@
+/**
+ * ev_backends_manager_get_document:
+ *  @mime_type: a string containing a mime-type (e.g "text/plain")

Extra space between * and @mime_type. This should say something like the mime-type of a document (and use pdf as an example instead of plain/text)

@@ +224,3 @@
+ * ev_backends_manager_get_document:
+ *  @mime_type: a string containing a mime-type (e.g "text/plain")
+ *

You should explain what this function does here.

@@ +225,3 @@
+ *  @mime_type: a string containing a mime-type (e.g "text/plain")
+ *
+ * Returns: (transfer full): A #EvDocument or %NULL

You should say here or in the comment body when and why this function might return NULL. For example:

Returns: (transfer full): A #EvDocument or %NULL if @mime_type is not supported document format.

@@ +321,3 @@
+/**
+ * ev_backends_manager_get_all_types_info:
+ *

Body is missing here too.

@@ +322,3 @@
+ * ev_backends_manager_get_all_types_info:
+ *
+ * Returns: (transfer full): A #GList of #EvTypeInfo

Add element-type annotation here too.

::: libdocument/ev-document-annotations.c
@@ +30,3 @@
 
+/**
+ * ev_document_annotations_get_annotations:

parameter @document_annots is missing here

@@ +31,3 @@
+/**
+ * ev_document_annotations_get_annotations:
+ *  @page: a page

Extra space between * and @page

@@ +33,3 @@
+ *  @page: a page
+ *
+ * Get the annotations for a page.

This should be something like Get the list of annotations of @page.

@@ +35,3 @@
+ * Get the annotations for a page.
+ *
+ * Returns: (transfer none): the annotations

Add element-type and that this returns a #GList
Comment 12 José Aliste 2011-11-03 11:54:39 UTC
Created attachment 200582 [details] [review]
libdocument: document EvAnnotation.
Comment 13 José Aliste 2011-11-03 17:07:37 UTC
Created attachment 200619 [details] [review]
libdocument: Document EvAnnotation

The previous patch had a doc for EvAnnotationAttachment that I want to push later (as it is not finished yet). This patch is complete and only documents EvAnnotation.
Comment 14 Carlos Garcia Campos 2011-11-03 17:22:53 UTC
Review of attachment 200582 [details] [review]:

::: libdocument/ev-annotation.c
@@ +265,3 @@
+ * Get the page where @annot appears.
+ *
+ * Returns: (transfer none): an #EvPage referencing the page.

the #EvPage where annotation appears

@@ +279,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Get the page index where @annot appears.

You should mention that page index is 0 based.

@@ +281,3 @@
+ * Get the page index where @annot appears.
+ *
+ * Returns: the page index.

the index of the page where annotation appears.

@@ +298,3 @@
+ * Compares @annot and @other.
+ *
+ * Returns: TRUE if both annots have the same name or FALSE otherwise. 

Use %TRUE and %FALSE.

@@ +314,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Get the contents of @annot.

You should explain here what the contents of an annotation is, this is what the PDF reference says:


Optional) Text that shall be displayed for the annotation or, if this type of
annotation does not display text, an alternate description of the
annotation’s contents in human-readable form. In either case, this text is
 useful when extracting the document’s contents in support of
  accessibility to users with disabilities or for other purposes (see 14.9.3,
 “Alternate Descriptions”). See 12.5.6, “Annotation Types” for more
        details on the meaning of this entry for each annotation type.

@@ +316,3 @@
+ * Get the contents of @annot.
+ *
+ * Returns: (transfer-mode none): a string contaning  the contents.

You don't need the transfer tag here because the function returns a const gchar *.
It could be just: a string with the contents of the annotation

@@ +330,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Set the contents of @annot and notifies when the contents have change. 

Instead of saying the g_object_notify is called, you could say something like:

You can monitor changes in the annotation contents by connecting to notify::contents signal of @annot.

@@ +332,3 @@
+ * Set the contents of @annot and notifies when the contents have change. 
+ *
+ * Returns: TRUE if the contents were changed, FALSE otherwise.

Use %TRUE and %FALSE

@@ +356,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Get the name of @annot.

You should explain here what the name of an annotation is.

@@ +358,3 @@
+ * Get the name of @annot.
+ *
+ * Returns: (transfer none): the string with the name.

You don't need transfer tag here either. 
a string with the annotation name

@@ +372,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Set the name of @annot and notifies if the name was changed.

Same here about the notify

@@ +374,3 @@
+ * Set the name of @annot and notifies if the name was changed.
+ *
+ * Returns: TRUE when the name is changed, FALSE otherwise.

%TRUE and %FALSE

@@ +398,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Get the modified field of @annot.

modified is the last modification date as a string

@@ +400,3 @@
+ * Get the modified field of @annot.
+ *
+ * Returns: (transfer-mode none): A string containing the modified field.

Don't need transfer tag

@@ +410,3 @@
 }
 
+/** ev_annotation_set_modified:

this should be in a new line

@@ +413,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Set the modified field of @annot and notifies if the field has changed.

Same here, modified is the last modification date

@@ +414,3 @@
+ *
+ * Set the modified field of @annot and notifies if the field has changed.
+ *

You could reference ev_annotation_set_modified_from_time() here, using just See also: or something like: To set the last modification date using a #GTime use ev_annotation_set_modified_from_time().

@@ +415,3 @@
+ * Set the modified field of @annot and notifies if the field has changed.
+ *
+ * Returns: TRUE if the modified field was changed, FALSE otherwise.

%TRUE if annotation last modification date has been updated, %FALSE otherwise

@@ +435,3 @@
 }
 
+/** ev_annotation_set_modified_from_time:

this should be in a new line

@@ +441,3 @@
+ * Set the modified field of @annot to the @utime and notifies 
+ * if the modified field has changed. For the time-format
+ * used, see #ev_document_misc_format_date.

functions don't need # just use ev_document_misc_format_date()

@@ +443,3 @@
+ * used, see #ev_document_misc_format_date.
+ *
+ * Returns: TRUE if the modified field has changed, FALSE otherwise.

%TRUE and %FALSE

@@ +469,3 @@
 }
 
+/** ev_annotation_get_color:

this should be in a new line

@@ +473,3 @@
+ * @color (out): a #GdkColor to be filled with the Annotation color.
+ *
+ * Gets the color of @annot.

You are using Get/Set in the other function, and here Gets, I don't mind which one but use the same everywhere for consistency

@@ +474,3 @@
+ *
+ * Gets the color of @annot.
+ *

Extra line here.

@@ +486,3 @@
 }
 
+/** ev_annotation_set_color:

this should be in a new line

@@ +1076,3 @@
+/**
+ * ev_annotation_attachment_get_attachment:
+ *

parameters missing here, I already reviewed this, I guess you forgot to update this method.
Comment 15 José Aliste 2011-11-03 22:21:09 UTC
Created attachment 200651 [details] [review]
libdocument: Document EvAnnotation

Corrected patch addressing all comments (and not adding the get_attachment doc that will come in a new patch)
Comment 16 José Aliste 2011-11-04 01:13:53 UTC
Created attachment 200657 [details] [review]
libdocument: Make EvSourceLink a Boxed Type.
Comment 17 José Aliste 2011-11-04 01:14:54 UTC
Review of attachment 175176 [details] [review]:

Thanks for the review.

::: libdocument/ev-document.c
@@ +746,3 @@
+	EvSourceLink *link = g_new (EvSourceLink, 1);
+	
+	link->filename = g_strdup (filename);

I believe that is better to duplicate it so we control the life of the string.
Comment 18 José Aliste 2011-11-06 02:11:51 UTC
Created attachment 200806 [details] [review]
libdocument: Add missing annotation to ev_document_factory_get_document.

Pointed out by Lucas Stadler.
Comment 19 Carlos Garcia Campos 2011-11-10 09:52:41 UTC
Review of attachment 200651 [details] [review]:

::: libdocument/ev-annotation.c
@@ +299,3 @@
+ * Compare @annot and @other. 
+ *
+ * Returns: %TRUE if both annots have the same name or %FALSE otherwise. 

%TRUE if @annot is equal to @other, %FALSE otherwise 

The fact that we are comparing the names is an implementation detail.

@@ +315,3 @@
+ * @annot: an #EvAnnotation
+ *
+ * Get the contents of @annot. From the PDF Spec, the contents of 

Remove "From the PDF Spec". This is libdocument, which is supposed to be independent on the backends, we are following the PDF spec as a standard because it's the only backend implementing annotations, but the doc should be generic.

@@ +319,3 @@
+ * alternate description of the annotation's content for non-text annotations
+ * 
+ * Returns: a string with the contents of the annotation.

or %NULL if annotation doesn't have contents

@@ +383,3 @@
+ * to the notify::name signal on @annot.
+ *
+ * Returns: %TRUE when the name is changed, %FALSE otherwise.

Please, try to be consistent, in set_contents you used "were changed" and here "is changed"

@@ +424,3 @@
+ * @modified: string with the last modification date.
+ *
+ * Set the last modification date of @annot using @modified. You can 

"using @modified" -> "to @modified"
"You can" -> "To"

@@ +454,3 @@
+ *
+ * Set the modified field of @annot to the @utime and notifies 
+ * if the modified field has changed. For the time-format

Remove the "and notifies ..." and add "You can monitor ..." like in the other methods.

@@ +457,3 @@
+ * used, see ev_document_misc_format_date().
+ *
+ * Returns: %TRUE if the last modified date has been updated, %FALSE otherwise.

And here you use "has been", I'm not english expert so I don't know which one is better, but please, use the same on all method for consistency.

@@ +505,3 @@
+ * @color: a #GdkColor
+ *
+ * Set the color of the annotation to match @color. You can monitor 

"Set the color of annotation to @color"

@@ +507,3 @@
+ * Set the color of the annotation to match @color. You can monitor 
+ * changes to the annotation's color by connecting to 
+ * notify::color on @annot. 

notify::color signal on @annot.

@@ +1091,3 @@
 
+/**
+ * ev_annotation_attachment_get_attachment:

This is still here.
Comment 20 Carlos Garcia Campos 2011-11-10 10:03:54 UTC
Review of attachment 200657 [details] [review]:

You need to update ev_view_synctex_backward_search() (and any other places where EvSourceLink is used, if any) to use ev_source_link_free() instead of g_free().

::: libdocument/ev-document.c
@@ +741,3 @@
+
+EvSourceLink *
+ev_source_link_new (const gchar *filename, gint line, gint col)

Use a different line for every parameter.

ev_source_link_new (const gchar *filename,
                                   gint                line,
                                   gint                col)

@@ +774,3 @@
+
+	g_free (link->filename);
+	g_free (link);

That should be g_slice_free()

@@ +784,3 @@
 {
 	EvDocumentInfo *copy;
+

Please don't include unrelated changes in the patch.

@@ +797,3 @@
 	copy->producer = g_strdup (info->producer);
 	copy->linearized = g_strdup (info->linearized);
+

Ditto.

::: libdocument/ev-document.h
@@ +199,3 @@
 };
 
+#define EV_SOURCE_LINK (ev_source_link_get_type ())

This should be EV_TYPE_SOURCE_LINK()
Comment 21 Carlos Garcia Campos 2011-11-10 10:04:52 UTC
Review of attachment 200657 [details] [review]:

You need to update ev_view_synctex_backward_search() (and any other places where EvSourceLink is used, if any) to use ev_source_link_free() instead of g_free().

::: libdocument/ev-document.c
@@ +741,3 @@
+
+EvSourceLink *
+ev_source_link_new (const gchar *filename, gint line, gint col)

Use a different line for every parameter.

ev_source_link_new (const gchar *filename,
                                   gint                line,
                                   gint                col)

@@ +774,3 @@
+
+	g_free (link->filename);
+	g_free (link);

That should be g_slice_free()

@@ +784,3 @@
 {
 	EvDocumentInfo *copy;
+

Please don't include unrelated changes in the patch.

@@ +797,3 @@
 	copy->producer = g_strdup (info->producer);
 	copy->linearized = g_strdup (info->linearized);
+

Ditto.

::: libdocument/ev-document.h
@@ +199,3 @@
 };
 
+#define EV_SOURCE_LINK (ev_source_link_get_type ())

This should be EV_TYPE_SOURCE_LINK()
Comment 22 Carlos Garcia Campos 2011-11-10 10:45:07 UTC
Review of attachment 200806 [details] [review]:

Thanks, please push it.
Comment 23 José Aliste 2011-11-11 01:06:35 UTC
Review of attachment 200657 [details] [review]:

Hey, thanks for the review. I am sorry that some of the changes in previous reviews didn't make it into the patch. 

I have corrected the patches, but have some question about the use of SourceLink in ev-window.c (line 7013), should I use boxed methods there also, or should I keep the stack allocated EvSourceLink?
Comment 24 José Aliste 2011-11-11 01:07:03 UTC
Review of attachment 200657 [details] [review]:

Hey, thanks for the review. I am sorry that some of the changes in previous reviews didn't make it into the patch. 

I have corrected the patches, but have some question about the use of SourceLink in ev-window.c (line 7013), should I use boxed methods there also, or should I keep the stack allocated EvSourceLink?
Comment 25 Carlos Garcia Campos 2011-11-12 08:30:43 UTC
stack allocated is preferred when possible.
Comment 26 José Aliste 2011-11-15 00:00:27 UTC
Created attachment 201411 [details] [review]
libdocument: Make EvSourceLink boxed.
Comment 27 José Aliste 2011-11-15 00:00:34 UTC
Created attachment 201412 [details] [review]
libdocument: Document EvAnnotation.
Comment 28 José Aliste 2011-11-15 00:00:38 UTC
Created attachment 201413 [details] [review]
libview: Use ev_source_link_free instead of g_free.
Comment 29 José Aliste 2011-11-15 00:01:27 UTC
Comment on attachment 200806 [details] [review]
libdocument: Add missing annotation to ev_document_factory_get_document.

pushed to git master! thanks
Comment 30 José Aliste 2011-11-15 00:03:15 UTC
Carlos, here are the updated patches. Thanks for your patience, hopefully now they are ready to go. Notice there is a new patch for libview to use ev_source_link_free method.
Comment 31 Carlos Garcia Campos 2011-11-15 16:34:54 UTC
Review of attachment 201411 [details] [review]:

You should include in this patch the change to use ev_source_link_free() in ev-view, or this revision will be broken. Please merge both patches and push it, thanks!
Comment 32 Carlos Garcia Campos 2011-11-15 16:35:42 UTC
Review of attachment 201413 [details] [review]:

This should be merged in the commit that makes EvSourceLink a boxed type.
Comment 33 Carlos Garcia Campos 2011-11-15 16:37:54 UTC
Review of attachment 201412 [details] [review]:

Looks good, thanks!
Comment 34 José Aliste 2011-11-16 01:44:26 UTC
Comment on attachment 201411 [details] [review]
libdocument: Make EvSourceLink boxed.

merged with the other patch and committed, thanks
Comment 35 José Aliste 2011-11-16 01:45:26 UTC
Comment on attachment 201413 [details] [review]
libview: Use ev_source_link_free instead of g_free.

merged with the other patch and committed
Comment 36 Germán Poo-Caamaño 2017-09-26 20:05:10 UTC
Review of attachment 188875 [details] [review]:

The API has already been documented. This patch is obsolete.
Comment 37 Germán Poo-Caamaño 2017-09-26 20:06:18 UTC
If there is new API to add, we can open a new bug. Other than that, this bug seems fixed eons ago.