On Saturday, 27 April 2013 15:39:48 CEST, Sreepriya Chalakkal wrote:
I had made a patch for the feature that we discussed. Can you
please review that?
Hi Sreepriya,
thank for the patch. In general, it looks good, but I have a couple of comments
nonetheless:
- Please always send your contributions to the mailing list. That way, more
people can see it and point out the potential problems.
- I appreciate if the patch is available as a git branch I can just pull from.
That saves me time.
- The patch changes unrelated newlines.
- Why are you replacing a LineEdit with a QLineEdit?
- This is wrong:
QLineEdit *edit = static_cast<QLineEdit*>(sender());
It should use either qobject_cast (because it's Qt) or dynamic_cast (for
non-QObject classes) and be accompanied by a Q_ASSERT(edit). That way it uses
C++'s features for increasing the type safety a bit.
- I don't like the green background on valid addresses that much. Yes, it's
useful to have the red background in case of an invalid address, but I think
that a regular, white one is fine for valid addresses.
- Please fix the indenting, it is different than the rest of the code.
- What is this unrelated change doing there?
@@ -665,6 +687,7 @@ bool
ComposeWidget::parseRecipients(QList<QPair<Composer::RecipientKind, Imap::M
} else {
errorMessage = tr("Can't parse \"%1\" as an e-mail
address.").arg(text);
return false;
+ i--;
}
}
return true;
With kind regards,
Jan
--
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
