On Jul 31, 2013, at 2:25 AM, Tavmjong Bah wrote:
I am not really sure that using "Shiny C++" code is the way to go here. At some point, it would be nice to convert all the sp-xxx files to proper C++, removing the dependency on GObject. But having one class being done differently (via GMEMWRAP) doesn't seem to bring much benefit.
Jon, perhaps you can comment further. Sebastian's code can be seen at:
https://bazaar.launchpad.net/~h-e-6/inkscape/connector-wip/files
The specific files are:
https://bazaar.launchpad.net/~h-e-6/inkscape/connector-wip/view/head:/src/sp... https://bazaar.launchpad.net/~h-e-6/inkscape/connector-wip/view/head:/src/sp... https://bazaar.launchpad.net/~h-e-6/inkscape/connector-wip/view/head:/src/go...
Well, yes first off the use of 'new' on the SPPoint is the immediate problem.
This also looks like a place where the pimpl idiom (private implementation) can be quite helpful. This addresses the issue with 'new', and improves encapsulation. It also happens to fall in line with the major change in direction that was taken for GTK+ 3.x.
It moves the members and methods into a private helper class that exists only in the implementation .cpp file and not in the public header .h file.
First looking at sp-point.h, I see a minor bit of cleanup that is needed:
class SPPoint : public SPItem { sigc::signal<void, SPPoint*, Geom::Point> _moved_signal;
SVGLength _x, _y;
public: ~SPPoint(){} ... };
In general, we would want to start a c++ class declaration with a single 'public:' followed optionally by a single 'protected:' section and finally an optional 'private:' section. Also, for a destructor we very often need it to be virtual. Of course, with C-based GTK+ style GObjects we can't really have that... but the pimpl idiom will fix it for us.
The other major style issue is collapsing multiple variable declarations to a single line. This can lead to many subtle problems, and in the past was initially done to save space on VT100 terminals and paper printouts. We don't really need to accommodate those any more. However, a more important factor is that keeping declarations to their own lines makes for easier maintenance, better legibility, easier tracking in source control, etc.
So a first cut would change to: class SPPoint : public SPItem { public: ~SPPoint(){}
... private: sigc::signal<void, SPPoint*, Geom::Point> _moved_signal;
SVGLength _x; SVGLength _y; };
Then we want to get rid of any function bodies in the header, as that locks us into promising a certain implementation. We want to hide the details inside of the .cpp and not let anyone know what is going on: class SPPoint : public SPItem { public: ~SPPoint();
... private: sigc::signal<void, SPPoint*, Geom::Point> _moved_signal;
SVGLength _x; SVGLength _y; };
Now to apply the magic of the pimpl idiom:
class SPPointImpl;
class SPPoint : public SPItem { friend class SPPointImpl; public:
... private: SPPointImpl *_impl; };
Suddenly the private section only contains a single pointer... and that is a type that is trivially handled by a C based GObject init function. It also hides all details such as member variables, signals, etc. This latter encapsulation is following C++ OO principals and as mentioned before is in line with the changes to GKT+ itself.
The init can now be void sp_point_init(SPPoint *point) { _impl = new SPPointImpl(point); }
And individual public member functions can be delegated to the private implementation:
char* SPPoint::description() const { return _impl->description(); } (noting that returning char* is not a good API, but that is something existing that we are not addressing in this pass)
And we can always have a few options with the implementation hooking:
Geom::Point SPPoint::getPosition(void) const { /* FIXME throw this through i2doc_affine() ? */ return Geom::Point(_impl->_x.computed, _impl->_y.computed); }
or
Geom::Point SPPoint::getPosition(void) const { return _impl->getPosition(); }
Of course, using the latter will keep things consistent and more centralized.
By the way, the "SP" of "SPPoint" is something we want to avoid for any new classes. That stands for "SodiPodi" which was the project Inkscape was forked from years ago.
Oh, and as far as the wrapping from GMEMWRAP, using a pimpl class can replace that thusly:
class SPPointImpl { public: static release(SPPoint *point) { point->release(); } // or: // static release(SPPoint *point) { // delete point->_impl; // point->_impl = 0; //}
static Geom::OptRect bbox(SPPoint *point, Geom::Affine const &transform, SPItem::BBoxType type) { return point->bbox(); } ... };
static void sp_point_class_init(SPPointClass *klass) { SPObjectClass *sp_object_class = SP_OBJECT_CLASS(klass); SPItemClass *sp_item_class = SP_ITEM_CLASS(klass);
sp_object_class->release = SPPointImpl::release; sp_object_class->set = SPPointImpl::setAttr; sp_object_class->write = SPPointImpl::write; sp_item_class->bbox = SPPointImpl::bbox; sp_item_class->description = SPPointImpl::description;