On Saturday, 13 July 2013 12:45:18 CEST, Pali Rohár wrote:
- 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.
Kevin suggested that. To have async behaviour and also make sure
that start() method does not emit job finished signals (which
simplify usage).
e-mails : Jan -> 1:0. Sorry, I missed this mail.
Could you please add a comment about this to the generic plugin .cpp?
- 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?
Access to PluginLoader is needed also from other classes, not
only from MainWindow.
I'd prefer to have this access explicit -- i.e. the other places should be
provided with either a pointer to MainWindow or to the plugin loader
itself. Hiding this into a singleton pattern is only making the whole thing
less opaque.
Plugins are shared libraries (.so, .dll, ...) and on more systems
are installed to common library directory.
And problem is: How to get this common library directory from Qt?
QLibraryInfo::LibrariesPath is directory of qt libraries not our
trojita plugins! On some systems are qt libraries not installed
in common library directory.
Maybe better solution could be to get prefix/install library
directory from cmake and pass it to cpp code via compiler define.
Yup, that's what I'd like to have. Again, sorry for note seeing this mail
earlier :(.
- The plugin loader contains extra qDebug; these shall be
removed or #ifdefed.
These qDebugs are usefull for debugging, so I'd like to have
them. Is there any common #ifdef name for qDebug?
In Trojita, these debugging stanzas are done in an ad-hoc, per-file manner.
Perhaps a commented-out #define PLUGIN_DEBUG at the top of the
PluginLoader.cpp file?
My current code allows that library can support more plugins via
one object. But I do not know if it is usefull or not...
Interesting, but would that work in practice -- wiht one create() method
and all?
- Why is there that feature for "reloading plugins"?
It is called from SettingsDialog when user change plugin from
menu. It is needed because if he change plugin in menu also
passwords should be reloaded/stored to new plugin.
OK, so it's for changing the "current implementation" of each feature --
makes sense, thanks.
- I guess packagers would appreciate a flag for only building
a single plugin at a time (or am I wrong?)
Maybe CMakeLists.txt can start using option() for all switches?
I'm not familiar with this, but it looks good -- patches welcome.
- 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.
This was already discussed. When user select plugin A, then close
trojita and uninstall plugin A. Trojita will show in settings
dialog that plugin A is missing. But Trojita should not show
internal plugin id, but transtaled description. So this is reason
why in config file is stored also last transtaled description. If
user install plugin A again, then Trojita after next (re)start
will use it.
I'm not convinced that it's worth the extra code to keep track of the
human-readable descriptions. But well, the code is written already, so I
guess it shall be left in now.
Hm, I do not know qt test api. I will look at existing tests.
What is needed to test? If addressbook and password jobs return
correct data?
I'm more interested with the *interaction* of the individual bits -- it's
good to know whether the IMAP code can actually request the password from
the plugin or what happens when the plugin takes too long to respond.
Sorry for missing this mail again.
Cheers,
Jan
--
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
