Created attachment 71197 [details] [review]
patch: add poppler_annot_set_flags

Created attachment 71198 [details] [review]
patch: update the demo

This seems like a feature so unless you can convince me it's really a bugfix maybe we should wait for 0.24 since 0.22 is already in feature freeze

This is new feature, yes, so it should wait until 0.24.

Comment on attachment 71197 [details] [review]
patch: add poppler_annot_set_flags

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

Patch looks good to me. Albert, the patch is simple and not risky, I think it can go in 0.22.

::: glib/poppler-annot.cc
@@ +513,5 @@
> + * @flags: a #PopplerAnnotFlag
> + *
> + * Sets the flag field specifying various characteristics of the
> + * @poppler_annot.
> + *

Please add a Since: tag here 0.22 or 0.24

@@ +520,5 @@
> +poppler_annot_set_flags (PopplerAnnot *poppler_annot, PopplerAnnotFlag flags)
> +{
> +  g_return_if_fail (POPPLER_IS_ANNOT (poppler_annot));
> +
> +  return poppler_annot->annot->setFlags ((guint) flags);

Remove the return, since the method is void. Also, add an early return if flags don't change, since Annot::setFlags doesn't check it and the xref will be updated and the document marked as modified.

Created attachment 71205 [details] [review]
updated patch: add_poppler_annot_set_flags

Comments addressed. I added Since:0.22 tag in case Albert decides to get it into 0.22.

Ok, if you want it in 0.22, put it in

Comment on attachment 71198 [details] [review]
patch: update the demo

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

Looks great, I have a just some minor comments

::: glib/demo/annots.c
@@ +694,5 @@
> +      case ANNOTS_FLAG_HIDDEN_COLUMN:
> +        flag_bit = POPPLER_ANNOT_FLAG_HIDDEN;
> +        break;
> +      default:
> +        flag_bit = POPPLER_ANNOT_FLAG_PRINT;

Instead of default, add the case ANNOTS_FLAG_PRINT_COLUMN: better, and add default: doing nothing or with g_assert_not_reached().

@@ +699,5 @@
> +        break;
> +    }
> +
> +    if (fixed)
> +        flags = flags | flag_bit;

This could be flags |= flag_bit no?

@@ +701,5 @@
> +
> +    if (fixed)
> +        flags = flags | flag_bit;
> +    else
> +        flags = flags & (~flag_bit);

And this flags &= ~flag_bit

@@ +941,4 @@
>                                                   NULL);
>  
>      renderer = gtk_cell_renderer_toggle_new ();
> +    g_signal_connect (G_OBJECT (renderer), "toggled",

g_signal_connect expects a gpointer so no need to cast to GObject.

@@ +965,5 @@
>      renderer = gtk_cell_renderer_toggle_new ();
> +    g_signal_connect (G_OBJECT (renderer), "toggled",
> +                      G_CALLBACK (pgd_annots_flags_toggled),
> +                      (gpointer) demo);
> +    g_object_set_data (G_OBJECT (renderer), "column", GINT_TO_POINTER (ANNOTS_FLAG_PRINT_COLUMN));

I think it would be simpler to add three different callbacks and call a custom method passing the column

Created attachment 71213 [details] [review]
updated patch: demo

Comments addressed.

Pushed both patches to git master, thanks!