<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - Add implementation of Square and Circle annotations" href="https://bugs.freedesktop.org/show_bug.cgi?id=70983#c10">Comment # 10</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - Add implementation of Square and Circle annotations" href="https://bugs.freedesktop.org/show_bug.cgi?id=70983">bug 70983</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=89941" name="attach_89941" title="glib: Add implementation of Square and Circle annotations">attachment 89941</a> <a href="attachment.cgi?id=89941&action=edit" title="glib: Add implementation of Square and Circle annotations">[details]</a> <a href='page.cgi?id=splinter.html&bug=70983&attachment=89941'>[review]</a> glib: Add implementation of Square and Circle annotations Review of <a href="attachment.cgi?id=89941" name="attach_89941" title="glib: Add implementation of Square and Circle annotations">attachment 89941</a> <a href="attachment.cgi?id=89941&action=edit" title="glib: Add implementation of Square and Circle annotations">[details]</a> <a href='page.cgi?id=splinter.html&bug=70983&attachment=89941'>[review]</a>: ----------------------------------------------------------------- Pushed the patch after addressing my comments below. Thank you very much. ::: glib/poppler-annot.cc @@ +425,5 @@ > + * > + * Creates a new Circle annotation that will be > + * located on @rect when added to a page. See > + * poppler_page_add_annot() > +* This is wrongly indented. @@ +426,5 @@ > + * Creates a new Circle annotation that will be > + * located on @rect when added to a page. See > + * poppler_page_add_annot() > +* > +* Creates a new empty #PopplerAnnotCircle circle. This line looks redundant @@ +428,5 @@ > + * poppler_page_add_annot() > +* > +* Creates a new empty #PopplerAnnotCircle circle. > +* > +* Return value: a newly #PopplerAnnotCircle annotation newly created @@ +470,5 @@ > + * Creates a new Square annotation that will be > + * located on @rect when added to a page. See > + * poppler_page_add_annot() > + * > + * Return value: a newly #PopplerAnnotSquare annotation newly created @@ +691,5 @@ > poppler_annot->annot->setFlags ((guint) flags); > } > > +static PopplerColor * > +_poppler_new_poppler_color (AnnotColor *color) We don't use the _ prefix for private static functions, but only for private functions declared in a header file. The name is a bit confusing, it could be called create_poppler_color_from_annot_color @@ +727,5 @@ > return poppler_color; > } > > +static AnnotColor * > +_poppler_new_annot_color (PopplerColor *poppler_color) And this create_annot_color_from_poppler_color @@ +729,5 @@ > > +static AnnotColor * > +_poppler_new_annot_color (PopplerColor *poppler_color) > +{ > + AnnotColor *color = NULL; We don't need the local variable if we return early when passed in pointer is NULL and then we can just return new AnnotColor() @@ +1635,5 @@ > +{ > + AnnotGeometry *annot; > + > + g_return_val_if_fail (POPPLER_IS_ANNOT_CIRCLE (poppler_annot) || > + POPPLER_IS_ANNOT_SQUARE (poppler_annot), NULL); Do not use g_return macros in private methods. Also these checks are already done by the callers. @@ +1649,5 @@ > +{ > + AnnotGeometry *annot; > + > + g_return_if_fail (POPPLER_IS_ANNOT_CIRCLE (poppler_annot) || > + POPPLER_IS_ANNOT_SQUARE (poppler_annot)); Ditto. @@ +1679,5 @@ > +} > + > +/** > + * poppler_annot_circle_set_interior_color: > + * @poppler_annot: a #PopplerAnnot a #PopplerAnnotCircle @@ +1717,5 @@ > +} > + > +/** > + * poppler_annot_square_set_interior_color: > + * @poppler_annot: a #PopplerAnnot a #PopplerAnnotSquare</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>