[prev in list] [next in list] [prev in thread] [next in thread]
List: kopete-devel
Subject: Re: [Kopete-devel] kopetecontact again
From: Grzegorz Jaskiewicz <gj () pointblue ! com ! pl>
Date: 2003-07-10 21:13:05
[Download RAW message or body]
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic