<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - glib-demo: Add support for simple line annotations" href="https://bugs.freedesktop.org/show_bug.cgi?id=70981#c10">Comment # 10</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - glib-demo: Add support for simple line annotations" href="https://bugs.freedesktop.org/show_bug.cgi?id=70981">bug 70981</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=89309" name="attach_89309" title="glib-demo: Add simple demo to add line annotations">attachment 89309</a> <a href="attachment.cgi?id=89309&action=edit" title="glib-demo: Add simple demo to add line annotations">[details]</a> <a href='page.cgi?id=splinter.html&bug=70981&attachment=89309'>[review]</a> glib-demo: Add simple demo to add line annotations Review of <a href="attachment.cgi?id=89309" name="attach_89309" title="glib-demo: Add simple demo to add line annotations">attachment 89309</a> <a href="attachment.cgi?id=89309&action=edit" title="glib-demo: Add simple demo to add line annotations">[details]</a> <a href='page.cgi?id=splinter.html&bug=70981&attachment=89309'>[review]</a>: ----------------------------------------------------------------- ::: glib/demo/annots.c @@ +68,4 @@ > ModeType mode; > > cairo_surface_t *surface; > + PopplerColor *annot_color; This is not specific to line annots. I think you could split the patch and add first support for setting the color and then support for lines. @@ +86,5 @@ > > + if (demo->annotations_idle > 0) { > + g_source_remove (demo->annotations_idle); > + demo->annotations_idle = 0; > + } This is not correctly indented. @@ +368,5 @@ > break; > + case POPPLER_ANNOT_LINE: > + demo->mode = MODE_DRAWING; > + pgd_annots_update_cursor (demo, GDK_TCROSS); > + break; Now that all annots transition to the same states we don't need any change in start_add_annot @@ +461,5 @@ > +pgd_annot_color_changed (GtkButton *button, > + GParamSpec *pspec, > + PgdAnnotsDemo *demo) > +{ > + GdkRGBA color; Extra spaces between GdkRGBA and color. @@ +476,5 @@ > + > +static void > +pgd_annot_view_set_annot_line (PgdAnnotsDemo *demo, > + GtkWidget *table, > + PopplerAnnot *annot, This should receive a PopplerAnnotLine @@ +483,5 @@ > + PopplerColor *poppler_color; > + GdkPixbuf *pixbuf; > + GtkWidget *image; > + > + if ((poppler_color = poppler_annot_get_color (annot))) { Why does this depend on having a color? @@ +484,5 @@ > + GdkPixbuf *pixbuf; > + GtkWidget *image; > + > + if ((poppler_color = poppler_annot_get_color (annot))) { > + demo->active_annot = annot; I think this should be generic and done by pgd_annot_view_set_annot(). @@ +488,5 @@ > + demo->active_annot = annot; > + > + pixbuf = pgd_pixbuf_new_for_color (poppler_color); > + image = gtk_image_new (); > + gtk_image_set_from_pixbuf (GTK_IMAGE (image), pixbuf); You can use gtk_image_set_from_pixbuf() directly here instead of new + set_pixbuf @@ +491,5 @@ > + image = gtk_image_new (); > + gtk_image_set_from_pixbuf (GTK_IMAGE (image), pixbuf); > + g_object_unref (pixbuf); > + > + pgd_table_add_property_with_custom_widget (GTK_GRID (table), "Color:", image, row); This is very confusing, because it's called set_annot_line, but the only property set is color, which is common to all annotations. The color is already shown for all annots in the tree view, why do we want to show it again here and only for lines? @@ +696,1 @@ > pgd_annots_add_annot_to_model (PgdAnnotsDemo *demo, Instead of duplicating the iter here to return it, I think we could add a boolean argument, gboolean selected, for example, and when TRUE we set the cursor after inserting the annot in the model. @@ +906,5 @@ > + demo->active_annot = annot; > + > + poppler_annot_set_color (annot, demo->annot_color); > + poppler_page_add_annot (demo->page, annot); > + iter = pgd_annots_add_annot_to_model (demo, annot, rect); Here you would pass selected = TRUE @@ +1025,5 @@ > + break; > + case MODE_DRAWING: > + pgd_annots_add_annotation (demo); > + default: > + break; With the current code you don't need to change button_press either. @@ +1036,5 @@ > +pgd_annots_drawing_area_motion_notify (GtkWidget *area, > + GdkEventMotion *event, > + PgdAnnotsDemo *demo) > +{ > + if (!demo->page || demo->mode == MODE_NORMAL || demo->mode == MODE_ADD) You could return early when mode != DRAWING here. @@ +1039,5 @@ > +{ > + if (!demo->page || demo->mode == MODE_NORMAL || demo->mode == MODE_ADD) > + return FALSE; > + > + if (demo->start.x != -1) { You could move this to the previous if to return early when demo->start.x == -1 @@ +1043,5 @@ > + if (demo->start.x != -1) { > + demo->stop.x = event->x; > + demo->stop.y = event->y; > + > + if (demo->mode == MODE_DRAWING) { You don't need this check, at this point we should always be in drawing mode. What you should check, though, is the annotation type, because the code below is not common to all annots when in drawing mode, but specific to line annotations. @@ +1054,5 @@ > + /* Keep the drawing within the page */ > + demo->stop.x = MAX (demo->stop.x, 0); > + demo->stop.y = MAX (demo->stop.y, 0); > + demo->stop.x = MIN (demo->stop.x, width); > + demo->stop.y = MIN (demo->stop.y, height); I think you could use CLAMP directly when setting the values above. demo->stop.x = CLAMP(event->x, 0, width); demo->stop.y = CLAMP(event->y, 0, height); I thinks this simplifies the code a bit. @@ +1069,5 @@ > + pgd_annot_view_set_annot (demo, demo->active_annot); > + if (demo->annotations_idle == 0) { > + demo->annotations_idle = > + g_idle_add ((GSourceFunc)pgd_annots_viewer_redraw, > + demo); I think you can reuse pgd_annots_viewer_queue_redraw(), by modifying it to return early if there's a draw already scheduled and initializing annotations_idle when it schedules a new redraw. I think it's a good idea for all other cases where we are using ugd_annots_viewer_queue_redraw() in any case. @@ +1098,5 @@ > + > + if (demo->annotations_idle > 0) { > + g_source_remove (demo->annotations_idle); > + demo->annotations_idle = 0; > + } With the current code you don't need to change anything in button_release either</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>