Created attachment 88268 [details] [review]
glib-demo: Add simple demo to add line annotations

Comment on attachment 88267 [details] [review]
glib: Add support for simple line annotations

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

::: glib/poppler-annot.cc
@@ +351,5 @@
> +{
> +}
> +
> +/**
> +* poppler_annot_line_new:

This line is not correctly indented.

@@ +359,5 @@
> + * Creates a new Line annotation that will be
> + * located on @rect when added to a page. See
> + * poppler_page_add_annot()
> + *
> + * Return value: A newly #PopplerAnnotLine annotation

newly created

@@ +361,5 @@
> + * poppler_page_add_annot()
> + *
> + * Return value: A newly #PopplerAnnotLine annotation
> + *
> + * Since: 0.25

0.26.

@@ +1439,5 @@
>  }
> +
> +/* PopplerAnnotLine */
> +void
> +poppler_annot_line_set_width (PopplerAnnotLine *poppler_annot, gdouble width)

This should be documented. Use a new line for every parameter-

@@ +1447,5 @@
> +  g_return_if_fail (POPPLER_IS_ANNOT_LINE (poppler_annot));
> +
> +  annot = static_cast<AnnotLine *>(POPPLER_ANNOT (poppler_annot)->annot);
> +  annot->setLeaderLineLength(width);
> +  annot->setLeaderLineExtension(width);

This is confusing, because this only applies to annot lines using leader lines. Note that simply calling this, changes the meaning of the vertices. In a normal lines the width is determined but its vertices, since it's a rectangle in the end. I'm not sure how to expose the leader lines thing, though.

@@ +1452,5 @@
> +  // annot->setOpacitiy(0.5);
> +}
> +
> +void
> +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,

This should be documented too.

@@ +1454,5 @@
> +
> +void
> +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> +                                 gdouble x1, gdouble y1,
> +                                 gdouble x2, gdouble y2)

I wonder if we should to expose PopplerPoint, I guess it will be required if we eventually expose Polygon annotations.

@@ +1498,5 @@
> +/**
> + * poppler_annot_line_get_start_style:
> + * @poppler_annot: a #PopplerAnnotLine
> + *
> + * Return value: (transfer none): The ending style located at the start of the line.

You don't need to use transfer annotations for a method returning an enum.

@@ +1524,5 @@
> + *
> + * Since: 0.25
> + **/
> +PopplerAnnotLineEndingStyle
> +poppler_annot_line_get_end_style (PopplerAnnotLine *poppler_annot)

I think I would merge both methods into a single poppler_annot_line_get_ending_style that returns both start and end as out parameters, allowing to pass NULL if you are not interested in any of them.

@@ +1556,5 @@
> +
> +  annot = static_cast<AnnotLine *>(POPPLER_ANNOT (poppler_annot)->annot);
> +
> +  annot->setStartEndStyle ((AnnotLineEndingStyle)start_style,
> +			   (AnnotLineEndingStyle)end_style);

Why are we just casting the enums here, but using a convert method in the getters? We can either always cast (adding a compile assert to make sure we break the build if the enums are not in sync) or use converter methods, but not mix it.

Created attachment 89307 [details] [review]
glib: Add PopplerPoint boxed type

Expose PopplerPoint to be used for lines.

Created attachment 89308 [details] [review]
glib: Add support for simple line annotations

Updated patch

Created attachment 89309 [details] [review]
glib-demo: Add simple demo to add line annotations

Updated patch

(In reply to comment #2)
> @@ +1439,5 @@
> >  }
> > +
> > +/* PopplerAnnotLine */
> > +void
> > +poppler_annot_line_set_width (PopplerAnnotLine *poppler_annot, gdouble width)
> 
> This should be documented. Use a new line for every parameter-
> 
> @@ +1447,5 @@
> > +  g_return_if_fail (POPPLER_IS_ANNOT_LINE (poppler_annot));
> > +
> > +  annot = static_cast<AnnotLine *>(POPPLER_ANNOT (poppler_annot)->annot);
> > +  annot->setLeaderLineLength(width);
> > +  annot->setLeaderLineExtension(width);
> 
> This is confusing, because this only applies to annot lines using leader
> lines. Note that simply calling this, changes the meaning of the vertices.
> In a normal lines the width is determined but its vertices, since it's a
> rectangle in the end. I'm not sure how to expose the leader lines thing,
> though.

None of them seems to work.  I had forgotten to split the patch because I was
testing some things with this.

> @@ +1454,5 @@
> > +
> > +void
> > +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> > +                                 gdouble x1, gdouble y1,
> > +                                 gdouble x2, gdouble y2)
> 
> I wonder if we should to expose PopplerPoint, I guess it will be required if
> we eventually expose Polygon annotations.

It is kind of redundant to initialize both, PopplerRect and PopplerPoint with
similar values. However, PopplerPoint seems to make the API simpler to understand.

I removed everything else that is only noise for very simple lines, and they
were not working.  In Poppler there are some "TODO" marked in the implementation
of lines, so better to leave it for later.

FWIW. The demo depends on patch https://bugs.freedesktop.org/attachment.cgi?id=88199 from Bug 69978

Comment on attachment 89307 [details] [review]
glib: Add PopplerPoint boxed type

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

Looks good, but I don't think we should assume a point represents only the position of an annotation in the page, I think it's generic even if it's only used by the annotations API at the moment. So, I would move it to poppler-page, like PopplerRectangle or PopplerColor.

::: glib/poppler-annot.h
@@ +178,5 @@
> + * @x: x coordinate
> + * @y: y coordinate
> + *
> + * A #PopplerPoint is used to describe a location point for an annotation
> + * on a page

s/for an annotation//

Comment on attachment 89308 [details] [review]
glib: Add support for simple line annotations

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

Looks great, I have just a few minor comments

::: glib/poppler-annot.cc
@@ +1495,5 @@
> +/* PopplerAnnotLine */
> +/**
> + * poppler_annot_line_set_vertices:
> + * @poppler_annot: a #PopplerAnnotLine
> + * @start: Coordinates of the starting vertice

a #PopplerPoint ... so that we have a link ot the PopplerPoint documentation here

@@ +1496,5 @@
> +/**
> + * poppler_annot_line_set_vertices:
> + * @poppler_annot: a #PopplerAnnotLine
> + * @start: Coordinates of the starting vertice
> + * @end: Coordinates of the starting vertice

Ditto.

@@ +1498,5 @@
> + * @poppler_annot: a #PopplerAnnotLine
> + * @start: Coordinates of the starting vertice
> + * @end: Coordinates of the starting vertice
> + *
> + * Set the point the coordinates where the @poppler_annot starts and ends.

the point the coordinates?

@@ +1505,5 @@
> + */
> +void
> +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> +                                 PopplerPoint start,
> +				 PopplerPoint end)

Use pointers for start and end

@@ +1509,5 @@
> +				 PopplerPoint end)
> +{
> +  AnnotLine *annot;
> +
> +  g_return_if_fail (POPPLER_IS_ANNOT_LINE (poppler_annot));

check also here that start and end are both != NULL

Comment on attachment 89309 [details] [review]
glib-demo: Add simple demo to add line annotations

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

::: glib/demo/annots.c
@@ +68,4 @@
>      ModeType         mode;
>  
>      cairo_surface_t *surface;
> +    PopplerColor    *annot_color;

This is not specific to line annots. I think you could split the patch and add first support for setting the color and then support for lines.

@@ +86,5 @@
>  
> +	if (demo->annotations_idle > 0) {
> +		g_source_remove (demo->annotations_idle);
> +		demo->annotations_idle = 0;
> +	}

This is not correctly indented.

@@ +368,5 @@
>              break;
> +        case POPPLER_ANNOT_LINE:
> +            demo->mode = MODE_DRAWING;
> +            pgd_annots_update_cursor (demo, GDK_TCROSS);
> +            break;

Now that all annots transition to the same states we don't need any change in start_add_annot

@@ +461,5 @@
> +pgd_annot_color_changed (GtkButton     *button,
> +                         GParamSpec    *pspec,
> +                         PgdAnnotsDemo *demo)
> +{
> +    GdkRGBA      color;

Extra spaces between GdkRGBA and color.

@@ +476,5 @@
> +
> +static void
> +pgd_annot_view_set_annot_line (PgdAnnotsDemo *demo,
> +                               GtkWidget     *table,
> +                               PopplerAnnot  *annot,

This should receive a PopplerAnnotLine

@@ +483,5 @@
> +    PopplerColor *poppler_color;
> +    GdkPixbuf    *pixbuf;
> +    GtkWidget    *image;
> +
> +    if ((poppler_color = poppler_annot_get_color (annot))) {

Why does this depend on having a color?

@@ +484,5 @@
> +    GdkPixbuf    *pixbuf;
> +    GtkWidget    *image;
> +
> +    if ((poppler_color = poppler_annot_get_color (annot))) {
> +        demo->active_annot = annot;

I think this should be generic and done by pgd_annot_view_set_annot().

@@ +488,5 @@
> +        demo->active_annot = annot;
> +
> +        pixbuf = pgd_pixbuf_new_for_color (poppler_color);
> +        image = gtk_image_new ();
> +        gtk_image_set_from_pixbuf (GTK_IMAGE (image), pixbuf);

You can use gtk_image_set_from_pixbuf() directly here instead of new + set_pixbuf

@@ +491,5 @@
> +        image = gtk_image_new ();
> +        gtk_image_set_from_pixbuf (GTK_IMAGE (image), pixbuf);
> +        g_object_unref (pixbuf);
> +
> +        pgd_table_add_property_with_custom_widget (GTK_GRID (table), "<b>Color:</b>", image, row);

This is very confusing, because it's called set_annot_line, but the only property set is color, which is common to all annotations. The color is already shown for all annots in the tree view, why do we want to show it again here and only for lines?

@@ +696,1 @@
>  pgd_annots_add_annot_to_model (PgdAnnotsDemo *demo,

Instead of duplicating the iter here to return it, I think we could add a boolean argument, gboolean selected, for example, and when TRUE we set the cursor after inserting the annot in the model.

@@ +906,5 @@
> +    demo->active_annot = annot;
> +
> +    poppler_annot_set_color (annot, demo->annot_color);
> +    poppler_page_add_annot (demo->page, annot);
> +    iter = pgd_annots_add_annot_to_model (demo, annot, rect);

Here you would pass selected = TRUE

@@ +1025,5 @@
> +            break;
> +        case MODE_DRAWING:
> +            pgd_annots_add_annotation (demo);
> +        default:
> +            break;

With the current code you don't need to change button_press either.

@@ +1036,5 @@
> +pgd_annots_drawing_area_motion_notify (GtkWidget      *area,
> +                                       GdkEventMotion *event,
> +                                       PgdAnnotsDemo  *demo)
> +{
> +    if (!demo->page || demo->mode == MODE_NORMAL || demo->mode == MODE_ADD)

You could return early when mode != DRAWING here.

@@ +1039,5 @@
> +{
> +    if (!demo->page || demo->mode == MODE_NORMAL || demo->mode == MODE_ADD)
> +        return FALSE;
> +
> +    if (demo->start.x != -1) {

You could move this to the previous if to return early when demo->start.x == -1

@@ +1043,5 @@
> +    if (demo->start.x != -1) {
> +        demo->stop.x = event->x;
> +        demo->stop.y = event->y;
> +
> +        if (demo->mode == MODE_DRAWING) {

You don't need this check, at this point we should always be in drawing mode. What you should check, though, is the annotation type, because the code below is not common to all annots when in drawing mode, but specific to line annotations.

@@ +1054,5 @@
> +            /* Keep the drawing within the page */
> +            demo->stop.x = MAX (demo->stop.x, 0);
> +            demo->stop.y = MAX (demo->stop.y, 0);
> +            demo->stop.x = MIN (demo->stop.x, width);
> +            demo->stop.y = MIN (demo->stop.y, height);

I think you could use CLAMP directly when setting the values above.

demo->stop.x = CLAMP(event->x, 0, width);
demo->stop.y = CLAMP(event->y, 0, height);

I thinks this simplifies the code a bit.

@@ +1069,5 @@
> +            pgd_annot_view_set_annot (demo, demo->active_annot);
> +            if (demo->annotations_idle == 0) {
> +                demo->annotations_idle =
> +                         g_idle_add ((GSourceFunc)pgd_annots_viewer_redraw,
> +                                     demo);

I think you can reuse pgd_annots_viewer_queue_redraw(), by modifying it to return early if there's a draw already scheduled and initializing annotations_idle when it schedules a new redraw. I think it's a good idea for all other cases where we are using ugd_annots_viewer_queue_redraw() in any case.

@@ +1098,5 @@
> +
> +	if (demo->annotations_idle > 0) {
> +		g_source_remove (demo->annotations_idle);
> +		demo->annotations_idle = 0;
> +	}

With the current code you don't need to change anything in button_release either

Created attachment 89384 [details] [review]
glib: Add PopplerPoint boxed type

Updated patch (moved from poppler-annot to poppler-page)

Created attachment 89385 [details] [review]
glib: Add support for simple line annotations

Updated patch.

(In reply to comment #9)
> Comment on attachment 89308 [details] [review] [review]
> [...]
> @@ +1505,5 @@
> > + */
> > +void
> > +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> > +                                 PopplerPoint start,
> > +				 PopplerPoint end)
> 
> Use pointers for start and end

why do you prefer pointers on this one?

Created attachment 89388 [details] [review]
glib: Add support for simple line annotations

Update patch. I had forgotten to update the prototype in poppler-annot.h

Created attachment 89391 [details] [review]
glib-demo: add color selection for new annotations

Updated patch. I think I addressed all comments.

The patch for the demo now depends on https://bugs.freedesktop.org/show_bug.cgi?id=71727 (which is simple).

(In reply to comment #13)
> (In reply to comment #9)
> > Comment on attachment 89308 [details] [review] [review] [review]
> > [...]
> > @@ +1505,5 @@
> > > + */
> > > +void
> > > +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> > > +                                 PopplerPoint start,
> > > +				 PopplerPoint end)
> > 
> > Use pointers for start and end
> 
> why do you prefer pointers on this one?

To avoid copying the struct like in the poppler_annot_set_rectangle case.

(In reply to comment #17)
> (In reply to comment #13)
> > (In reply to comment #9)
> > > Comment on attachment 89308 [details] [review] [review] [review] [review]
> > > [...]
> > > @@ +1505,5 @@
> > > > + */
> > > > +void
> > > > +poppler_annot_line_set_vertices (PopplerAnnotLine *poppler_annot,
> > > > +                                 PopplerPoint start,
> > > > +				 PopplerPoint end)
> > > 
> > > Use pointers for start and end
> > 
> > why do you prefer pointers on this one?
> 
> To avoid copying the struct like in the poppler_annot_set_rectangle case.

Ok, thanks.  FWIW, the patch addresses this and the other comments.

Comment on attachment 89384 [details] [review]
glib: Add PopplerPoint boxed type

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

Ok, pushed to git master.

Comment on attachment 89388 [details] [review]
glib: Add support for simple line annotations

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

LGTM, pushed to git master.

Comment on attachment 89391 [details] [review]
glib-demo: add color selection for new annotations

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

Looks great, thanks! I've pushed it to git master with some minor modifications.

::: glib/demo/annots.c
@@ +663,3 @@
>  {
> +    GtkTreeIter      iter;
> +    GtkTreePath      *path;

This is only used inside the if (selected) block, so it could be moved there.

@@ +846,4 @@
>  pgd_annots_add_annot (PgdAnnotsDemo *demo)
>  {
>      PopplerRectangle  rect;
> +    PopplerPoint      start, end;

This is only used for line annots, so it can be moved to the case POPPLER_ANNOT_LINE: block.

@@ +880,3 @@
>      g_object_unref (annot);
> +
> +    pgd_annots_viewer_queue_redraw (demo);

This is already called right after pgd_annots_add_annot

@@ +1013,5 @@
> +    demo->stop.y = event->y;
> +
> +    PopplerRectangle rect;
> +    PopplerPoint start, end;
> +    gdouble width, height;

Declarations block should be at the beginning of the function body.

@@ +1052,2 @@
>  
>      demo->start.x = -1;

This could be moved to the finish_add_annot method.

@@ +1054,5 @@
>  
> +	if (demo->annotations_idle > 0) {
> +		g_source_remove (demo->annotations_idle);
> +		demo->annotations_idle = 0;
> +	}

finish_add_annot schedules a new redraw, so if there's a pending redraw, it's better to not cancel it here.