D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[patrick j pereira]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200510/df67839c/attachment-0001.htm>