D11343: T1513

[Fabian Kristof]

fkristof added inline comments.

INLINE COMMENTS

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

Remove empty line.

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

Make this function const, it's a getter.

> TextLabel.h:78
> +        TextLabel::LabelShape getLabelShape();
> +        int getLabelShapeint();
> +        int getLabelBorder();

Remove this getter, you've got already

  TextLabel::LabelShape getLabelShape();

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

Make this function const, it's a getter.

> TextLabel.h:81
> +        TextLabelPrivate * qp;
> +        QColor backColor;
> +        QColor borderColor;

I think this should be

  QColor backgroundColor;

or

  QColor fillColor;

> TextLabel.h:83
> +        QColor borderColor;
> +        void chooseLabelBackColor(const QColor);
> +        void chooseLabelBorder(const int);

This should be:

  void chooseLabelBackgroundColor(const QColor&);

or

  void chooseLabelFillColor(const QColor&);

(pass const reference to avoid copy constrution)

> TextLabel.h:85
> +        void chooseLabelBorder(const int);
> +        void chooseLabelBorderColor(const QColor);
> +        void choosePatternStyle(const int);

Here too, pass const reference

  const QColor&

> TextLabel.h:86
> +        void chooseLabelBorderColor(const QColor);
> +        void choosePatternStyle(const int);
> +        int getPatternStyle();

This should be

  void choosePatternStyle(const Qt::BrushStyle);

because

  Qt::BrushStyle patternStyle;

> TextLabel.h:87
> +        void choosePatternStyle(const int);
> +        int getPatternStyle();
> +

This should return

  Qt::BrushStyle

because

  Qt::BrushStyle patternStyle;

> TextLabel.h:159
> +
> +        int m_labelshape;
> +        int m_patternStyle;

Move these members to TextLabelPrivate.

> TextLabel.h:25


Remove empty lines.

> TextLabel.h:32


This should be a const member function.

> TextLabel.h:33


There's no need of another getter, you have already one:

  TextLabel::LabelShape getLabelShape();

> TextLabel.h:40
>  #include <QPen>
> +#include <QColor>
>  

This should return

  Qt::BrushStyle

because of

  Qt::BrushStyle patternStyle;

> cartesianplotdock.ui:20
>       <property name="currentIndex">
> -      <number>0</number>
> +      <number>3</number>
>       </property>

Revert this change.

> cartesianplotdock.ui:1184
>    </customwidget>
> -  <customwidget>
> -   <class>QLineEdit</class>

Revert this change too, it shouldn't be removed.

> labelwidget.ui:308
> +      <item row="17" column="3">
> +       <widget class="QPushButton" name="bBackColor">
> +        <property name="text">

The name of this widget should be

  bBackgroundColor

or

  bFillColor

> labelwidget.ui:633
> +      <item row="18" column="3">
> +       <widget class="QComboBox" name="cbBackPattern"/>
> +      </item>

The name of this widget should be

  cbBackgroundPattern

or

  cbFillPattern

> LabelWidget.cpp:129
> +
> +    ui.cbBorderStyle->addItem(i18n("no line"));
> +    ui.cbBorderStyle->addItem(i18n("solid line"));

These combobox items/styles/patterns should all start with an upper-case letter.

> LabelWidget.cpp:200
> +
> +    connect(ui.cbLabelsShape, SIGNAL(currentIndexChanged(int)), this, SLOT( LabelShapeIndexChanged(int)) );
> +    connect(ui.cbLabelsShape, SIGNAL(highlighted(int)), this, SLOT(LabelShapePreview(int)));

Remove extra space after

  SLOT(

> LabelWidget.cpp:894
> +
> +void LabelWidget::LabelShapeIndexChanged(int NewShape)
> +{

The parameter name should start with a lower-case letter.

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

Use const pointer:

  for (auto* const label

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

Const pointer here too.

> LabelWidget.cpp:918
> +{
> +    if (obj == (QObject*)ui.cbLabelsShape)
> +    {

No need to cast to QObject*, ui.cbLabelsShape inherits QObject too.

> LabelWidget.cpp:922
> +        {
> +            for (auto* label : m_labelsList)
> +                label->chooseLabelsShape(m_saveShape);

Const pointer.

> LabelWidget.cpp:934
> +{
> +    QColor backcolor = QColorDialog::getColor(Qt::transparent, this);
> +    if (backcolor.isValid())

This object can be const if you don't intend to change it in the future.

> LabelWidget.cpp:944
> +    }
> +    else return;
> +

No use of this else.

> LabelWidget.cpp:950
> +{
> +    for (auto* label : m_labelsList)
> +        label->chooseLabelBorder(border);

Const pointer.

> LabelWidget.cpp:955
> +{
> +    QColor backcolor = QColorDialog::getColor(Qt::black, this);
> +    if (backcolor.isValid())

backcolor can be const.

> LabelWidget.cpp:959
> +        QPalette p = palette();
> +        p.setColor(QPalette::Button, (const QColor)backcolor);
> +        ui.bBackColor->setPalette(p);

There's no need of a cast, backcolor it's a QColor already.

> LabelWidget.cpp:961
> +        ui.bBackColor->setPalette(p);
> +        for (auto* label : m_labelsList)
> +            label->chooseLabelBorderColor(backcolor);

Const pointer.

> LabelWidget.cpp:965
> +    }
> +    else return;
> +

Neither here has any use to have this else here.

> LabelWidget.cpp:971
> +{
> +    for (auto* label : m_labelsList)
> +        label->choosePatternStyle(pattern);

Const pointer.

> LabelWidget.h:11


It should be

  void labelShapeChanged(int);

> LabelWidget.h:12


It should be

  void labelShapePreview(int);

> LabelWidget.h:64
>  private:
> +    int m_saveShape;
>  	Ui::LabelWidget ui;

What does this represent? The name isn't very self-describing.

> LabelWidget.h:103
>  
> +    void LabelShapeIndexChanged(int);
> +    void LabelShapePreview(int);

It should be

  void labelShapeIndexChanged(int);

The same applies for the other functions you've introduced, they start with upper-case "L".

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/20180321/104e314d/attachment-0001.html>