Bug 45384

Created attachment 56312 [details] [review]
glib: introspection fixes issues, don't expose PopplerAttachmentClass

(In reply to comment #0)
> I noticed a few issues while working on the Vala bindings:
> 
> 1. Poppler.ActionLayer.layers is missing an element-type annotation
> 2. Poppler.ActionOCGState.state_list is missing an element-type annotation
> 3. PopplerAttachmentClass is exposed publicly when it probably shouldn't be
> 
> The first two are relatively straightforward; simply add the annotations. The
> third is a little more complicated. Removing the struct from the public headers
> is technically an API break, but it's quite unlikely that anyone is actually
> using the struct and it wasn't documented so IMHO it's okay to remove it.

You are right, but it's not enough reason to break the API.

Comment on attachment 56312 [details] [review]
glib: introspection fixes issues, don't expose PopplerAttachmentClass

Review of attachment 56312 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch.

::: glib/poppler-action.h
@@ +261,5 @@
>  };
>  
> +/**
> + * PopplerActionOCGState:
> + * @type: action type

We know the action type, so add it in parentheses:

@type: action type (%POPPLER_ACTION_OCG_STATE)

@@ +263,5 @@
> +/**
> + * PopplerActionOCGState:
> + * @type: action type
> + * @title: action title
> + * @state_list: (element-type PopplerActionLayer): list of #PopplerActionLayer<-- -->s

We should explain briefly what this action is for.

@@ +264,5 @@
> + * PopplerActionOCGState:
> + * @type: action type
> + * @title: action title
> + * @state_list: (element-type PopplerActionLayer): list of #PopplerActionLayer<-- -->s
> + */

All other action types are also undocumented, I guess it's a good time to document all of them :-P

Created attachment 74809 [details] [review]
glib: add documentation for PopplerActions

> ::: glib/poppler-action.h
> @@ +261,5 @@
> >  };
> >  
> > +/**
> > + * PopplerActionOCGState:
> > + * @type: action type
> 
> We know the action type, so add it in parentheses:
> 
> @type: action type (%POPPLER_ACTION_OCG_STATE)

Okay, done for all of them.

> @@ +263,5 @@
> > +/**
> > + * PopplerActionOCGState:
> > + * @type: action type
> > + * @title: action title
> > + * @state_list: (element-type PopplerActionLayer): list of #PopplerActionLayer<-- -->s
> 
> We should explain briefly what this action is for.

I honestly have no idea what this action is for.  Or any of them, really.  I'm not at all familiar with the Poppler API, just trying to get introspection up to snuff.

I've copied what could be called, if you're feeling generous, brief explanations from the PopplerActionType enum, but it would be nice if someone who knows the API could improve it.

> @@ +264,5 @@
> > + * PopplerActionOCGState:
> > + * @type: action type
> > + * @title: action title
> > + * @state_list: (element-type PopplerActionLayer): list of #PopplerActionLayer<-- -->s
> > + */
> 
> All other action types are also undocumented, I guess it's a good time to
> document all of them :-P

I've done what I can.  To be honest I haven't really added any insight, but it should look a little better than empty docs.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/33.