GNOME Bugzilla – Bug 635705
API Annotation work for EvinceDocument and EvinceView
Last modified: 2017-09-26 20:06:18 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.
Created attachment 175175 [details] [review] Use G_DEFINE_BOXED instead of custom macro.
Created attachment 175176 [details] [review] Make EvSourceLink boxed
+ 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.
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?
Created attachment 188875 [details] [review] Some annotations for libdocument Really not sure if it is correct.
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):
+ Trace 228969
return info.invoke(*args, **kwargs)
Any idea about what this mean? I have compiled from git, applying all these 3 patches.
forget the previous comment. I needed initialize before. Now I can open and show a pdf file.
*** Bug 662709 has been marked as a duplicate of this bug. ***
Review of attachment 175175 [details] [review]: This is unrelated to this bug, please next time file another bug report.
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);
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
Created attachment 200582 [details] [review] libdocument: document EvAnnotation.
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.
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.
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)
Created attachment 200657 [details] [review] libdocument: Make EvSourceLink a Boxed Type.
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.
Created attachment 200806 [details] [review] libdocument: Add missing annotation to ev_document_factory_get_document. Pointed out by Lucas Stadler.
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.
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()
Review of attachment 200806 [details] [review]: Thanks, please push it.
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?
stack allocated is preferred when possible.
Created attachment 201411 [details] [review] libdocument: Make EvSourceLink boxed.
Created attachment 201412 [details] [review] libdocument: Document EvAnnotation.
Created attachment 201413 [details] [review] libview: Use ev_source_link_free instead of g_free.
Comment on attachment 200806 [details] [review] libdocument: Add missing annotation to ev_document_factory_get_document. pushed to git master! thanks
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.
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!
Review of attachment 201413 [details] [review]: This should be merged in the commit that makes EvSourceLink a boxed type.
Review of attachment 201412 [details] [review]: Looks good, thanks!
Comment on attachment 201411 [details] [review] libdocument: Make EvSourceLink boxed. merged with the other patch and committed, thanks
Comment on attachment 201413 [details] [review] libview: Use ev_source_link_free instead of g_free. merged with the other patch and committed
Review of attachment 188875 [details] [review]: The API has already been documented. This patch is obsolete.
If there is new API to add, we can open a new bug. Other than that, this bug seems fixed eons ago.