This is an attempt to solve TASK T1513
https://phabricator.kde.org/T1513
I added several options to choose the shape of the label from.
Video about the functionality:
https://www.youtube.com/watch?v=15pwzEUDoHo&feature=youtu.be
fkristof | |
ferenczkovacs |
This is an attempt to solve TASK T1513
https://phabricator.kde.org/T1513
I added several options to choose the shape of the label from.
Video about the functionality:
https://www.youtube.com/watch?v=15pwzEUDoHo&feature=youtu.be
The testing was done manually as you can see in the pictures.
Lint Skipped |
Unit Tests Skipped |
src/backend/worksheet/TextLabel.cpp | ||
---|---|---|
80 | Remove unnecessary comment. | |
81 | Initialize members in the initializer list of the class instead. | |
508 | I don't think that the name "labelShape" would be too long to be necessary to shorten it. | |
516 | Remove unnecessary comment. | |
518 | In this case a switch-case would be more handy in my opinion. | |
523 | If you don't intend to modify these then you can make them const. | |
527 | Check the coding style for these lines (spacing in the expressions). | |
556 | Remove empty line. | |
560 | Remove empty line. | |
991 | 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. | |
993 | Remove empty line. | |
998 | 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. | |
1000 | Remove this function. | |
src/backend/worksheet/TextLabel.h | ||
56 | Remove unnecessary comment. | |
58 | Respect our coding style, it should be "Rect = 0" | |
68 | 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) | |
69 | Getters should be const functions. | |
70 | This is unnecessary, a single getter is enough for a member variable. | |
71 | Remove this declaration, can't see any use of it. | |
139 | Remove unnecessary comment. | |
140 | Why is this member an int? Its type should be TextLabel. | |
src/backend/worksheet/TextLabelPrivate.h | ||
63 | 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 =) | |
src/kdefrontend/ui/labelwidget.ui | ||
558 | Don't leave the name of the QLabel default. It sould be "lLabel". | |
src/kdefrontend/widgets/LabelWidget.cpp | ||
107 | Remove unnecessary comment. | |
169 | Remove unnecessary comment. | |
793 | Remove unnecessary comment. | |
794 | 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. | |
850 | Remove unnecessary comment. | |
856 | Possible improvement: use const pointer (auto* const label : m_labelsList). | |
src/kdefrontend/widgets/LabelWidget.h | ||
103 | Remove unnecessary comment. | |
104 | Respect out coding style, the name of this method should be "labelShapeIndexChanged". I think "labelShapeChanged" would do the job too. |
I made some new features regarding the label's border style, border color, and its fill color and pattern. There is also a preview feature available when one wants to choose the label's shape.
src/backend/worksheet/TextLabel.cpp | ||
---|---|---|
564 | Remove empty line. | |
src/backend/worksheet/TextLabel.h | ||
25 | Remove empty lines. | |
32 | This should be a const member function. | |
33 | There's no need of another getter, you have already one: TextLabel::LabelShape getLabelShape(); | |
40 | This should return Qt::BrushStyle because of Qt::BrushStyle patternStyle; | |
77 | Make this function const, it's a getter. | |
78 | Remove this getter, you've got already TextLabel::LabelShape getLabelShape(); | |
79 | Make this function const, it's a getter. | |
81 | I think this should be QColor backgroundColor; or QColor fillColor; | |
83 | This should be: void chooseLabelBackgroundColor(const QColor&); or void chooseLabelFillColor(const QColor&); (pass const reference to avoid copy constrution) | |
85 | Here too, pass const reference const QColor& | |
86 | This should be void choosePatternStyle(const Qt::BrushStyle); because Qt::BrushStyle patternStyle; | |
87 | This should return Qt::BrushStyle because Qt::BrushStyle patternStyle; | |
157 | Move these members to TextLabelPrivate. | |
src/kdefrontend/ui/dockwidgets/cartesianplotdock.ui | ||
20 | Revert this change. | |
1184 | Revert this change too, it shouldn't be removed. | |
src/kdefrontend/ui/labelwidget.ui | ||
308 | The name of this widget should be bBackgroundColor or bFillColor | |
633 | The name of this widget should be cbBackgroundPattern or cbFillPattern | |
src/kdefrontend/widgets/LabelWidget.cpp | ||
129 | These combobox items/styles/patterns should all start with an upper-case letter. | |
200 | Remove extra space after SLOT( | |
894 | The parameter name should start with a lower-case letter. | |
899 | Use const pointer: for (auto* const label | |
911 | Const pointer here too. | |
918 | No need to cast to QObject*, ui.cbLabelsShape inherits QObject too. | |
922 | Const pointer. | |
934 | This object can be const if you don't intend to change it in the future. | |
944 | No use of this else. | |
950 | Const pointer. | |
955 | backcolor can be const. | |
959 | There's no need of a cast, backcolor it's a QColor already. | |
961 | Const pointer. | |
965 | Neither here has any use to have this else here. | |
971 | Const pointer. | |
src/kdefrontend/widgets/LabelWidget.h | ||
11 | It should be void labelShapeChanged(int); | |
12 | It should be void labelShapePreview(int); | |
64 | What does this represent? The name isn't very self-describing. | |
103 | It should be void labelShapeIndexChanged(int); The same applies for the other functions you've introduced, they start with upper-case "L". |
I tried to solve the problems mentioned in the comments. Furthermore i uploaded a video about the features which i implemented:
https://www.youtube.com/watch?v=15pwzEUDoHo&feature=youtu.be
src/backend/worksheet/TextLabel.cpp | ||
---|---|---|
991 | I don't think so, because then i'll need a plus set of set-get functions | |
src/backend/worksheet/TextLabel.h | ||
78 | I'll rather use const int getLabelShapeint(); | |
157 | I'll pass if possible, i'll need unnecessarily an additional set/get function pair | |
src/kdefrontend/widgets/LabelWidget.cpp | ||
794 | can't manage to solve it | |
src/kdefrontend/widgets/LabelWidget.h | ||
64 | a copy of the temporary shape, i think it's quite clear |
src/backend/worksheet/TextLabel.cpp | ||
---|---|---|
991 | That's not a problem. The main idea I have here is that you use labelShape in TextLabelPrivate, and not in TextLabel, this implies that labelShape should be a member of TextLabelPrivate, not TextLabel. So I suggest you to make this change. The same applies to the other parameters you've added, like backgroundColor, borderStyle, etc, they all belong to TextLabelPrivate. TextLabel can access them through the d pointer, if needed you can write getters and setters for them. | |
src/backend/worksheet/TextLabel.h | ||
78 | Since labelShape has the type of TextLabel::LabelShape your getter should look like TextLabel::LabelShape getLabelShape() const; | |
src/kdefrontend/widgets/LabelWidget.h | ||
64 | I think m_previewTempLabelShape would be more self-describing in this case. |
Adjusted and completed version of this patch was uploaded in https://phabricator.kde.org/R262:2728e198e5b724bd7b43d6d441e938918968bd7f