On Thu, 10 Jul 2003, Martijn Klingens wrote:

> Ok, let's tear the diff apart :)
> 
> On Thursday 10 July 2003 19:54, Grzegorz Jaskiewicz wrote:
> > Index: kopetecontact.cpp
> > ===================================================================
> > RCS file: /home/kde/kdenonbeta/kopete/libkopete/kopetecontact.cpp,v
> > retrieving revision 1.140
> > diff -u -p -u -r1.140 kopetecontact.cpp
> > --- kopetecontact.cpp   6 Jul 2003 14:52:24 -0000       1.140
> > +++ kopetecontact.cpp   10 Jul 2003 17:49:31 -0000
> > @@ -101,6 +101,9 @@ KopeteContact::KopeteContact( KopeteAcco
> >  
> >                 parent->addContact( this );
> >         }
> > +       
> > +       reasonStatus=QString::null;
> > +       
> 
> This is the default for a QString constructor anyway, no need to explicitly 
> initialize it like this. Also, this should be 'd->reasonStatus', not 
> 'reasonStatus' (see below).
Aye aye.
 
> >  }
> >  
> >  KopeteContact::~KopeteContact()
> > @@ -153,6 +156,20 @@ void KopeteContact::setOnlineStatus( con
> >         KopeteOnlineStatus oldStatus = d->onlineStatus;
> >         d->onlineStatus = status;
> >         emit onlineStatusChanged( this, status, oldStatus );
> > +}
> > +
> > +void KopeteContact::setReason( QString& newreason)
> 
> Should be 'const QString &', not 'QString &'.
Aye aye sir.
 
> > +{
> > +    reasonStatus=newreason;
> > +    // i am not sure about that, but i don't want to create new signal
> > just +    // for a "status reason" change. And this is a status change too.
> > +    // so why not to use one signal for that ?
> > +    emit onlineStatusChanged(this, d->onlineStatus, d->onlineStatus);
> > +}
> 
> This code itself is ok, but as you already correctly point out the signals 
> being emitted are awkward, to say the least.
> 
> Just a while thought, opinions welcome:
> 
> Would it be a good idea to change setOnlineStatus into something like
> void setOnlineStatus( const KopeteOnlineStatus &newStatus,
>     const QString &description = QString::null );
As for gadu, yes. This is enough. I will incorporate all your comments and 
give another patch.

> Should be fully documented, i.e. 'void setReason( const QString &newReason )'. 
> That is, if we keep this method...
No, we going to get rid of it, i will move it to setOnlineStatus if nobody 
is against.
 
> >  private:
> >         KopeteContactPrivate *d;
> > +       QString reasonStatus;
> >  };
> And why do you think that d pointer is there? Just for fun? ;-)
Sorry sir, :-)
 
> This all said, I find the 'reasonStatus' name awkward, but I can't come up 
> with a good name either. Some native English speaker with a clue here?
Well, i am living in UK., anyway -  this was just a quick patch. 
I guess statusReason is more proper. Maybe statusDescription (but this 
will confuse people, becouse of description field in kopeteonlinestatus. 
Of course my plan was to remove this field from kopeteonlinestatus - if 
so, i will leave it as statusDescription).

Thanks. 

--
GJ

_______________________________________________
Kopete-devel mailing list
Kopete-devel@mail.kde.org
http://mail.kde.org/mailman/listinfo/kopete-devel