D21575: Add Cursor

[Alexander Semke]

asemke added a comment.


  The class name TreeModel sounds very generic but this model will most probably only used for cursors only. Can we call it CursorTreeModel similar to how have AspectTreeModel already?
  
  The are several code style glitches. Please execute astyle with the options in admin/README.astyle.

INLINE COMMENTS

> AbstractAspect.cpp:339
> +AbstractAspect* AbstractAspect::parent(AspectType type) const {
> +
> +	AbstractAspect* parent = parentAspect();

remove empty line.

> ColumnPrivate.cpp:1289
>  					monotonic_increasing = 1;
> -				else if (value < prevValue)
> +				else if (value < prevValue || value != value) {// or if NAN
>  					monotonic_increasing = 0;

std::isnan(value)

> ColumnPrivate.cpp:1299
>  					monotonic_decreasing = 1;
> -				else if (value > prevValue)
> +				else if (value > prevValue || value != value) {
>  					monotonic_decreasing = 0;

std::isnan(value)

> ColumnPrivate.cpp:1301
>  					monotonic_decreasing = 0;
> +					if (monotonic_increasing == 0) // leaf loop if nor decreasing nor increasing
> +						break;

small typo - leave, not leaf.

> TreeModel.cpp:36
> +TreeItem::TreeItem(const QVector<QVariant> &data, TreeItem *parent) {
> +    parentItem = parent;
> +    itemData = data;

use constructor member initializer list.

> TreeModel.h:63
> +    QColor m_backgroundColor;
> +    TreeItem *parentItem;
> +};

TreeItem* parentItem{nullptr};

> TreeModel.h:74
> +public:
> +    TreeModel(const QStringList &headers,
> +              QObject *parent = 0);

TreeModel(const QStringList& headers, QObject* parent = nullptr);

> TreeModel.h:106
> +
> +    TreeItem *rootItem;
> +};

TreeItem* rootItem{nullptr};

> Worksheet.cpp:427
> +WorksheetElement *Worksheet::getPlot(int index){
> +
> +	QVector<CartesianPlot*> cartesianPlots = children<CartesianPlot>();

remove empty line.

> Worksheet.cpp:429
> +	QVector<CartesianPlot*> cartesianPlots = children<CartesianPlot>();
> +	if(cartesianPlots.length()-1 >= index){
> +		return cartesianPlots[index];

no brackets for one-liners.

> Worksheet.cpp:976
> +
> +void Worksheet::curveDataChanged(const XYCurve* curve) {
> +	CartesianPlot* plot = dynamic_cast<CartesianPlot*>(QObject::sender());

In curveDataChanged() and in the following functions there is a lot of logic handling the tree model. Can me move this code into TreeModel and have in Worksheet only some abstract interface of the model hiding the technical details?

> Worksheet.cpp:1226
>  WorksheetPrivate::WorksheetPrivate(Worksheet* owner) : q(owner), m_scene(new QGraphicsScene()) {
> +	QStringList headers = {"Plot/Curve", "V1", "V2", "V2-V1"};
> +    cursorData = new TreeModel(headers, nullptr);

add i18n for "Plot/Curve".

> CartesianCoordinateSystem.cpp:250
> +						x = pageRect.x();
> +					if (x > pageRect.x()+pageRect.width())
> +						x = pageRect.x()+pageRect.width();

add spaces between the operators here.

> CartesianPlot.cpp:1069
>  
> +STD_SETTER_CMD_IMPL_F_S(CartesianPlot, SetCursorPen, QPen, cursorPen, retransformScales)
> +void CartesianPlot::setCursorPen(const QPen &pen) {

do we really need to retransform the scale if we set the pen for the cursor?

> CartesianPlot.cpp:1359
>  
> +int CartesianPlot::signalCount(){
> +	return children<XYCurve>().length();

"signal" is not the terminology used in the code. Let's stick to "curve".

> CartesianPlot.cpp:3050
> +    writer->writeStartElement( "cursor" );
> +    writer->writeAttribute( "width", QString::number(d->cursorPen.width()));
> +    writer->writeAttribute( "style", QString::number(d->cursorPen.style()));

use WRITE_QPEN macro.

> ProjectExplorer.cpp:629
> +	for (const QModelIndex& index : items) {
> +		aspect = static_cast<AbstractAspect*>(index.internalPointer());
> +		selectedAspects<<aspect;

use directly selectedAspects << static_cast<AbstractAspect*>(index.internalPointer());, no need for an extra variable and this assignment in the loop.

> MainWin.cpp:179
> +	if (!cursorDock) {
> +		cursorDock = new QDockWidget("Cursor", this);
> +		cursorWidget = new CursorDock(cursorDock);

i18n("Cursor")

> MainWin.cpp:190
> +
> +		cursorDock->setWindowTitle("Cursor");
> +	}

isn't this obsolete since you set the title in the constructor already?

> CartesianPlotDock.h:181
> +    // Cursor
> +    void plotCursorPenChanged(QPen pen);
> +

void plotCursorPenChanged(QPen);

REPOSITORY
  R262 LabPlot

REVISION DETAIL
  https://phabricator.kde.org/D21575

To: Murmele, asemke
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190608/2cf4eec4/attachment-0001.html>