kdeeduplot API review (was Re: Moving kdeeduplot to kdelibs)

[Simon Hausmann]

On Wednesday 10 January 2007 12:31, Cyrille Berger wrote:
[...]
> I think the API dox speeks better for itself that what I can do myself:
> http://cyrille.diwi.org/tmp/krita/kplot/classKPlotWidget.html
> (I didn't find a link to a libkdeedu API website, so I have put the
> relevant files on the link above).

Thanks, that makes it indeed easier to look at.

I've just gone through the API and I have some recommendations. Feel free to 
ignore them completely or consider them as constructive criticism :)

* KPlotWidget(QWidget *parent, double x1, double x2, ...)

	In QObject subclasses the parent object/parent always comes last, so I
	recommend moving that to the end.

* virtual x(), x2(), y(), y2(), dataWidth() and dataHeight() getters

	I suggest to replace them with a non-virtual function that simply returns the
	the rectangle they operate on. I don't believe it is actually possible to
	re-implement those virtual functions because the KPlotWidget implementation
	does not seem to use them. Besides that is is not common to have property
	getters virtual, especially if the setter is virtual, too. Do you know of any
	code re-implementing the current virtual setter or getter?

	So I think simply two rectangles as regular properties (non-virtual
	setter/getter), one for your primary and one for your secondary limit would
	be an easier solution. It gives you the same flexibility, the same speed but
	you have less API that I think is easier to understand.

* virtual [left/right/top/bottom]Padding getters and setters (both virtual)

	Most of the arguments from the previous point apply here. I suggest not to
	make any of them virtual and of course make the getters const. Perhaps it
	would be nice to also add 'contents' as suffix, so make it clearer that these
	properties specify the padding between the content and the axes.

* void setAntialias(bool b)

	It might be easier and more flexible to just have a renderhints property,
	something along the lines of

	QPainter::RenderHints renderHints() const
	void setRenderHints(QPainter::RenderHints);

	For example in Qt 4.3 we've added another render hint called FastAntialiasing
	that when painting on OpenGL allows switching between a high-quality
	anti-aliasing method and a faster multi-sampling based approach. With a
	RenderHints based property the users of your class would be able to utilize
	these without the need to adapt the KPlotWidget API.

* int objectCount() const
  void replaceObject(int i, KPlotObject *o)
  void addObject(KPlotObject *o)
  void clearObjectList()
  KPlotObject *object(int i) (missing const , btw)

	I suggest to replace objectCount() and object(int i) and clearObjectList()
	with

	void setPlotObjects(const QList<KPlotObject *> &objects);
	QList<KPlotObject *> plotObjects() const;

	I also suggest to add the suffix/prefix 'plot' to those functions to increase
	the readability of the calling code. For example:

		view->addObject(obj);
	vs.
		view->addPlotObject(obj);

	The add function I think is a very good idea because it simplifies the common
	use-case.

* QPointF toScreen(const QPointF &p) const

	A rule for naming a function that often applies is to look at the first few
	words of the documentation of a function. And indeed there we can find the
	verb that I think is missing in that function name, hence my suggestion:

	QPointF mapToScreen(const QPointF &) const;

	Having map in the name is consistent to Qt, which uses this naming pattern in
	a lot of places (QWidget mapToGlobal, QGraphicsView mapToScene, QScreen's
	mapToDevice, etc.).

	I'm however not sure about the term screen for this function because the
	coordinate it returns are not actual screen coordinates. You could either
	call it mapToWidget or try to work around the problem by calling it
	mapFromContents.

* pixRect / setPixRect

	I suggest to attempt to remove this function from the public API alltogether.
	The name is not very clear and it seems the paintEvent() already calls (and
	therefore sets up the rectangle in question) automatically.

In fact I suggest to remove all the currently protected functions from the 
public API. The same applies to the protected variables. Having variables 
like double px[100] protected means that you will never be able to change 
then anymore. I suggest to move all the variables and protected functions 
into the d-pointer, which is required for kdelibs inclusion anyway.

On a more general note I think it might be interesting to make KPlotWidget 
itself not a widget anymore (and give it a different name at the same time) 
but with a public draw/drawContents functions that takes a painter. The goal 
would be to be able to easily embed your plotting functionality in graphical 
environments like koffice's flake canvas or QGraphicsView.

KPlotPoint

	There's a typo with the stBarWidth setter that is missing an 'e' :)

KPlotObject:

* enum PlotType { POINTS, LINES, BARS, UNKNOWN_TYPE }

	For consistency I suggest to use camelcase style instead of the full
	uppercase naming. Since this is actually a flag I suggest to turn this into
	a full typesafe QFlag (PlotType for the individual enum and PlotTypes for the
	flags type).

* enum PStyle { NOPOINTS, CIRCLE, LETTER, ... }

	The same applies here and I also suggest to rename PStyle to something more
	descriptive. The documentation again gives a hint, perhaps PointStyle would
	be more readable.

* bool showPoints() const;
  bool showLines() const;
  bool showBars() const;

	The name suggest that these functions would actually cause points, lines or
	bars to be shown, but they are actually getters. But it might be just easier
	to have one getter that returns the type that is passed to the constructor.
	That means less API, consistency (you can retrieve again what you pass to the
	constructor) and you avoid the naming problem mentioned in the first
	sentence.

* unsigned int pointStyle() const
  void setPointStyle(PStyle p)

	I suggest to make the getter return a type-safe enum instead of an unsigned
	int.

With the functions to modify the points of the KPlotObject (count, point(int 
idx), removePoint) I suggest to apply the same pattern as for KPlotWidget's 
object functions.

KPlotAxis:

* bool showTickLabels()

	This is the same naming problem as with showPoints() in KPlotObject.
	areTickLabelsShown() would already be a big improvement I think.

* void setTickLabelFormat()
  char tickTableFmt() const

	I think the getter should be called tickTableFormat().

Also I think tickTablePrec() should be called tickTablePrecision().

* QList<double> &majorTickMarks()
  QList<double> &minorTickMarks()

	I suggest to not return a reference but to turn this into a normal property
	with setter and getter, for consistency with Qt and KDE.


I hope you find these suggestions useful. It would also be great if others 
could give their comments on the API, too. The more eyes on it the better.


Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070113/2086f858/attachment.sig>