[Poppler-bugs] [Bug 70981] glib-demo: Add support for simple line annotations

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=70981

--- Comment #10 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 89309
  --> https://bugs.freedesktop.org/attachment.cgi?id=89309
glib-demo: Add simple demo to add line annotations

Review of attachment 89309:
-----------------------------------------------------------------

::: 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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20131117/339b88d8/attachment.html>