Hi Pali,
looking at the work you've done so far. First of all, it looks great -- I
believe that you're on a good track.
- Why are TrojitaPluginJob::start/stop postponing the actual operation via the
event loop? I'm not saying it's bad, but I'm wondering why. A comment
explaining this would be great.
- Please prefer member initialization within the constructor, not its body,
like this:
Foo::Foo(): member(0), something("else")
{
// more stuff
}
- typedef QList<QPair<QString, QString> > NameList; is unintuitive (what's
first, a mail address or a pretty name?). Could you please make this a two-member struct?
- The individual enums within the AddressbookInterface::Feature should be
doxygened as well.
- I'd presonally pick "plugin" instead of "pluginloader" for the QSettings keys.
- Why are you employing the singletton (anti)pattern within the PluginLoader?
What are the advantages of using it (i.e. a global variable in disguise) over
an ordinary member of the MainWindow?
- Adding "/usr/lib" into the plugin path is just broken, IMHO. Sure, plugins
"can" be installed into /usr/lib, but why exactly? The .ts files are not looked up from
random places, either.
- The plugin loader contains extra qDebug; these shall be removed or #ifdefed.
- I'd rewrite the PluginLoader::loadPlugin plugin type detection like this, but
it's just me:
if (FooPlugin *plug = qobject_cast<...>(...)) {
// got Foo
} else if (BarPlugin *plug = qobject_cast...) {
// got plugin Bar
} else {
// WTF is this
}
- Why is there that feature for "reloading plugins"?
- In 6fe8ad615d58c27987f9a7a969e692b4bd96337a, you're using a "QAction
*action"; could you please give it a more specific name?
- Typo in commit message of 16c308fe1fc26de7aba33bd22372f5b59757b6cf: "...which
hide*s* the input characters"
- Nope, the approach taken in d68ecb9d507d98de0afd33a21b393ff33fe93fdf is *not*
acceptable. I understand that the qwwsmtpclient's code sucks, but using these event loops
is just asking for tons of trouble. What about chaning the MSA's interface so that it
includes signals/slots for asking for password (similar to what the IMAP Model already
does)? You might want to do this at the time the MSA gets created and "wait"
for an appropriate slot being called, similar to how the src/Composer/Submission.cpp
handles state transitions.
- Please don't touch src/XtConnect.
- I cannot accept the nested event loops in the EnvelopeView::htmlizeAddresses.
What about collecting all addresses during the first pass, requesting a lookup
of all of them asynchronously, setting the evenlope text without pretty colors
and replacing them with the real, pretty thing when all jobs have
failed/succeeded?
- Why the QVariant::type comparisons in the setings dialog?
- Are you sure that 'if (passwordPlugin == "None")' really does what you want
here? What about l10n?
- The async stuff for saving passwords from inside the settings dialog is
interesting, but I don't recall seeing such an application. Perhaps I'm wrong,
but wouldn't it make more sense to move the password remembering bits into
extra dialogs? If you don't like this, then it would be cool to have some
visual clue when the PWs are *loading* at the initial show event -- perhaps a
disabled widget, or a spinner, or both?
- Coding style: try to wrap overly long lines (iirc 130 chars) like the one in
b/src/Plugins/ClearTextPassword/ClearTextPassword.cpp (and others)
- I'd prefer to have a single commit adding both the .cpp/.h files and the
corresponding cmake stanzas, at least for plugins (it's fine to have a separate
commit for each plugin)
- New code shall build with -DQT_NO_CAST_FROM_ACII (i.e. use QLatin1String
around string literals)
- I guess packagers would appreciate a flag for only building a single plugin
at a time (or am I wrong?)
- babbe972343a7f57f32a44d13d61035360ecd4b0 looks like it should be squashed
somewhere (as well as a848f045d7838e6a39bc290d8e3427a611470ed4 and maybe cople
more of them)
- Typo in a commit message: "when selected configuration do*es* not use
password"
- I agree with only showing the plugin description in the settings dialog, but
I'm not sure why do you need to store both name and description in the settings
file.
- Regarding the KABC/Akonadi discussion, what I'd like to have in the end is a way for
KDE users to use "their addressbook" from inside Trojita. I lack any knowledge
of the KDEPIM internals; if there's a compatibility API which can access both (i.e. make
the stuff easier for you) and which won't get removed (ABI stability promise), I don't
see a problem with that. But as I said, I'm not familiar with the details, so perhaps
there's a compelling reason for either of the two separate APIs.
- I don't see any unit tests for verifying that the whole thing works. (While
implementing this, you'll probably find out why I dislike singletons.)
So, keep up the good work -- it's looking great.
Thank you Thomas, Kevin and Caspar for your feedback and for stepping up for
mentoring, this is much appreciated, you've raised good and valuable comments.
I'm now going back to packing for my next trip with confidence that Pali will
receive good guidance. I owe you guys a beer (Thomas, please keep count of the
times I've promised one to you and let's settle them when we meet some day).
Cheers,
Jan
--
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
