D11343: T1513

[Fabian Kristof]

fkristof added inline comments.

INLINE COMMENTS

> TextLabel.cpp:80
>  
> +    //feri kod
> +    m_labelshape = 0;

Remove unnecessary comment.

> TextLabel.cpp:81
> +    //feri kod
> +    m_labelshape = 0;
> +

Initialize members in the initializer list of the class instead.

> TextLabel.cpp:508
>  */
> -void TextLabelPrivate::recalcShapeAndBoundingRect() {
> -	prepareGeometryChange();
> -
> -	QMatrix matrix;
> -	matrix.rotate(-rotationAngle);
> -	transformedBoundingRectangle = matrix.mapRect(boundingRectangle);
> -
> -	labelShape = QPainterPath();
> -	labelShape.addRect(boundingRectangle);
> -	labelShape = matrix.map(labelShape);
> -
> -	emit(q->changed());
> +void TextLabelPrivate::recalcShapeAndBoundingRect(const TextLabel::LabelShape lbShape) {
> +    prepareGeometryChange();

I don't think that the name "labelShape" would be too long to be necessary to shorten it.

> TextLabel.cpp:516
> +    labelShape = QPainterPath();
> +    //feri kod
> +

Remove unnecessary comment.

> TextLabel.cpp:518
> +
> +    if(lbShape == TextLabel::LabelShape::Rect)
> +        labelShape.addRect(boundingRectangle);

In this case a switch-case would be more handy in my opinion.

> TextLabel.cpp:523
> +    {
> +        double xs = boundingRectangle.x();
> +        double ys = boundingRectangle.y();

If you don't intend to modify these then you can make them const.

> TextLabel.cpp:527
> +        double h = boundingRectangle.height();
> +        labelShape.addEllipse(xs-0.1*w, ys-0.1*h, 1.2*w,  1.2*h);
> +    }

Check the coding style for these lines (spacing in the expressions).

> TextLabel.cpp:556
> +        labelShape.quadTo(xs, ys, xs+0.2*h, ys);
> +
> +    }

Remove empty line.

> TextLabel.cpp:560
> +    {
> +
> +        double xs = boundingRectangle.x();

Remove empty line.

> TextLabel.cpp:991
> +{
> +    m_labelshape = shape;
> +    d_ptr->recalcShapeAndBoundingRect(static_cast<TextLabel::LabelShape>(shape));

You should move m_labelShape (instead of m_labelshape) to TextLabelPrivate. That way you won't need any getter, you can just set it in TextLabelPrivate. You should also rename this function to recalcShapeAndBoundingRect(TextLabel::LabelShape) too.

> TextLabel.cpp:993
> +    d_ptr->recalcShapeAndBoundingRect(static_cast<TextLabel::LabelShape>(shape));
> +
> +}

Remove empty line.

> TextLabel.cpp:998
> +{
> +    return LabelShape(static_cast<TextLabel::LabelShape>(m_labelshape));
> +}

There's no need of a second cast, the static_cast does what you wanted. Anyway, this will be reduced to

  return m_labelShape;

if you'll change its type to TextLabel.

> TextLabel.cpp:1000
> +}
> +int TextLabel::getLabelShapeint()
> +{

Remove this function.

> TextLabel.h:56
>  
> -			QString text;
> -			bool teXUsed;
> -		};
> +        //feri kod
> +        enum LabelShape {

Remove unnecessary comment.

> TextLabel.h:58
> +        enum LabelShape {
> +            Rect=0,
> +            Ellipse,

Respect our coding style, it should be "Rect = 0"

> TextLabel.h:68
> +        };
> +        void chooseLabelsShape(const int);
> +        TextLabel::LabelShape getLabelShape();

Why do you use an int instead of the previously declared enum? Using the enum you've created becomes more clear what your parameter is about. (LabelShape)

> TextLabel.h:69
> +        void chooseLabelsShape(const int);
> +        TextLabel::LabelShape getLabelShape();
> +        int getLabelShapeint();

Getters should be const functions.

> TextLabel.h:70
> +        TextLabel::LabelShape getLabelShape();
> +        int getLabelShapeint();
> +        TextLabelPrivate * qp;

This is unnecessary, a single getter is enough for a member variable.

> TextLabel.h:71
> +        int getLabelShapeint();
> +        TextLabelPrivate * qp;
> +

Remove this declaration, can't see any use of it.

> TextLabel.h:139
>  
> +        //feri kod
> +        int m_labelshape;

Remove unnecessary comment.

> TextLabel.h:140
> +        //feri kod
> +        int m_labelshape;
> +

Why is this member an int? Its type should be TextLabel.

> TextLabelPrivate.h:63
>  		bool swapVisible(bool on);
> -		virtual void recalcShapeAndBoundingRect();
> +        virtual void recalcShapeAndBoundingRect(const TextLabel::LabelShape=TextLabel::LabelShape::Rect);
>  		void updatePosition();

Not providing a name when using a default argument is a bad practice, the other developers can't deduce what the default parameter means since it has no name. Also the recheck the coding style (space before/after =)

> labelwidget.ui:558
> +      <item row="14" column="0">
> +       <widget class="QLabel" name="label">
> +        <property name="text">

Don't leave the name of the QLabel default. It sould be "lLabel".

> LabelWidget.cpp:107
>  
> +    //feri kod
> +    ui.cbLabelsShape->addItem( i18n("Rectangle"));

Remove unnecessary comment.

> LabelWidget.cpp:169
>  
> +    //feri kod
> +    connect(ui.cbLabelsShape, SIGNAL(currentIndexChanged(int)), this, SLOT( LabelShapeIndexChanged(int)) );

Remove unnecessary comment.

> LabelWidget.cpp:793
>  
> +    //feri kod
> +    ui.cbLabelsShape->setCurrentIndex(m_label->getLabelShapeint());

Remove unnecessary comment.

> LabelWidget.cpp:794
> +    //feri kod
> +    ui.cbLabelsShape->setCurrentIndex(m_label->getLabelShapeint());
> +

This isn't the way you load the configuration. Check the other parameters how they are saved and then loaded here from the config file.

> LabelWidget.cpp:850
> +
> +//feri kod
> +void LabelWidget::LabelShapeIndexChanged(int NewShape)

Remove unnecessary comment.

> LabelWidget.cpp:856
> +
> +    for (auto* label : m_labelsList)
> +        label->chooseLabelsShape(NewShape);

Possible improvement: use const pointer (auto* const label : m_labelsList).

> LabelWidget.h:103
> +
> +    //feri kod
> +    void LabelShapeIndexChanged(int);

Remove unnecessary comment.

> LabelWidget.h:104
> +    //feri kod
> +    void LabelShapeIndexChanged(int);
> +

Respect out coding style, the name of this method should be "labelShapeIndexChanged". I think "labelShapeChanged" would do the job too.

REPOSITORY
  R262 LabPlot

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

To: ferenczkovacs, fkristof
Cc: asemke, #kde_edu, garvitkhatri, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180315/ca94ec7c/attachment-0001.html>