Bug 69978 - Make demo extensible for other type of annotations
Summary: Make demo extensible for other type of annotations
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: glib frontend (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 70981
  Show dependency treegraph
 
Reported: 2013-09-30 18:57 UTC by Germán Poo-Caamaño
Modified: 2013-11-17 11:31 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
glib: Fix rectangle calculation for new annotations in demo (1.09 KB, patch)
2013-09-30 18:57 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Expand short names for annotations used in demo (1.51 KB, patch)
2013-09-30 18:58 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Rearrange layout for annotations in demo (6.82 KB, patch)
2013-09-30 18:58 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Show widgets altogether in annotations demo (6.12 KB, patch)
2013-09-30 18:58 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Merge columns Type and Color in annotations demo (2.99 KB, patch)
2013-09-30 18:59 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Prepare UI to add multiple annotations type in demo (4.17 KB, patch)
2013-09-30 18:59 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Make the Remove annotation button prominent in demo (2.65 KB, patch)
2013-09-30 19:00 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Add annotations interactively in demo (12.19 KB, patch)
2013-09-30 19:00 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Expand short names for annotations used in demo (2.37 KB, patch)
2013-10-04 15:40 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Expand short names for annotations used in demo (3.03 KB, patch)
2013-10-04 20:37 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Make the Remove annotation button prominent in demo (2.24 KB, patch)
2013-10-04 20:39 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib: Add annotations interactively in demo (15.30 KB, patch)
2013-10-04 20:41 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib-demo: Make the Remove annotation button prominent in demo (3.02 KB, patch)
2013-10-26 23:57 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib-demo: Add annotations interactively in demo (16.33 KB, patch)
2013-10-26 23:59 UTC, Germán Poo-Caamaño
Details | Splinter Review
glib-demo: Add annotations interactively (15.50 KB, patch)
2013-10-27 20:38 UTC, Germán Poo-Caamaño
Details | Splinter Review

Description Germán Poo-Caamaño 2013-09-30 18:57:27 UTC
Created attachment 86862 [details] [review]
glib: Fix rectangle calculation for new annotations in demo

The glib demo for annotations is tied to add only text annotations and it makes tedious to add and test newer ones.

The following set of patches tries to improve the demo for annotations. They include:
- Small fixes.
- Layout re-arrange.
- Automatically render and update list of annotations.
- Add/Remove annotations interactively instead of an extra dialog.

You can find them in a branch at https://github.com/gpoo/poppler/tree/annot-demo-ui

FWIW, the demo code for annotations has a mix tabs and 4 spaces.  Given the later were more common, I used those.
Comment 1 Germán Poo-Caamaño 2013-09-30 18:58:03 UTC
Created attachment 86863 [details] [review]
glib: Expand short names for annotations used in demo
Comment 2 Germán Poo-Caamaño 2013-09-30 18:58:31 UTC
Created attachment 86864 [details] [review]
glib: Rearrange layout for annotations in demo
Comment 3 Germán Poo-Caamaño 2013-09-30 18:58:48 UTC
Created attachment 86865 [details] [review]
glib: Show widgets altogether in annotations demo
Comment 4 Germán Poo-Caamaño 2013-09-30 18:59:10 UTC
Created attachment 86866 [details] [review]
glib: Merge columns Type and Color in annotations demo
Comment 5 Germán Poo-Caamaño 2013-09-30 18:59:41 UTC
Created attachment 86867 [details] [review]
glib: Prepare UI to add multiple annotations type in demo
Comment 6 Germán Poo-Caamaño 2013-09-30 19:00:01 UTC
Created attachment 86868 [details] [review]
glib: Make the Remove annotation button prominent in demo
Comment 7 Germán Poo-Caamaño 2013-09-30 19:00:20 UTC
Created attachment 86869 [details] [review]
glib: Add annotations interactively in demo
Comment 8 Carlos Garcia Campos 2013-10-04 10:29:48 UTC
Comment on attachment 86862 [details] [review]
glib: Fix rectangle calculation for new annotations in demo

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

Good catch, pushed. Thanks
Comment 9 Carlos Garcia Campos 2013-10-04 10:31:09 UTC
Comment on attachment 86863 [details] [review]
glib: Expand short names for annotations used in demo

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

Why? we are still using annots in other places of the demo UI, I'm not against the change but we should be consistent
Comment 10 Carlos Garcia Campos 2013-10-04 10:51:55 UTC
Comment on attachment 86864 [details] [review]
glib: Rearrange layout for annotations in demo

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

Pushed after fixing the list of annots when a new annot is added.

::: glib/demo/annots.c
@@ +839,5 @@
>      g_object_unref (page);
>  
>      gtk_widget_destroy (dialog);
> +
> +    pgd_annots_viewer_queue_redraw (demo);

We should also update the list of annots in this case too
Comment 11 Carlos Garcia Campos 2013-10-04 10:53:14 UTC
Comment on attachment 86865 [details] [review]
glib: Show widgets altogether in annotations demo

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

I'm not a fan of show_all, I prefer to show every widget after being added to a container.
Comment 12 Carlos Garcia Campos 2013-10-04 10:56:47 UTC
Comment on attachment 86866 [details] [review]
glib: Merge columns Type and Color in annotations demo

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

Yes, it looks much better, thanks. Pushed to git master
Comment 13 Carlos Garcia Campos 2013-10-04 11:08:30 UTC
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.

@@ +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
Comment 14 Germán Poo-Caamaño 2013-10-04 15:15:06 UTC
(In reply to comment #9)
> Comment on attachment 86863 [details] [review] [review]
> glib: Expand short names for annotations used in demo
> 
> Review of attachment 86863 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Why? we are still using annots in other places of the demo UI, I'm not
> against the change but we should be consistent

These are the strings shown in the UI of glib-demo.  Likely, I should
have also changed in the button, but given I am removing it in another
patch...
Comment 15 Germán Poo-Caamaño 2013-10-04 15:26:41 UTC
(In reply to comment #13)
> Comment on attachment 86867 [details] [review] [review]
> glib: Prepare UI to add multiple annotations type in demo
> 
> Review of attachment 86867 [details] [review] [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
Comment 16 Germán Poo-Caamaño 2013-10-04 15:40:45 UTC
Created attachment 87129 [details] [review]
glib: Expand short names for annotations used in demo

Updated the patch making the string changes consistent.
Comment 17 Germán Poo-Caamaño 2013-10-04 16:04:33 UTC
(In reply to comment #13)
> Comment on attachment 86867 [details] [review] [review]
> glib: Prepare UI to add multiple annotations type in demo
> 
> Review of attachment 86867 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: glib/demo/annots.c
> @@ +38,5 @@
> >  
> > +enum {
> > +    ANNOTS_NONE,
> > +    ANNOTS_TEXT
> > +};
> 
> Why not using PopplerAnnotType?

I missed this one comment.

Because:
- I needed a way to avoid any interaction in the drawing area
- To use the same value returned by the combobox for selecting a
  new annotation without the hassle of converting the enum to
  strings and viceversa (when using gtk_combo_box_text_append)

I can change it or use gtkcombobox and a model.
Comment 18 Germán Poo-Caamaño 2013-10-04 20:37:13 UTC
Created attachment 87139 [details] [review]
glib: Expand short names for annotations used in demo

I think now I expanded all the UI strings.
Comment 19 Germán Poo-Caamaño 2013-10-04 20:39:14 UTC
Created attachment 87141 [details] [review]
glib: Make the Remove annotation button prominent in demo

Patch rebased

I reordered the patches to squash the ones related to
remove the modal dialog and add annotations interactively.
Comment 20 Germán Poo-Caamaño 2013-10-04 20:41:16 UTC
Created attachment 87142 [details] [review]
glib: Add annotations interactively in demo

Squashed patches and address the comments in the review.

This patch:
* Prepare UI to add multiple annotations type in demo
* Remove dialog and button add annotations.
* Allows to add annotations interactively

At this moment there is only one annotation type, but
having this code structure, adding new annotations should
be straightforward.
Comment 21 Carlos Garcia Campos 2013-10-05 07:54:07 UTC
Comment on attachment 87141 [details] [review]
glib: Make the Remove annotation button prominent in demo

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

::: glib/demo/annots.c
@@ +959,4 @@
>      gtk_widget_show (label);
>      g_free (str);
>  
> +    button = gtk_button_new_from_stock (GTK_STOCK_REMOVE);

If we move this close to the Add Annotation button, I would label this one as Remove Annotation or Delete Annotation. Also note that the button was only present in the annotation properties section, so it was obvious which annotation was going to be removed. I would disable the button when there no annotation selected.
Comment 22 Carlos Garcia Campos 2013-10-05 08:02:25 UTC
Comment on attachment 87139 [details] [review]
glib: Expand short names for annotations used in demo

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

Ok, pushed.
Comment 23 Carlos Garcia Campos 2013-10-05 08:06:38 UTC
Comment on attachment 87142 [details] [review]
glib: Add annotations interactively in demo

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

I like the removal of the dialog to add an annotation, but it's a bit weird that while you change the combo to select and annotation, every click on the view adds a new one. I prefer to keep a combo with only the annots types and a button to add annot, similar to render demo. Also I would change the cursor to make it obvious that you are in adding an annot mode. Note also that the original idea of using a dialog was to add more annots properties when adding a new one, allowing you to select the color, or the icon in case of text annots, etc. I guess we can make annots properties editable instead, so that you can change the props multiple times after the annot has been created.
Comment 24 Germán Poo-Caamaño 2013-10-26 23:57:51 UTC
Created attachment 88159 [details] [review]
glib-demo: Make the Remove annotation button prominent in demo

Addressed comment regarding to sensitivity of the button only when there is an annotation selected.

I did not rename from "Remove" to "Remove annotation" because:
  1. The UI is already about annotations
  2. After adding "Add annotation" it could become too wide for
     no major gain.
Comment 25 Germán Poo-Caamaño 2013-10-26 23:59:34 UTC
Created attachment 88160 [details] [review]
glib-demo: Add annotations interactively in demo

Addressed comments.

* Add annotation is done after pressing a button, only once.
* Cursor is changed when adding the annotation.
Comment 26 Germán Poo-Caamaño 2013-10-27 20:38:02 UTC
Created attachment 88199 [details] [review]
glib-demo: Add annotations interactively

Updated patch, simplified and more extensible.

With respect to previous version of the patch, this one adds a mode:
Normal, Add, Edit, Drawing.

Different types of annotation have different interactions.  Add a
comment just require the user click on the page.  Adding a line
requires the user draw the line, etc.

I also added the Edit mode to implement later moving or editing the
annotation (e.g. when the mouse is over an annotation). Low
priority, but might be desirable in some cases.
Comment 27 Carlos Garcia Campos 2013-11-05 08:38:41 UTC
Comment on attachment 88159 [details] [review]
glib-demo: Make the Remove annotation button prominent in demo

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

Pushed to git master, thanks!
Comment 28 Carlos Garcia Campos 2013-11-17 11:25:10 UTC
Comment on attachment 88199 [details] [review]
glib-demo: Add annotations interactively

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

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.
Comment 29 Carlos Garcia Campos 2013-11-17 11:26:41 UTC
Comment on attachment 88199 [details] [review]
glib-demo: Add annotations interactively

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

::: glib/demo/annots.c
@@ +825,5 @@
> +    rect.x2 = demo->stop.x;
> +    rect.y2 = height - demo->stop.y;
> +    switch (demo->annot_type) {
> +        case POPPLER_ANNOT_TEXT:
> +            annot = poppler_annot_text_new (demo->doc, &rect);

The annotation is leaked here. When it's added to the model, the model takes a reference, and this one is never released, so we should call g_object_unref after add_annot_to_model
Comment 30 Carlos Garcia Campos 2013-11-17 11:30:41 UTC
Comment on attachment 88199 [details] [review]
glib-demo: Add annotations interactively

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

Ok, I've updated the patch addressing my comments, and pushed it to git master.


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.