[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 #13 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 86867
  --> https://bugs.freedesktop.org/attachment.cgi?id=86867
glib: Prepare UI to add multiple annotations type in demo

Review of attachment 86867:
-----------------------------------------------------------------

::: glib/demo/annots.c
@@ +38,5 @@
>  
> +enum {
> +    ANNOTS_NONE,
> +    ANNOTS_TEXT
> +};

Why not using PopplerAnnotType?

@@ +52,4 @@
>      GtkWidget       *timer_label;
>  
>      gint             num_page;
> +    gint             type_selector;

type_selector sounds like the widget not the value. I would use selected_type
or new_annot_type. I don't see why we need to save the value if we are using a
modal dialgo to add annots.

@@ +670,5 @@
>  static void
> +pgd_annots_type_selector_changed (GtkComboBox   *widget,
> +                                  PgdAnnotsDemo *demo)
> +{
> +    demo->type_selector = gtk_combo_box_get_active (widget);

Same here, we can get the value directly from the selector right after the
dialog_run and before destroying the dialog.

@@ +766,4 @@
>      PopplerAnnot *annot;
>      PopplerRectangle rect;
>  
> +    g_return_if_fail (demo->type_selector != ANNOTS_NONE);

Do not use g_return macros here, either return early or use an assert.

@@ +973,5 @@
>  
>      gtk_box_pack_start (GTK_BOX (vbox), hbox, FALSE, TRUE, 0);
> +
> +    type_selector = gtk_combo_box_text_new ();
> +    gtk_combo_box_text_append_text (GTK_COMBO_BOX_TEXT (type_selector), "None");

I don't remember why we allowed to add unknown annots, but I think it doesn't
make sense

-- 
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/20131004/6b89a5c9/attachment.html>