<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - Make demo extensible for other type of annotations" href="https://bugs.freedesktop.org/show_bug.cgi?id=69978#c28">Comment # 28</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - Make demo extensible for other type of annotations" href="https://bugs.freedesktop.org/show_bug.cgi?id=69978">bug 69978</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=88199" name="attach_88199" title="glib-demo: Add annotations interactively">attachment 88199</a> <a href="attachment.cgi?id=88199&action=edit" title="glib-demo: Add annotations interactively">[details]</a> <a href='page.cgi?id=splinter.html&bug=69978&attachment=88199'>[review]</a> glib-demo: Add annotations interactively Review of <a href="attachment.cgi?id=88199" name="attach_88199" title="glib-demo: Add annotations interactively">attachment 88199</a> <a href="attachment.cgi?id=88199&action=edit" title="glib-demo: Add annotations interactively">[details]</a> <a href='page.cgi?id=splinter.html&bug=69978&attachment=88199'>[review]</a>: ----------------------------------------------------------------- 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>