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: 


> 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...

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

> 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 :-)

  R236 KWidgetsAddons


To: rkflx, #frameworks
Cc: dhaumann

Reply via email to