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 ↗ | (On Diff #29534) | Remove unnecessary comment. |
81 ↗ | (On Diff #29534) | Initialize members in the initializer list of the class instead. |
508 ↗ | (On Diff #29534) | I don't think that the name "labelShape" would be too long to be necessary to shorten it. |
516 ↗ | (On Diff #29534) | Remove unnecessary comment. |
518 ↗ | (On Diff #29534) | In this case a switch-case would be more handy in my opinion. |
523 ↗ | (On Diff #29534) | If you don't intend to modify these then you can make them const. |
527 ↗ | (On Diff #29534) | Check the coding style for these lines (spacing in the expressions). |
556 ↗ | (On Diff #29534) | Remove empty line. |
560 ↗ | (On Diff #29534) | Remove empty line. |
991 ↗ | (On Diff #29534) | 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 ↗ | (On Diff #29534) | Remove empty line. |
998 ↗ | (On Diff #29534) | 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 ↗ | (On Diff #29534) | Remove this function. |
src/backend/worksheet/TextLabel.h | ||
56 ↗ | (On Diff #29534) | Remove unnecessary comment. |
58 ↗ | (On Diff #29534) | Respect our coding style, it should be "Rect = 0" |
68 ↗ | (On Diff #29534) | 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 ↗ | (On Diff #29534) | Getters should be const functions. |
70 ↗ | (On Diff #29534) | This is unnecessary, a single getter is enough for a member variable. |
71 ↗ | (On Diff #29534) | Remove this declaration, can't see any use of it. |
139 ↗ | (On Diff #29534) | Remove unnecessary comment. |
140 ↗ | (On Diff #29534) | Why is this member an int? Its type should be TextLabel. |
src/backend/worksheet/TextLabelPrivate.h | ||
63 ↗ | (On Diff #29534) | 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 ↗ | (On Diff #29534) | Don't leave the name of the QLabel default. It sould be "lLabel". |
src/kdefrontend/widgets/LabelWidget.cpp | ||
107 ↗ | (On Diff #29534) | Remove unnecessary comment. |
169 ↗ | (On Diff #29534) | Remove unnecessary comment. |
793 ↗ | (On Diff #29534) | Remove unnecessary comment. |
794 ↗ | (On Diff #29534) | 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 ↗ | (On Diff #29534) | Remove unnecessary comment. |
856 ↗ | (On Diff #29534) | Possible improvement: use const pointer (auto* const label : m_labelsList). |
src/kdefrontend/widgets/LabelWidget.h | ||
103 ↗ | (On Diff #29534) | Remove unnecessary comment. |
104 ↗ | (On Diff #29534) | 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 | ||
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; | |
159 | Move these members to TextLabelPrivate. | |
25 ↗ | (On Diff #29534) | Remove empty lines. |
32 ↗ | (On Diff #29534) | This should be a const member function. |
33 ↗ | (On Diff #29534) | There's no need of another getter, you have already one: TextLabel::LabelShape getLabelShape(); |
40 ↗ | (On Diff #29534) | This should return Qt::BrushStyle because of Qt::BrushStyle patternStyle; |
src/kdefrontend/ui/dockwidgets/cartesianplotdock.ui | ||
20 | Revert this change. | |
1184 ↗ | (On Diff #30106) | 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 | ||
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". | |
11 ↗ | (On Diff #29534) | It should be void labelShapeChanged(int); |
12 ↗ | (On Diff #29534) | It should be void labelShapePreview(int); |
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 ↗ | (On Diff #29534) | 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(); | |
159 | I'll pass if possible, i'll need unnecessarily an additional set/get function pair | |
src/kdefrontend/widgets/LabelWidget.cpp | ||
794 ↗ | (On Diff #29534) | 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 ↗ | (On Diff #29534) | 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