Bug 70981

Summary: glib-demo: Add support for simple line annotations
Product: poppler Reporter: Germán Poo-Caamaño <gpoo+bfdo>
Component: glib frontendAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 69978, 70901, 71727    
Bug Blocks:    
Attachments: glib: Add support for simple line annotations
glib-demo: Add simple demo to add line annotations
glib: Add PopplerPoint boxed type
glib: Add support for simple line annotations
glib-demo: Add simple demo to add line annotations
glib: Add PopplerPoint boxed type
glib: Add support for simple line annotations
glib: Add support for simple line annotations
glib-demo: add color selection for new annotations

Description Germán Poo-Caamaño 2013-10-29 07:12:18 UTC
Created attachment 88267 [details] [review]
glib: Add support for simple line annotations

Here a couple of patches to add support for Line annotations in poppler-glib.

The patch for glib-demo depends on https://bugs.freedesktop.org/show_bug.cgi?id=70901
Comment 1 Germán Poo-Caamaño 2013-10-29 07:12:54 UTC
Created attachment 88268 [details] [review]
glib-demo: Add simple demo to add line annotations
Comment 2 Carlos Garcia Campos 2013-11-03 11:07:51 UTC
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.
Comment 3 Germán Poo-Caamaño 2013-11-16 09:43:40 UTC
Created attachment 89307 [details] [review]
glib: Add PopplerPoint boxed type

Expose PopplerPoint to be used for lines.
Comment 4 Germán Poo-Caamaño 2013-11-16 09:44:36 UTC
Created attachment 89308 [details] [review]
glib: Add support for simple line annotations

Updated patch
Comment 5 Germán Poo-Caamaño 2013-11-16 09:45:24 UTC
Created attachment 89309 [details] [review]
glib-demo: Add simple demo to add line annotations

Updated patch
Comment 6 Germán Poo-Caamaño 2013-11-16 09:50:48 UTC
(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.
Comment 7 Germán Poo-Caamaño 2013-11-16 09:55:07 UTC
FWIW. The demo depends on patch https://bugs.freedesktop.org/attachment.cgi?id=88199 from Bug 69978
Comment 8 Carlos Garcia Campos 2013-11-17 11:41:41 UTC
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 9 Carlos Garcia Campos 2013-11-17 11:47:02 UTC
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 10 Carlos Garcia Campos 2013-11-17 12:27:50 UTC
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
Comment 11 Germán Poo-Caamaño 2013-11-18 07:32:15 UTC
Created attachment 89384 [details] [review]
glib: Add PopplerPoint boxed type

Updated patch (moved from poppler-annot to poppler-page)
Comment 12 Germán Poo-Caamaño 2013-11-18 07:39:32 UTC
Created attachment 89385 [details] [review]
glib: Add support for simple line annotations

Updated patch.
Comment 13 Germán Poo-Caamaño 2013-11-18 07:40:46 UTC
(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?
Comment 14 Germán Poo-Caamaño 2013-11-18 08:20:59 UTC
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
Comment 15 Germán Poo-Caamaño 2013-11-18 09:09:22 UTC
Created attachment 89391 [details] [review]
glib-demo: add color selection for new annotations

Updated patch. I think I addressed all comments.
Comment 16 Germán Poo-Caamaño 2013-11-18 09:10:03 UTC
The patch for the demo now depends on https://bugs.freedesktop.org/show_bug.cgi?id=71727 (which is simple).
Comment 17 Carlos Garcia Campos 2013-11-18 13:43:52 UTC
(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.
Comment 18 Germán Poo-Caamaño 2013-11-18 19:34:31 UTC
(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 19 Carlos Garcia Campos 2013-11-19 17:07:46 UTC
Comment on attachment 89384 [details] [review]
glib: Add PopplerPoint boxed type

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

Ok, pushed to git master.
Comment 20 Carlos Garcia Campos 2013-11-19 17:09:17 UTC
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 21 Carlos Garcia Campos 2013-11-19 17:13:37 UTC
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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.