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

List:       kopete-devel
Subject:    Re: [Kopete-devel] patch, and other stuff
From:       Daniel Stone <dstone () kde ! org>
Date:       2002-05-05 12:26:50
[Download RAW message or body]

On Sun, May 05, 2002 at 01:19:48PM +0200, Martijn Klingens wrote:
> 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.

Yes, there is. This is how I had it. I suggest it stays that way.

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

Um, they are - in kopetemessage.h. If you define the default param
*again* in the actual source file, it generates a compiler warning. I
cleaned all of these occurrences up.

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

Umm.

> > +signals:
> > +       void done(const KopeteMessage&);
> 
> Could you make that 'void sent( ... );' ? I think 'sent' is a better name than 
> 'done'

I don't like the idea of a message emitting signals. At all.

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

Yes.

> > +       // 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. :-)

Heh.

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

I put that there. The logic behind it is this:
* You send a message.
* Message goes into the queue.
* Message then waits with all other unread messages until you click on
  "read messages".

Obviously, this sucks IMHO. If you send a message, then I dare say you
want to read all the unread ones. So, I added this code in, to make sure
the message queue got cleared; including the message you just sent.
Pressing enter (well, ctrl+enter, but it should be enter, grumble
grumble), and watching your message disappear would be nasty. :)

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

I'm buggered if I know what's going on here. AIMContact can't inherit
both kxContact and KopeteContact, surely??

> > +       void messageSent( KopeteMessage& msg );
> 
> const KopeteMessage * instead I'd say. Is there a reason to make this 
> non-const?

I agree.

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

Hm, I agree. +1 for sanity.

-- 
Daniel Stone	   <daniel@raging.dropbear.id.au>   http://raging.dropbear.id.au
KDE Developer	   <dstone@kde.org>	                      http://www.kde.org
Kopete: Multi-protocol IM client	    http://www.kdedevelopers.net/kopete/

[Attachment #3 (application/pgp-signature)]
_______________________________________________
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]