Hi Thomas,
it looks like my changes are conflicting, so patches 2-5 won't apply
anymore -- sorry, I didn't have a chance to have a look at your patches
before that. Could you please rebase them?
A few comments inline.
+ foreach (const QString &contact, contacts) {
Please use Q_FOREACH instead of foreach.
+ list << contact.first % QString(" <") % mail % QString(">");
This is QStringBuilder, right? AFAIK you cannot use it in Qt 4.6 (which
is the minimal required version and yes, it's needed because of stable
Debian and a random OpenSuSE box at work where my colleagues test
Trojita occasionally.
+ /**
+ Returns a list of matching contacts in the form "[Name <][email protected][>]"
Please use "/** @short Returns blablabla" followed by an empty line and
the rest of the comment
+ - @p max defines the demanded maximum reply length - the return can be
longer, but the client will not make use of that
+ negative values mean "uncapped"
What is this used for?
+ // TODO: convince Jan to have more than one recp. per line
What for? If you have a use case for this, I'd like to hear about it,
otherwise it's going out. I don't really want to see yet more
hand-crafted parsers for strings like '"Arthur C. Clarke, jr."
<[email protected]>'. Parsing of these headers is a can of worms, I
*don't* want to deal with it unless needed. That's why I object.
+ QStringList contacts = m_mainWindow->addressBook()->complete(current,
QStringList(), 8);
Magic number. Please make it a constant/enum/whatever somewhere.
+ struct Completion {
+ int pos, length;
+ QMenu *popup;
+ QLineEdit *receiver;
+ } m_completion;
I don't know what is this class for. Could you please add comments?
I don't insist on having all members documented, but I want to make sure
that I can fix your code seven years in future.
-#include "AutoCompletion.h"
#include "Window.h"
#include "ComposeWidget.h"
#include "Util.h"
@@ -50,6 +49,7 @@
#include "MsgListView.h"
#include "SettingsDialog.h"
#include "TaskProgressIndicator.h"
+#include "AbookAddressbook.h"
I try to have the headers sorted in a certain way -- see the docs [1].
+ m_addressBook = new AbookAddressbook;
Object ownership? MainWindow::nukeModels() handles only IMAP models
because they are tied to the config options. (Feel free to submit a
patch renaming/documenting that function.)
0003-don-t-recreate-layout-when-deleting-a-rec.-it-s-poin.patch
Well, haven't tried that patch as it doesn't apply, but I suspect it's
there for a reason. Where are you adding the extra widgets for new
addresses?
ratio
a) used to setFocus on optionally focus change (triggeres editingFinished)
-> great way to annoy users (enter address, focus to subject -> focus
at new To)
b) if the user actually wanted to add a new address, he would not know hot to
until pressing
enter or set away the focus (see a ...)
c) old way was far too complex and allowed blank fields in the middle
-> new solution is to just ensure there's always a blank fiels at the end
Okay, makes sense. Better to have 0003 and 0004 together, then (IMHO) --
as otherwise you'd remove functionality for a while in history. I don't
like that due to reverts, bisecting etc.
+ recipients <<
qMakePair<ComposeWidget::RecipientKind,QString>(Imap::Mailbox::MessageComposer::Recipient_To,
QString::fromAscii("%1@%2").arg(url.userName(), url.host()));
Line wrapping at column <= 130 please.
But I like this idea of deferring the string-based decision to the latest.
> please notice that the "custom" completer was pretty much a result of
the wish to complete comma separated lists
this works, but the parser will likely rank them invalid.
I'd still like you to have a short look and if you finally say nay, i'll
remove that and send a new patch (currently patches 4 & likey 5 will not
apply w/o 1 & 2)
Unless you can provide a compelling reason, I strongly object to that.
Parsing is already fragile (see above).
W/o custom completion one could also use a regular QCompleter and
reimplement the ::match() function of the model.
This rules out remote adressbooks but actually you don't want to
(blocking) lookup the network on every keypress either, so one would
either require local addressbooks anyway or write an async completion
(possible - we update the activities menu async in kwin - as long as you
don't have the mouse over a chaning item, everything's just fine ;-)
If it's racy, I don't like it. I guess that if a sync API makes is
easier for you, I can live with that.
Last note: if you want to keep the multientry lineedit, it's easy to
replace the popup by a combobox droppdown in case you just don't like that
Not sure what this refers to, sorry -- been a long day here. Could you
please rephrase?
3) for some reason i absolutely don't know the layout was recreated
whenever a recipient was removed.
Because that's how I managed to get it working many years ago :). It's
ugly, thanks for fixing that.
Since the layout changed and that wasn't covered in this place, this
effectively broke the layout.
Unless there's a known good reason and issue for that action, it should
just not be there.
As long as the whole solution matches my requirements, I'd love to get
rid of that hack.
An incomplete set of requirements:
- it's possible to add/remove any recipients in any order
- it doesn't crash
- it isn't more hacky than the current version
- focus order is OK (=better than the current version)
- it doesn't leave empty fields behind
Question: should there be a common include for used types (enums,
typedefs etc.)?
The current de::e::e::e::p nesting clutters the code quite some.
Ack. It's used by both the MessageComposer (which is tied to IMAP) and
by the GUI. Either solve that via some "using...." statement, or propose
a better fit.
Thanks for your work -- looking forward to updated iteration.
Cheers,
Jan
[1] https://projects.flaska.net/projects/trojita/wiki/Coding_style
--
Trojita, a fast e-mail client -- http://trojita.flaska.net/
