[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 11:19:48
[Download RAW message or body]

On Sunday 05 May 2002 05:50, Andres Krapf wrote:
> [ A patch ]

Ok, there's my nitpicking below. Please read it and comment on it where 
needed. Please don't take all my complaining personally, I just want the API 
to be as sane as possible with the knowledge and means we have together.

> +KopeteMessage::KopeteMessage(const KopeteContact *fromKC, KopeteContact*
> toKC, QString body, MessageDirection direction, QColor fg, QColor bg, QFont
> fnt)

Hmm, dropping the timestamp might be a good idea. I'm not so sure I like your 
change from const KopeteContactList & to KopeteContact*, AFAIK there's a 
default constructor for QPtrList that takes a single pointer as argument.

I'd vote for making the timestamp, foregroundColor, backgroundColor and font 
properties and not mandatory parameters to the contructor btw. They are all 
optional and should all default to the Kopete/user defaults and not require 
additional code to support that. Dunca, would that be ok with you?

Also, I think MessageDirection should become KopeteMessage::Direction, because 
that's the namespace where it ultimately belongs.

> Index: libkopete/kopetemessage.h
> ===================================================================
> +       void done() { emit done(*this); }; // call when a message is done

This one sounds *very* redundant to me. Please don't...

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

Could you make that 'void sent( ... );' ? I think 'sent' is a better name than 
'done'

>         mMessageQueue.clear();
> -       mChatWindow->show();    // show message window again
> +       mChatWindow->setActiveWindow();
> +       mChatWindow->show();
> +       cancelUnreadMessageEvent();
>  }

>  void KopeteMessageManager::messageSentFromWindow(const QString &message)
>  {
> +       // Create the kopete message
>         QString body = message;
> -       KopeteMessage tmpmessage(mUser, mContactList, body,
> KopeteMessage::Outbound);
> -       emit messageSent ( tmpmessage );
> +       KopeteMessage tmpMessage(mUser, mContactList, body,
> KopeteMessage::Outbound);

NOOO!!! Never ever pass QObjects by reference! That will cause all nasty 
crashes you can think of and more! You have to change those methods to all 
use pointers instead, or you will get a crash ...

> +       // Get to know when the message is sent so we can print it in the
> chatwindow
> +       connect(&tmpMessage, SIGNAL(done(const KopeteMessage&)), this,
> SLOT(printMessage(const KopeteMessage&)));

... here. By the time your function is left the temporary variable is deleted. 
I can assure you that accessing the message from the slot will be a read from 
already-freed memory. :-)

> +       // What is this doing here ???? messages should be read when they
> arrive, not when they're sent
>         readMessages();

No idea. Duncan?

>  void KopeteMessageManager::appendMessage( const KopeteMessage &msg )
>  {
>         //mMessageQueue.append( KopeteMessage( msg.timestamp(), msg.from(),
> msg.to(), msg.body(), msg.direction(),msg.fg(), msg.bg(), msg.font()));
> -       mMessageQueue.append(msg);
> +       mMessageQueue.append(&msg);

Should be a pointer instead...

> +void KopeteMessageManager::printMessage( const KopeteMessage& message ) {
> +
> +       mChatWindow->messageReceived(message);

Again, use pointers.

>         const KopeteContactList& members() const { return mContactList; };
> /* Sorry, had to change this to members(), it was conflicting with
> kxContact */

Huh? Those are two completely different classes...

> +       void messageSent( KopeteMessage& msg );

const KopeteMessage * instead I'd say. Is there a reason to make this 
non-const?

> +KopeteMessageManager* KopeteMessageManagerFactory::create(
> +       const KopeteContact *user, KopeteContact* other, KopeteProtocol*
> protocol, +       QString logFile)
> +{
> +       KopeteContactList tempKcl;
> +       tempKcl.append(other);
> +       return (create(user, tempKcl, protocol, logFile));
> +}

Again, please use QPtrList directly, instead of adding this extra method.

Oh, there is no easy accessor method to QPtrList... Hmm...
Ok, leave this in, also above in KopeteMessage.

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]