[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 #15 from Germán Poo-Caamaño <gpoo at gnome.org> ---
(In reply to comment #13)
> Comment on attachment 86867 [details] [review]
> glib: Prepare UI to add multiple annotations type in demo
> 
> Review of attachment 86867 [details] [review]:
> -----------------------------------------------------------------
> 
> ::: 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.

To make it extensible immediately.  I could squash this changes when
adding the support for Lines (or another annotation type), but I think
it is cleaner to have them in separated commits, as in:
https://github.com/gpoo/poppler/commit/b9a3c2a2501169b2f7dafd868b9302a228b0e8cd

[...]
> @@ +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

It does when the annotations are added interactively instead of a modal dialog
(in another patch submitted).  This allow to add a new annotation just by
clicking or selecting a rectangle in the drawing area.

I could have removed it and added it later squashed in the another patch,
but I followed the same rationale of having separated commits.

I also could have added a toggle button or something like that, but I thought
it
was overkill at this stage.  What do you thinl

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