Hi, review time again.
Three comments in general:
1) Some of the commits look a bit too fine grained to me. I don't see any
benefits in e.g. adding the plugin interface .h declaration in once commit,
settings keys in a second one, a plugin loader in the third one and only
enabling them in cmake in the next commit. To me, that doesn't help with
bisecting and makes it harder to follow the directions in which the patch
series is going, so I'd prefer to have these squashed into a single commit.
2) The second thing, what I found a bit unintuitive was the naming of
various classes. In my opinion, "PasswordInterface" shall be named
"PasswordPlugin" because it's, well, a plugin that one can ask to execute
certain actions. The current naming has its own consistency, it's just that
I find it a bit unintuitive.
The same applies for "TrojitaPluginInterface" -- I would call it an
"AbstractPlugin".
3) Some of the commits are breaking the build. That's unacceptable; if a
commit breaks the build, it will immediately break the `git bisect`
workflow for finding regressions. As a rule of thumb, if you change API of
any class, you are supposed to fix all of its users in the same commit. If
you break any tests, you have to fix them within the same commit.
I'd prefer to have the same rules for changes which introduce breakage to
the user-facing functionality (like being able to login to an IMAP server)
-- hence my requests for backporting some of your changes to master and
rebasing your branch on top of that.
More specific issues follow; some of them are simply a repeat from the next
time. Please be sure that comments are addressed; an "I don't agree with
this, for me, the way of XYZ is better because of ABCD" is better than not
reacting to the review comment at all.
- Commit df40ea7f83ec08350c1736f9a1d20e81c8b81203 -- why do you need to
track the plugin directory in application settings? Again, why it isn't
enough to copy the same logic as for loading the .ts files for l10n?
- Why do you store the plugin's *description* in settings? I understand the
plugin ID, but not the human readable description. Perhaps it's needed --
but if so, a comment saying that *must* be in the source.
- The fact that qobject_cast doesn't work across shared library boundaries
is suspicious. Wasn't supporting *that* use case one of the reasons
(besides not requiring RTTI) for introducing qobject_cast over
dynamic_cast?
- Why is function "loadPlugin" trying to unload plugins?
- Coding style: look out for e.g. extra whitespace when changing the
includes and make sure that they are sorted (order: the cpp's matching .h,
all system headers, all Qt headers, all Trojita headers. Sort order is
alphabetical, including the path. Use with the directory path (not
including "src/") when including stuff different than .cpp's own .h.)
- The plugins, interfaces, jobs etc. shall probably be namespaced.
"namespace Plugins"?
- 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.
I haven't got any response about this (commit
2a70055482dd90a8834fc643e41c102aef720836).
- Please prefer member initialization within the constructor,
not its body, like this:
Foo::Foo(): member(0), something("else")
{
// more stuff
}
This one still applies as well. Commit
2a70055482dd90a8834fc643e41c102aef720836, maybe others, too.
- 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?
Not answered.
- 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 comments about Debian's multilib do not answer this to me. Please
explain why you need /usr/lib in there.
Also keep in mind the use case of a developer who has both a systemwide
Trojita installation from packages and a git checkout (without having
called `make install`). Trojita from git shall definitely load *local*
plugins, not the systemwide ones, IMHO.
- The plugin loader contains extra qDebug; these shall be
removed or #ifdefed.
Not addressed.
- 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
}
Have you considered this comment? A good reason for that is that the
related code (variable being defined & set and the place it's actually
used) are together.
- Why is there that feature for "reloading plugins"?
Still not clear to me.
- The plugin headers were added into libCommon. Perhaps a separate static
lib would be better.
+ // Load plugins (needs correct application data)
+ Common::PluginLoader::instance();
If a class is named "plugin loader", my understanding is that it is *used*
for loading plugins when the programmer calls some method on that class.
The fact that it happens as a side effect of the instantiation is very
surprising to me.
If you'd like to have an RAII-ish approach to plugin lifecycle management,
perhaps it should be called "AutoLoadPlugins" or something like that. If
you go that route, I would prefer to have an actual "loader" separated into
another class, though.
- commit 546581201b79d721f6bcb22bc930eb4aaca2536f:
+ if (job) {
+ disconnect(job, 0, 0, 0);
+ job->stop();
+ job->deleteLater();
+ }
This is disconnecting everything connected to any signal of that job. Did
you mean to disconnect(job, 0, this, 0)?
+ AddressbookJob *job = static_cast<AddressbookJob *>(sender());
Kevin suggested to pass Job* as an argument; that would save you a call to
sender() here.
- Commit ec7e8ca0b61b5649644ec9073bf7b1278336abfb contains a regression, it
removes the feature for showing the proper icon in the envelope view.
- Commit f13ff62ae8ea8264073a1cf3e8a048eabdadee8c should be probably
cherry-picked to master and removed from your branch because it has nothing
to do with the platform integration plugins.
- The same for 9e77b0a12f9176fc4d113f12e2048a3cae1b1a22 and
6df3e61005bed504ed4f05fe00794375d6cf09f3.
Commit 6df3e61005bed504ed4f05fe00794375d6cf09f3:
+void SMTP::sendMailContinue()
That name doesn't explain what is it for. "sendMailGotPassword"?
- qwwSmtp->sendMailBurl(from, to, imapUrl);
+ qwwSmtp->sendMailBurl(from, to, data);
That looks like a copy-paste error, isn't it?
- Are you sure that this passes the unit tests? Is the new way of setting
passwords covered by a unit test?
- Commit e2dc2346967d46815b75ecd6b35e6dee753f166a (and the next one) should
be squashed into the previous ones; otherwise you're introducing
regressions to the tree. In general, I'd prefer to have this "prompt for
SMTP password" in master before the plugins are there; it's a very useful
feature (and an already requested one). Does it work for you?
- Commit 8938f94457ca4c15bbb766dcc2b84ebd409812ae is an absolute no-go --
each and every comit *MUST* compile.
- Commit 0be7a151cbe7c2eadf122705e149d523d3ca3ac9 for XtConnect is broken;
it's a non-interactive application and therefore it will break when
passwords are requested interactively. It shall *not* request the password
from KWallet etc, it shall read it from the QSettings to prevent breakage
for the existing users.
- Commit e3923adc697e3e6ae0dbcf4e22832304d43e686e:
-void EnvelopeView::setMessage(const QModelIndex &index)
+void EnvelopeView::setMessage(const QModelIndex &modelIndex)
{
// ... some real diff here
+
+ index = modelIndex;
Looks like excess diff noise.
- That commit also queues all addresses to be looked up sequentially. Why
not sending them away all at once and watching for all requests to
complete?
- The disconnect(job, 0, 0, 0) as above.
+ setToolTip(m_link + tr("<hr/><b>Address Book:</b><br/>") +
identities);
Perhaps include the name/description of the plugin in the tooltip?
+ QPointer <AddressbookJob> addressJob;
Extra space before the first <.
- Commit 38da896ce740be286366ff09562967af1b132061 -- if it's a bugfix for
the existing master, please submit it separately. If it's fixing something
in previous commits in the gsoc branch, please squash it to the offending
commit instead.
- Commit 96dca654d3dc2db3a4dfd32f355bddbe781aa39e:
+ const QList<QPair<QString, QString> > &addressbookPlugins =
pluginLoader->availableAddressbookPlugins();
+ const QList<QPair<QString, QString> > &passwordPlugins =
pluginLoader->availablePasswordPlugins();
+ const QPair <QString, QString> &addressbookPlugin =
pluginLoader->preferredAddressbookPlugin();
+ const QPair <QString, QString> &passwordPlugin =
pluginLoader->preferredPasswordPlugin();
+ int addressbookIndex = -1;
+ int passwordIndex = -1;
Please put related code together -- e.g. passwords first, then abook.
- Why are you passing both the plugin ID and the description to the
userData of the QComboBox?
- Why using QL1S("none") instead of, say, null QVariant() to indicate a
null plugin? "none" is a magic word.
+ addressbookBox->addItem(addressbookPlugin.second + QLatin1Char(' ') +
tr("(not found)")
addressbookBox->addItem(tr("%1 (not found)").arg(addressbookPlugin.second)
preferrably with some explaining comment (check QObject::tr's docs on how
to pass it, it's a magic comment iirc) telling the translators what is it
for.
In general, one shall not make assumptions on how is some thing done in a
foreign language.
- You can use QComboBox::findData to find the correct index.
commit 978ad3b4d6242c072eeacdbb6976d8e04f7f0b9e
+QMessageBox::critical(this, tr("IMAP password"), tr("Storing IMAP
password failed"));
Please tell the user:
- that the error comes from some plugin,
- which plugin it is,
- anything that the plugin might have to add ("cannot connect to your fancy
proprietary PW storage because you need to pay $100 for your license
extension", "cannot contact UberSafe PW daemon: not enough entropy, please
move your mouse" etc)
The same applies to other places which are saving passwords as well.
+PasswordJob *job = password->requestPassword(imapUser->text(),
QLatin1String("imap"));
How is this going to change when Trojita supports multiple passwords? It
shall probably include the hostname at least.
- Commit 983414e26f38a0780431471dadb933b94ac03afb:
Would be cool to not show the fancy border of the warning label when saving
in encrypted storage. Also perhaps include the plugin name?
- Commits c162f6e22db41156231c5ede4e4be8eb5478cc1b and
7ee728efb0ca0ee1741a5112b47f5a0099ad92bf break the build as you're moving
files around without modifying CMakeLists.txt. This *must* be fixed and
makes me unhappy.
- Commit 35628964681e7d662d6a301ebf5290781c4a05aa -- what's the point of
said API changes? I understand the namespace move, but not the rest. Also,
it's apprently breaking the build -> a no go.
- Commit 6e9c3dbf79dd86a2bcf8d40004e229be5eb8c903:
+ if (m_accountType == QLatin1String("imap"))
+ password =
QSettings().value(Common::SettingsNames::imapPassKey);
+ else if (m_accountType == QLatin1String("smtp"))
+ password =
QSettings().value(Common::SettingsNames::smtpPassKey);
+ if (password.type() != QVariant::String)
+ emit error(PasswordJob::NoSuchPassword);
+ else
+ emit passwordAvailable(password.toString());
- In this case, please use {} braces even for one liners; it will
dramatically improve readability. I had to look twice to notice that it's
not one big blcok of if/else-if/else-if/else, but in fact two separate
ones.
- It shall include an else (for unrecognized PW type) complainig that it
got called with wrong account info (not "imap" or "smtp"). Yes, it's
covered by the first condition of the second if switch, but that's more of
a side effect than a robust code, IMHO.
- This class apparently requires the delayed call to doStart so that the
connections are already set up. I'm still not sold on that it's a good idea
to enforce this for all TrojitaPluginJobs, but perhaps you're right. This
shall be documented in that generic class.
- Whenever any branch of the if/else contains {} braces, *all* branches
must have them.
- Again, separate commits for adding .cpp/.h and another for cmake
modifications are wrong.
- What are NullAddressbook and NullPasswordPlugin and why are they needed?
- Commit 70ce7d1b20d2419f4f56d629479891b5356fabd8 -- is there some error
handling for `kaddressbook` not being available, crashing etc?
- Commit 8d2c9128ebb94dbd4fdd950cf20d172febb673c2 shall be squashed into
the previous two.
- WITH_KDE cannot supported with Qt5.
- include(KDE4Defaults) and messing around with
add_definitions(${KDE4_ENABLE_EXCEPTIONS}), etc -- what about simply
applying the KDE-specific stuff to only the KDE-specific plugins?
- What about making a standalone CMAkeLists.txt for the KDE-specific
plugins? That should make it very easy for packages, won't it?
Cheers,
Jan
--
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
