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

List:       kmail-devel
Subject:    Re: [PATCH] Don't block remaining messages when sending a message
From:       Till Adam <till () adam-lilienthal ! de>
Date:       2003-04-27 18:16:20
[Download RAW message or body]

On Sunday 27 April 2003 17:51, Ingo Klöcker wrote:
> On Sunday 27 April 2003 11:30, Till Adam wrote:
> > currently failure to send one message in the outbox blocks all
> > others. Unless there is some sort of policy/security reason for this
> > which I don't know about, I propose the attached changes to avoid
> > that.
[snip]
> A few comments:
> - Even if sending of some messages failed we should still show how many
> messages could be sent. So you should change
>   setStatusMsg(i18n("Failed to send (some) queued messages."));
> to something like
>   if( mSentMessages == 0 )
>     setStatusMsg( i18n( "No message was sent successfully." ) )
>   else if( mSentMessages == mTotalMessages )
>     setStatusMsg( i18n( "%n message was sent successfully.",
>                         "%n messages were sent successfully.",
>                         mSentMessages ) );
>   else
>     setStatusMsg( i18n( "%n of %1 messages was sent successfully.",
>                         "%n of %1 messages were sent successfully.",
>                         mSentMessages ) )
>                   .arg( mTotalMessages ) );

I thought informing the user that not all is well (which is the default) was 
the most important piece of information. Telling him 13 of 14 succeeded 
doesnt make the fact very explicit that one failed. But then, there is the 
dialog when it fails, so you are right, I guess. I'll change that.

> - The KDE style guide says that buttons with the labels "Yes" and "No"
> should be avoided. Instead use "&Continue sending" and "&Abort sending"
> as labels for the buttons.

Will do.

> - "\n\n" in messages is depreciated. Use Qt's richtext tags if you want
> to separate paragraphs, i. e. enclose paragraphs in <p>...</p>.

Ah, nice. Will do.

> - Text puzzles like msg = msg + i18n("\n\n...") should be avoided as
> they could prevent proper translation into some strange languages. In
> this particular case it might work in all languages since it just
> combines two different paragraphs, but to be on the safe side please
> remove the text puzzle and instead repeat the first paragraph in the
> message with the additional paragraph.

Ok, done.

> - I don't understand this:
> +      if (mCurrentMsg && mCurrentMsg->msgId() < mLockedMessages)
> +        mLockedMessages--;
>
> KMMessage::msgId() is a string (the Message-Id of the message) and
> mLockedMessages an integer. How can you compare them and for what
> purpose are you trying to compare them? And why do you decrease
> mLockedMessages? It's only increased if sending of a message failed. So
> in which case is decreasing this variable necessary?

Thinko/Typo (too much working with threading ;). The idea was to cope with the 
fact that a locked message might leave the outbox after all, but that isn't 
possible until the next queue run, you are right.

> BTW, I would prefer mFailedMessages instead of mLockedMessages as
> variable name.

Right again, I will change that.

Thanks for the review. I will work in your changes and commit this, if no one 
yells.

Till

_______________________________________________
KMail Developers mailing list
kmail@mail.kde.org
http://mail.kde.org/mailman/listinfo/kmail

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