D22200: Column observer

Alexander Semke noreply at phabricator.kde.org
Wed Jul 10 07:49:36 BST 2019


asemke added inline comments.

INLINE COMMENTS

> Project.cpp:202
> + * \brief Project::descriptionChanged
> + * When the column is created, it gets a random name and is eventually not connected to any curve.
> + * When changing the name it can match a curve and should than be connected to the curve.

move this comment inside the function definition. This is the comment for the handling of columns only and not for the whole function which does a bit more.

> Project.cpp:221
> +		// xColumnPath is set in setXColumn. Same for the yColumn.
> +		for (auto* curve : curves) {
> +			auto* analysisCurve = dynamic_cast<XYAnalysisCurve*>(curve);

in this for loop we call several set-functions which will produce a new entry on the undo/redo-stack. Do we want to have these entries if we change the name of a column? I'd say no. Le's work with curve->setUndoAware(false);
curve->set*Column(column);
curve->setUndoAware(true);

> Project.cpp:276
> +
> +	for (auto* curve : curves) {
> +		auto* analysisCurve = dynamic_cast<XYAnalysisCurve*>(curve);

same here. Let's use setUndoAware(false) prior to setting the columns.

> Project.h:37
>  class QString;
> +class ColumnObserver;
>  

can be removed.

> XYCurveDock.cpp:556
>  
> +	checkCurveAvailability(m_curve, cbXColumn, "xColumn");
> +	checkCurveAvailability(m_curve, cbYColumn, "yColumn");

wouldn't it be easier here to provide the column and the column path directly to this function like

  checkCurveAvailability(cbXColumn,  curve->xColumn(), curve->xColumnPath());

With this you'd avoid this cumbersome if-else logic in XYCurveDock::checkCurveAvailability().

REPOSITORY
  R262 LabPlot

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

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


More information about the kde-edu mailing list