[Poppler-bugs] [Bug 69978] Make demo extensible for other type of annotations

[bugzilla-daemon at freedesktop.org]

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

--- Comment #28 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 88199
  --> https://bugs.freedesktop.org/attachment.cgi?id=88199
glib-demo: Add annotations interactively

Review of attachment 88199:
-----------------------------------------------------------------

Patch looks good in general, but I find some parts confusing, and I think it
would be simpler if all annotations transition to the same states:

 - on add annot -> mode = ADD
 - on button press -> mode = DRAWING
 - on button release -> mode = NORMAL

::: glib/demo/annots.c
@@ +59,5 @@
>      GtkWidget       *annot_view;
>      GtkWidget       *timer_label;
>      GtkWidget       *remove_button;
> +    GtkWidget       *type_selector;
> +    GtkWidget       *vbox;

This is very generic name, I would use main_box or something like that.

@@ +315,5 @@
>  static void
> +pgd_annots_update_cursor (PgdAnnotsDemo *demo,
> +                          GdkCursorType  cursor_type)
> +{
> +    GdkWindow *window = gtk_widget_get_window (demo->vbox);

This is used only once and only when cursor has changed, so we don't really
need the local variable.

@@ +350,5 @@
> +
> +    switch (demo->annot_type) {
> +        case POPPLER_ANNOT_TEXT:
> +            demo->mode = MODE_ADD;
> +            pgd_annots_update_cursor (demo, GDK_TCROSS);

I think we should transition to ADD mode and change the cursor for all
annotation types.

@@ +353,5 @@
> +            demo->mode = MODE_ADD;
> +            pgd_annots_update_cursor (demo, GDK_TCROSS);
> +            break;
> +        default:
> +            g_printerr ("Annotation not implemented: %d\n", demo->annot_type);

This is impossible to happen, since the annot_type is set from the values in
the combo box model that are hardcoded by us. We should use
g_assert_not_reached() instead.

@@ +812,4 @@
>  }
>  
>  static void
> +pgd_annots_add_annotation (PgdAnnotsDemo *demo)

This method name is a bit confusing, this is finishing an ADD operation started
by pgd_annots_start_add_annot, so this should be called something like
pgd_annots_finish_add_annot.

@@ +816,4 @@
>  {
> +    PopplerRectangle  rect;
> +    PopplerAnnot     *annot;
> +    gdouble           width, height;

width is unused.

@@ +820,2 @@
>  
> +    poppler_page_get_size (demo->page, &width, &height);

You can pass NULL instead of &width, since we are only interested in the page
height.

@@ +827,5 @@
> +    switch (demo->annot_type) {
> +        case POPPLER_ANNOT_TEXT:
> +            annot = poppler_annot_text_new (demo->doc, &rect);
> +            poppler_page_add_annot (demo->page, annot);
> +            pgd_annots_add_annot_to_model (demo, annot, rect);

I think these (add_annot + add_annot_to_model) should be common to all
annotations.

@@ +830,5 @@
> +            poppler_page_add_annot (demo->page, annot);
> +            pgd_annots_add_annot_to_model (demo, annot, rect);
> +            break;
> +        default:
> +            g_printerr ("Annotation not implemented: %d\n", demo->annot_type);

g_assert_not_reached() here too.

@@ +837,3 @@
>  
>      pgd_annots_viewer_queue_redraw (demo);
> +    demo->mode = MODE_NORMAL;

This method could actually be split in two: pgd_annots_add_annot() that creates
and inserts the annot in the page and pgd_annots_finish_add_annot() that
restores the mode to NORMAL and the cursor to last cursor. We should also
update the timer label, because after adding a new annot it doesn't make sense
anymore, specially when the document didn't contain annotations.

@@ +927,5 @@
> +pgd_annots_drawing_area_button_press (GtkWidget      *area,
> +                                      GdkEventButton *event,
> +                                      PgdAnnotsDemo  *demo)
> +{
> +    if (!demo->page || demo->mode == MODE_NORMAL)

I think we should check that the mode is ADD, returning early if it isn't.

@@ +931,5 @@
> +    if (!demo->page || demo->mode == MODE_NORMAL)
> +        return FALSE;
> +
> +    if (event->button != 1)
> +        return FALSE;

We could merge this check in the previous if.

@@ +937,5 @@
> +    demo->start.x = event->x;
> +    demo->start.y = event->y;
> +    demo->stop = demo->start;
> +
> +    if (demo->mode == MODE_ADD) {

We don't need this check if we returning early when the mode is not ADD.

@@ +939,5 @@
> +    demo->stop = demo->start;
> +
> +    if (demo->mode == MODE_ADD) {
> +        pgd_annots_add_annotation (demo);
> +        pgd_annots_update_cursor (demo, GDK_LAST_CURSOR);

To use the same state transitions for all annotations we could finish the
operation on button release instead, and here we switch to DRAWING mode, even
if for text annotations it does nothing. On button release we always check that
the current state is DRAWING and finish the operation, restoring the mode to
NORMAL and the cursor to last cursor.

-- 
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/dda260b5/attachment-0001.html>