<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#c2">Comment # 2</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=88270" name="attach_88270" title="glib: Add implementation of Square and Circle annotations">attachment 88270</a> <a href="attachment.cgi?id=88270&action=edit" title="glib: Add implementation of Square and Circle annotations">[details]</a> <a href='page.cgi?id=splinter.html&bug=70983&attachment=88270'>[review]</a> glib: Add implementation of Square and Circle annotations Review of <a href="attachment.cgi?id=88270" name="attach_88270" title="glib: Add implementation of Square and Circle annotations">attachment 88270</a> <a href="attachment.cgi?id=88270&action=edit" title="glib: Add implementation of Square and Circle annotations">[details]</a> <a href='page.cgi?id=splinter.html&bug=70983&attachment=88270'>[review]</a>: ----------------------------------------------------------------- ::: glib/poppler-annot.cc @@ +118,4 @@ > PopplerAnnotMarkupClass parent_class; > }; > > +struct _PopplerAnnotGeometry I think we should expose circle and square as different objects, the spec doesn't mention Geometry annots at all, it's an internal class used for the implementation. We could use a base class for them for the common API (everything except the constructors), but I'm not sure geometry is the best name, a polygon is also a geometric shape, but polygon annotations don't inherit from geometry annotations (same for lines, etc.). Since the API is quite simple and the common implementation is in the core in the end, maybe we can just duplicate the API for circle and square. What do you think? @@ +406,5 @@ > +{ > +} > + > +/** > +* poppler_annot_geometry_new_circle: This line is not correctly indented. @@ +418,5 @@ > +* Creates a new empty #PopplerAnnotGeometry circle. > +* > +* Return value: a newly #PopplerAnnotGeometry annotation > +* > +* Since: 0.25 0.26 @@ +433,5 @@ > + border = new AnnotBorderArray (); > + border->setWidth (1.5); > + > + annot = new AnnotGeometry (doc->doc, &pdf_rect, Annot::typeCircle); > + annot->setBorder (border); Why do we set a border by default? The BS entry is optional. @@ +449,5 @@ > + * poppler_page_add_annot() > + * > + * Return value: a newly #PopplerAnnotGeometry annotation > + * > + * Since: 0.25 0.26 @@ +464,5 @@ > + border = new AnnotBorderArray (); > + border->setWidth (1.5); > + > + annot = new AnnotGeometry (doc->doc, &pdf_rect, Annot::typeSquare); > + annot->setBorder (border); Same comments here about the border. @@ +1718,5 @@ > + * > + * Return value: a new allocated #PopplerColor with the color values of > + * @poppler_annot, or %NULL. It must be freed with g_free() when done. > + * > + * Since: 0.25 0.26 @@ +1752,5 @@ > + poppler_color->red = (guint16) (values[0] * 65535); > + poppler_color->green = (guint16) (values[1] * 65535); > + poppler_color->blue = (guint16) (values[2] * 65535); > + > + break; Do not copy paste all this code, factor it out in a helper method.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>