[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kopete-devel
Subject:    Re: [Kopete-devel] patch, and other stuff
From:       Martijn Klingens <klingens () kde ! org>
Date:       2002-05-05 15:57:05
[Download RAW message or body]

On Sunday 05 May 2002 17:30, Andres Krapf wrote:
> -       mBg = QColor();
> -       mFg = QColor();
> +       mBg = QColor(255,255,255);
> +       mFg = QColor(0,0,0);

Is there something like QColor::null ? I think that unless explicitly 
specified the colors should be used from the user's color scheme. Either you 
ask those, or use a null color (if it exists) to indicate 'use default'.

This code would break for people using non-white bg or non-black fg colors in 
KDE.

> -KopeteMessage::KopeteMessage(const KopeteContact *fromKC,
> KopeteContactList toKC, QString body, MessageDirection direction, QColor
> fg, QColor bg, QFont fnt) +KopeteMessage::KopeteMessage(const KopeteContact
> *fromKC,
> +               KopeteContactList toKC, QString body, MessageDirection
> direction) {
>         mTimestamp = QDateTime::currentDateTime();
>         mFrom = fromKC;
>         mTo = toKC;
>         mBody = body;
>         mDirection = direction;
> -       mBg = bg;
> -       mFg = fg;
> -       mFont = fnt;
> +       mBg = QColor(255,255,255);
> +       mFg = QColor(0,0,0);
> +       mFont = QFont();
> +}

I see a lot of code duplication here and in the other KM c'tors. The risk of 
uninitialized variables in one method or another is growing rapidly. Better 
add a KopeteMessage::init() call that is called from all c'tors and does the 
real work, I'd say.

> -class KopeteMessage
> +class KopeteMessage : public QObject

The 'does KM need to emit signals?' problem still remains, unfortunately. the 
overhead of QObjects is rather high and probably unnecessary. If we can't 
find a working alternative it's of course ok, but I'm still a bit hesitant.

> +       // Mutators
> +       void setFg(QColor color) { mFg = color; };
> +       void setBg(QColor color) { mBg = color; };
> +       void setFont(QFont font) { mFont = font; };

As KopeteMessage will be subject to binary compatibility one day or another, 
better not inline the methods. For now it's ok, though. Just good practice 
not to do so, especially in shared libraries.

> +signals:
> +       void sent(const KopeteMessage&);

Another 'good practice' item: name the variables also in signals. In this case 
it's pretty obvious that the argument is called 'msg', but if signals are 
more complex it's not. Make it yourself a habit to always specify the names 
will make your code often more readable.

> -       void messageSent( const KopeteMessage msg );
> +       void messageSent( KopeteMessage& msg );

No references to QObjects! ;-) And please make everything as 'const' as 
possible...

Apart from these minor issues the patch is looking very good already. Thanks 
for the hard work! It's almost ready for commit if you ask me ;-)

Martijn

_______________________________________________
Kopete-devel mailing list
Kopete-devel@mail.kde.org
http://mail.kde.org/mailman/listinfo/kopete-devel
[prev in list] [next in list] [prev in thread] [next in thread]