On Thursday, 8 August 2013 22:04:52 CEST, Pali Rohár wrote:
- Why is function "loadPlugin" trying to unload plugins?
It is not "trying", but clean up resources when library is not
for trojita or if plugin for category is already loaded and
selected. But for creating list of available plugins it is needed
to open all libraries call needed functions and close them.
Please add a comment about this.
- The plugins, interfaces, jobs etc. shall probably be
namespaced. "namespace Plugins"?
It is possible to have Qt interfaces in some namespace? Or can
there be some problems?
They should be supported, AFAIK.
- The plugin headers were added into libCommon. Perhaps a
separate static lib would be better.
I think that plugins code is something for Common. It is could be
usefull in any trojita code part.
Based on Thiago's comment, I assume that the way to go is to create a
shared library with plugin-specific code and nothing else.
What about renaming PluginLoader to PluginManager?
+1
Kevin suggested to pass Job* as an argument; that would save
you a call to sender() here.
I think this is only place where I used AddressbookJob sender().
Are there any problems with sender()?
Yes, consistency. You are adding two types of plugins; using one of them
requires sender() while the other one passes that information within the
signals. I'd prefer to have this unified.
- Commit ec7e8ca0b61b5649644ec9073bf7b1278336abfb contains a
regression, it removes the feature for showing the proper
icon in the envelope view.
This commit changing Window.{h,cpp}. Which icon it breaking?
Maybe I referred to toher commit. The point was that because you've split
that change into multiple commits, there's now time when the EnvelopeView
won't add a correct icon next to the e-mail addresses. If possible, this
should be addressed before the final merge.
Commit 6df3e61005bed504ed4f05fe00794375d6cf09f3:
+void SMTP::sendMailContinue()
That name doesn't explain what is it for.
"sendMailGotPassword"?
Is there problem with gotPassword signal?
Nope, the problem is that "sendMailContinue" doesn't make it clear on
*when* it's called. This is not a generic function for continuing a process
after some undefined pause -- it's a method which should be called after
the password is provided.
In addition to that, I think that you should change the logic a bit. Both
sendMail and sendBurl should set some private variable "m_smtpMode" (enum
of SMTP_DATA and SMTP_BURL to be added by you) so that you don't require
two slots, sendMailContinue and sendBurlContinue. You've added a partial
unification of these code paths via the `QByteArray data` member (and the
from and to variables), so let's make it complete.
- qwwSmtp->sendMailBurl(from, to, imapUrl);
+ qwwSmtp->sendMailBurl(from, to, data);
That looks like a copy-paste error, isn't it?
No. data is private variable. And because from and to variables
are used both in sendMail and sendMailBurl I added only one
variable for third param - so data.
Indeed, my bad.
- Are you sure that this passes the unit tests? Is the new way
of setting passwords covered by a unit test?
I did not touched unit tests now.
But you are running `make test` after each commit, right? Breaking the unit
tests is similar to breaking the build in my criteria of a branch
acceptance -- all commits must be atomic in this regard as well.
In
general, I'd prefer to have this "prompt for SMTP password"
in master before the plugins are there; it's a very useful
Could you please backport this to master?
Perhaps include the name/description of the plugin in the
tooltip?
Do you really need to see description of addressbook plugin in
tooltip? I think that tooltip should be simple.
The tooltips for the envelopeview already contains an info about the
address being stored in the address book with some human-readable name.
What I was suggesting is including the source of that information in
preparation to support of multiple address books.
That way, the application could show that this address is known in the
"Automatically collected addresses" as "John Sixpack", while my personal
address book calls it "Johhny the Bull", or whatever.
- Why are you passing both the plugin ID and the description
to the userData of the QComboBox?
Because for configuring/selecting plugin. It storing also
description and code which configuring plugin is called from
GeneralPage::save where I have only userData of QComboBox.
...so only because you need to save not only "internal plugin ID", but also
the "human readable description" in the settings, right?
I would get rid of that.
- Why using QL1S("none") instead of, say, null QVariant() to
indicate a null plugin? "none" is a magic word.
Because none is also stored to config file. Empty string (which is
by default when user start trojita first time or doing upgrade)
means first available plugin and not none. I think that none
option is not good idea to have it as default. And due to
QSettings empty string is default value.
I don't understand the reasoning behind "first available plugin". If I
understand this correctly, the plugin loading order is completely
unspecified, so the "first one" is a random choice. I don't think that's a
useful property.
What I suggest is either defaulting to AbookAddressbook and
within-the-qsettings passwords by default, or adding some code for checking
how much the plugin "fits" into the system.
Of course, this will have to be yet another pure virtual in the plugin
interface, something like `int howMuchThisFitsToTheSession() const`.
- 0-50: the plugin will work, but it might suck a bit
- 50-100: the plugin will work as good or as bad as an other plugin
- 101-150: the plugin has detected that it should probably be preferred in
this session (e.g. the user is running KDE and this is a KDE plugin, so
let's return 110 for the KABC-based one and 120 for the Akonadi one)
That's just an idea, the point is that picking random choices (and even
reserving an item for this in the GUI!) is, IMHO, rather unexpected.
- You can use QComboBox::findData to find the correct index.
Where you think this can be used?
When you get rid of the tuple passing into userData(), you can use that
funciton to find the QComboBox offset of the chose plugin ID.
Multiple passwords? Maybe you mean multiple accounts.
Yes, sorry for a typo (or maybe a braino).
Method
requestPassword has two arguments. First is accountId and second
accountType. When Trojita support multiple accounts, it will
needs some unique identifier for account.
The point is that multiple accounts *will* be there, and I definitely do
not want to have to fix the plugin API when it gets merged. Please come up
with some requirements on what you need to have in order to support
password storage for multiple accounts, and change the code so that it uses
that.
AbookAddressbook is now shared library. And by default shared
library will export only symbols marked as Q_DECL_EXPORT/IMPORT.
Old code created new instance of AbookAddressbook and then passed
pointer to contructor of Contacts. To reduce count of symbols
which needs to be export, I rather moved this code into new
methos BE::MainWindow() and exported it.
Ah, cool. Please try to include these bits in the commit messages (and you
can do it now, `git rebase -i` and `git commit --amend` support this).
- WITH_KDE cannot supported with Qt5.
I think this is up to packages. Or do you want to add all checks
for all configurations into cmake? There is also Harmattan code
which cannot be enabled with anything else...
I think that it would be an useful addition, yep.
Problem is that KDE4Defaults moding global CXX_FLAGS. But now
compilation should work fine. KDE4_ENABLE_EXCEPTIONS is what is
needed.
- What about making a standalone CMAkeLists.txt for the
KDE-specific plugins? That should make it very easy for
packages, won't it?
It is needed? I already added some cmake options. And I can add
option per plugin...
In the end, you still have to provide cmake configuration for:
1) building trojita *and* some well-defined subset of plugins
2) building trojita the application and nothing else
3) building one plugin at a time
It looks to me that having one CMakeLists.txt per plugin and enabling them
via the add_directory cmake command is the easiest solution here.
Cheers,
Jan
--
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
