patrickelectric added a comment.
Just some small tips.
INLINE COMMENTS
> kcolorcombo.cpp:74
> bool paletteBrush = (k_colorcombodelegate_brush(index,
> Qt::BackgroundRole).style() == Qt::NoBrush);
> - if (isSelected) {
> - innercolor = option.palette.color(QPalette::Highlight);
> - } else {
> - innercolor = option.palette.color(QPalette::Base);
> - }
> + QColor innercolor = option.palette.color( isSelected ?
> QPalette::Highlight : QPalette::Base);
> +
Missing const, also the name should be `innerColor` with a capital C if we are
following the code style from this file.
> kcolorcombo.cpp:83
> +
> + auto textColorLambda = [&paletteBrush, &isSelected, &option,
> innercolor]() -> QColor {
> + QColor textColor;
missing reference for innercolor ?
> kcolorcombo.cpp:107
> painter->setRenderHint(QPainter::Antialiasing);
> - painter->drawRoundedRect(innerrect, 2, 2);
> + QRect colorRect = !tv.toString().isEmpty() ?
> QRect(innerrect.x(), innerrect.y() + innerrect.height() / 2,
> innerrect.width(), innerrect.height() / 2) : innerrect;
> + painter->drawRoundedRect(colorRect, 2, 2);
Maybe changing the logic to make it more simpler:
QRect colorRect = tv.toString().isEmpty() ? innerrect : QRect(innerrect.x(),
innerrect.y() + innerrect.height() / 2, innerrect.width(), innerrect.height() /
2);
> kcolorcombo.cpp:107
> painter->setRenderHint(QPainter::Antialiasing);
> - painter->drawRoundedRect(innerrect, 2, 2);
> + QRect colorRect = !tv.toString().isEmpty() ?
> QRect(innerrect.x(), innerrect.y() + innerrect.height() / 2,
> innerrect.width(), innerrect.height() / 2) : innerrect;
> + painter->drawRoundedRect(colorRect, 2, 2);
It's missing const
> kcolorcombo.h:52
> Q_PROPERTY(QList<QColor> colors READ colors WRITE setColors)
> + Q_PROPERTY(QList<QPair<QString, QColor>> namedColors READ namedColors
> WRITE setNamedColors)
>
This is just a suggestion and not something that's necessary to do, but maybe
it could help to create a simple class to replace QPair:
class KComboColor {
Q_PROPERTY(QColor color MEMBER color)
Q_PROPERTY(QString name MEMBER name)
public:
QColor color;
QString name;
}
REPOSITORY
R236 KWidgetsAddons
REVISION DETAIL
https://phabricator.kde.org/D29502
To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham,
bruns
