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

List:       kopete-devel
Subject:    Re: [Kopete-devel] kopetecontact again
From:       Martijn Klingens <klingens () kde ! org>
Date:       2003-07-10 20:52:20
[Download RAW message or body]

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

>  }
>  
>  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 &'.

> +{
> +    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 );

?

No definitive answer, brainstorming aloud, and thoughts welcome.

> (...)
> +       
> +       void setReason(QString&);

Should be fully documented, i.e. 'void setReason( const QString &newReason )'. 
That is, if we keep this method...

>  private:
>         KopeteContactPrivate *d;
> +       QString reasonStatus;
>  };

And why do you think that d pointer is there? Just for fun? ;-)

Please move your reasonStatus to the d object as well.

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?

Please do not commit this yet, even with the above incorporated, I want to 
hear other opinions regarding the potential addition of the reason directly 
to setOnlineStatus first.

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