D7163: KSqueezedTextLabel: Add several autotests

[Dominik Haumann]

dhaumann added a comment.


  I think this is already a very good patch. I just have some minor comments. Could you have another look?
  
  By the way: Given you have more patches that build on this one, you should consider applying for a KDE developer account, if you don't have one yet, see: https://community.kde.org/Infrastructure/Get_a_Developer_Account

INLINE COMMENTS

> ksqueezedtextlabelautotest.cpp:39
> +void KSqueezedTextLabelAutotest::squeezeLabel(
> +    KSqueezedTextLabel *label, const int amount = 1) const
> +{

To me, it was not immediately clear whether amount referts to pixel or to characters. So you may want to rename the parameter to pixelAmount or pxAmout or amountInPixels, or maybe even simply "pixels".

> ksqueezedtextlabelautotest.cpp:46
> +{
> +    KSqueezedTextLabel *label = createLabel(QStringLiteral(""));
> +

Use QString() instead of QStringLiteral("") for empty strings.

> ksqueezedtextlabelautotest.cpp:140
> +{
> +    KSqueezedTextLabel *label = createLabel(QStringLiteral(""));
> +

Same here: Prefer QString() over QStringLiteral("")

> ksqueezedtextlabelautotest.cpp:217-234
> +    void (KSqueezedTextLabel::*attributeFn)(int);
> +
> +    // TODO: Does Q_DECLARE_METATYPE support pointers to member functions?
> +    //       Then the switch statement and the enum could be removed.
> +    switch(attribute) {
> +        case setIndent:
> +            attributeFn = &KSqueezedTextLabel::setIndent;

margin, indent, and lineWidth are all Q_PROPERTYs, maybe you can use QObject::setProperty(const char *, QVariant) to set the values instead of using a function pointer? The function pointer is a bit ugly...

Example:
label->setProperty("indent", 40); // will automatically call label->setIndent(40);

> ksqueezedtextlabelautotest.h:35-36
> +private:
> +    KSqueezedTextLabel* createLabel(const QString &text) const;
> +    void squeezeLabel(KSqueezedTextLabel *label, const int amount) const;
> +

Given you don't use any member variables, these could also be free functions in e.g. an anonymous namespace in the .cpp file. This is just a comment though, no strict requirement. I guess it is fine as is :-)

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7163

To: rkflx, #frameworks
Cc: dhaumann
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170806/6ed669e1/attachment-0001.html>