[trojita] Re: Another GSoC review
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
[trojita] Re: GSoC merge request
On Tuesday, 30 July 2013 23:23:13 CEST, Kevin Krammer wrote: Hmm. I guess the class could indeed be called KABCAddressbook or KResourceAddressbook. But since those are framework names the user visible string should probably be something like "Legacy KDE Addressbook". Agreed. Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
[trojita] Re: GSoC merge request
On Tuesday, 30 July 2013 22:58:42 CEST, Thomas Lübking wrote:
What if there're successive sendMail calls before the password turned in?
The MSA classes are designed to only support a single submission during
their lifetime, so this is not a problem.
You're right that a defensive design would enforce this via asserts and
mention this in the docs. Patches for both are welcome.
+enum Type {
+Request,
+Store,
+Delete
+};
enums are extendable (though i'm absolutely not sure about
compilers here, we'd have to research this)
-> you could likely move this into PasswordJob, have a barrier enum Type
{
Request = 0,
Store,
Delete,
UserType = 256
};
and in doubt enum Type {
Foo = UserType + 1,
};
in some inheriting class
I don't follow here -- do we expect to have something inherit from this
class?
I agree with the rest of the comments (in all three mails). Thanks for the
review!
Cheers,
Jan
--
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
[trojita] Re: Addressbook interface for qt plugins
On Monday, 8 July 2013 16:05:28 CEST, Kevin Krammer wrote: - AbookAddressbook has lots of public API that is not in AddressbookInterface (e.g. model). It is needed in AddressbookInterface (somewhere for Trojita)? Or where is problem? AbookAddressbook is also used by be.contacts standalone application, so that methods must be public for access outside class. It might be better to separate this into two classes. One that implements the addressbook and one that implements the interface using an instance of that class. One reason is that anyone creating new plugins will look at the existing plugins and it would be better to clearly show which parts are the plugin and which are the code dealing with the actual data access behind the scenes. But it would be good to get the opinion of the ABook code creator/maintainer on this first. This one is for Thomas. From my side, splitting them in any way which makes sense is OK as long as it doesn't complicate the code too much (and now I see that it probably explains what I asked about in my review, I guess). The comment about the existing plugins being used as a baseline is a valid one. Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
[trojita] Re: Addressbook interface for qt plugins
On Saturday, 29 June 2013 10:26:26 CEST, Pali Rohár wrote: - What's the point of storing the plugin path in the settings? Check how the localization is loaded to see what I mean with the path discovery. I see that trojita loading localization from PKGDATADIR. But I do not see where it is defined. PKGDATADIR in autoconf/automake systems points somewhere to /usr/share// but for storing libraries it is better to use system /usr/lib (or other dynamic linker path like /usr/lib/x86_64-linux-gnu/ on multiarch). Library path can be reused from cmake. Ah, I missed this mail when I did the review in my today's mail -- sorry about that. You've found a bug -- the PKGDATADIR used to be set by qmake (see commit 9adbd5d690e42b98ea091bfba58d88f03a4e9d77), but I forgot to port this when switching to cmake -- filed as [1]. The idea is that this points to $PREFIX/share/trojita which is determined at build time. At runtime, "/locale" is appended to that path and .ts files are looked up in there. I was hoping that the plugins could be looked up from PKGDATADIR/plugins. Perhaps that's a wrong path -- it sounds like a bad idea to place binaries in there, after all. The general point is still valid, IMHO -- the path to use for plugin lookup shall IMHO be specified at build time. Perhaps it will be something like /usr/lib/trojita/plugins. Either way, the places to look for plugins shall IMHO be limited to: a) the directory determined at build time, b) application's applicationDirPath() + "/plugins" - The signal/slot signatures in connect(...) are not normalized Do you mean this? http://marcmutz.wordpress.com/effective-qt/prefer-to-use-normalised-signalslot-signatures Yes. This will not work, because in class is full QPointer object - not pointer or reference. My bad, sorry. - What is the NullAddressbookCompletionJob and why is it returning bogous data? Plugin for testing if interface, plugin loader and completion in gui working. Nothing for users. I'd like to have this information in the doxygen docs. I know that some classes in Trojita which I wrote are not exactly a perfect example in this discipline, so sorry for applying double standards here. Anyway, unless you're going to use some of these classes in the automated tests (you probably should, at least for the password operations), the null classes shall probably be removed. Now I have problems with cmake. It does not autogenerate two moc files, so I had to add QT4_GENERATE_MOC to force cmake for manuall generation. Another problem is that all plugins not compiling without adding line "#include "file.moc" at end of plugin cpp file. I assume that this is now fixed, right? I don't see anything like that in current pali-gsoc-merge. Another problem could be qt5. My linux distribution does not have qt5 yet. What do you use for compiling with qt5? I can use virtualized system in qemu, but need to know which distribution have qt5 out-of-box and working fine. Getting it building was a lot of work for me (manual builds from git on Gentoo). I know that Gentoo has some packages, but some pieces like qttools are still missing (and Trojita needlessly depends on that, unfortunately). I don't know how much work is to get another branch from another repo being checked by KDE's Jenkins. Perhaps the easiest way is to get you push access to the repo at github which is set to perform CI using Travis, some hosted CI solution. That way you can know whether your branch at least builds. Under the hood, Travis uses Ubuntu 12.04 and the Qt comes from some third-party PPA -- see .travis.yml in Trojita's source tree. Any KDE plugin has to be disabled when building with Qt5 as you can't have both Qt4 and Qt5 linked into the same object. The other plugins should still work, though. [1] https://bugs.kde.org/show_bug.cgi?id=323197 -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
[trojita] Where to install plugins and localization?
Hi, it turns out that the existing /usr/share/trojita/* is probably not that great place for plugins, binary objects which depends on the platform's ABI etc (thanks to Pali for bringing this up). 1) Where shall we install application-specific plugins? Something like $LIBDIR/trojita/plugins/? 2) Right now, we're installing the .ts translations into /usr/share/trojita/locales/. I see that some applications are instead using /usr/share/apps/$appname/$whatever instead. Do you guys know what is the correct thing to do? Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
Re: [trojita] Re: Another GSoC review
On Monday, 2013-08-05, Jan Kundrát wrote:
> - 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?
Indeed. It is the way all Qt plugins work (styles, image formats, etc)
> > - 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).
Since that was my suggestion let me comment on that :)
It is more or less a safe guard. Some jobs will be implemented in a
synchronous way, e.g. the API they are using being sychronous.
If they do this in start() then their result will be available after start(),
which can easily lead to developers using the job assume that this is always
the case (they start to treat start() like a form of exec()).
If they are testing against an implementation that does, of course.
Such code will work flawlessly for them but not for others, increasing the
test matrix.
If all start() calls are asynchronous by default, nobody will fall into the
trap of assuming start() does anything immediatley useful.
Another option is to lead by example, i.e. make sure all default shipped
plugins that do not use asynchronous API do delay their actual work method in
start().
> - 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)?
I think this is to protect against stop() emitting any signals. Probably
easier to call job->disconnect();
Or avoiding sigals in stop().
> > +AddressbookJob *job = static_cast(sender());
>
> Kevin suggested to pass Job* as an argument; that would save you a call to
> sender() here.
True, though this is closer to Qt APIs like QNetworkReply.
My suggestion was more grounded in the idea that all jobs (actually the base
class) would have a single result signal (like KJob does).
I guess it is mostly a matter of style and I am biased there due to long
exposure to KJob based APIs :)
> - WITH_KDE cannot supported with Qt5.
Not yet :)
But yeah, that will be a while :)
Cheers,
Kevin
--
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring
signature.asc
Description: This is a digitally signed message part.
Re: [trojita] Where to install plugins and localization?
On Monday, 2013-08-05, Jan Kundrát wrote: > Hi, > it turns out that the existing /usr/share/trojita/* is probably not that > great place for plugins, binary objects which depends on the platform's ABI > etc (thanks to Pali for bringing this up). > > 1) Where shall we install application-specific plugins? Something like > $LIBDIR/trojita/plugins/? > > 2) Right now, we're installing the .ts translations into > /usr/share/trojita/locales/. I see that some applications are instead using > /usr/share/apps/$appname/$whatever instead. Do you guys know what is the > correct thing to do? KDE uses $LIBDIR (or maybe something that can be configured but falls back to $LIBDIR), e.g. most of my KDE plugins here are under /usr/lib/kde4 (see also output of kde4-config --path module or kde4-config --path qtplugins) Cheers, Kevin -- Kevin Krammer, KDE developer, xdg-utils developer KDE user support, developer mentoring signature.asc Description: This is a digitally signed message part.
[trojita] Re: Another GSoC review
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/
[trojita] Re: Another GSoC review
On Montag, 5. August 2013 16:40:02 CEST, Jan Kundrát wrote: - 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? Did i miss this? Where's mentioned "does not work" (reason is perhaps lacking Q_OBJECT macro?) Cheers, Thomas
[trojita] Re: Where to install plugins and localization?
On Montag, 5. August 2013 18:28:23 CEST, Jan Kundrát wrote: 1) Where shall we install application-specific plugins? Something like $LIBDIR/trojita/plugins/? +1 It's what Qt and KDE do. Gtk calls them "modules", but those gtk guys are freaks anyway and it's effectively the same ;-) 2) Right now, we're installing the .ts translations into /usr/share/trojita/locales/. I see that some applications are instead using /usr/share/apps/$appname/$whatever instead. Do you guys know what is the correct thing to do? following gettext: /usr/share/locale/$LC/LC_MESSAGES? But sorry, no real idea or opinion here. Cheers, Thomas
[trojita] Re: GSoC merge request
On Montag, 5. August 2013 17:09:58 CEST, Jan Kundrát wrote:
You're right that a defensive design would enforce this via
asserts and mention this in the docs. Patches for both are
welcome.
=P
+enum Type {
+Request,
+Store,
+Delete
+};
enums are extendable (though i'm absolutely not sure about
compilers here, we'd have to research this)
-> you could likely move this into PasswordJob, have a barrier enum Type
{
Request = 0,
Store,
Delete,
UserType = 256
}; ...
I don't follow here -- do we expect to have something inherit
from this class?
There's class "ClearTextPasswordJob : public PasswordJob" and ClearTextPasswordJob
defines "enum Type { Request, Store, Delete };" what sounded pretty common to all
PasswordJob's to me (and both present implementation implement them)
Therefore i thought moving the Type enum to PasswordJob and let the specific
implementations just extend them on demand - better for generic job handling
processing.
THOUGH BEWARE: I have yet NOT checked on whether this is safe for all (usable,
i bet MSVC6 fails) compilers. It's so far just an idea.
(The other idea was to have "Type" and "CTPWJType" - but that's not very
elegant either)
Cheers,
Thomas
[trojita] Re: Another GSoC review
On Monday, 5 August 2013 18:34:04 CEST, Kevin Krammer wrote: Such code will work flawlessly for them but not for others, increasing the test matrix. Understood, you guys have a good point -- thanks. This is disconnecting everything connected to any signal of that job. Did you mean to disconnect(job, 0, this, 0)? I think this is to protect against stop() emitting any signals. Probably easier to call job->disconnect(); Or avoiding sigals in stop(). I was thinking about another valid use case, something watching for object deletion. IMHO it's concievable to have a monitor of all pending jobs which listens for QObject::destroyed(QObject*). This disconnect will suddenly break this use case. I was worried that this might even break QPointer, but it turns out that it's using a completely different mechanism than signals/slots (QObject::addGuard). I learned yet another new thing today -- cool. Anyway, I'm not sure whether this is usable for the monitor use case. Why is there the need to mute the signals anyway? - WITH_KDE cannot supported with Qt5. Not yet :) But yeah, that will be a while :) Yep, I meant "for now" :). Cheers, Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
[trojita] Re: Another GSoC review
On Monday, 5 August 2013 19:54:05 CEST, Thomas Lübking wrote: Did i miss this? Where's mentioned "does not work" (reason is perhaps lacking Q_OBJECT macro?) From pali-gsoc-merge:src/Common/PluginLoader.cpp: // NOTE: qobject_cast not working when trojita is compiled as shared library, dynamic_cast working fine AddressbookInterface *newAddressbook = dynamic_cast(plugin); PasswordInterface *newPassword = dynamic_cast *>(plugin); -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
[trojita] Re: Another GSoC review
On Montag, 5. August 2013 22:32:48 CEST, Jan Kundrát wrote: On Monday, 5 August 2013 19:54:05 CEST, Thomas Lübking wrote: Did i miss this? Where's mentioned "does not work" (reason is perhaps lacking Q_OBJECT macro?) From pali-gsoc-merge:src/Common/PluginLoader.cpp: // NOTE: qobject_cast not working when trojita is compiled as shared library, dynamic_cast working fine Ok, i'll open bets. -- Pali, did you figure this trying on the NullPlugins? -- On a general note and particular on this case (which i missed because after understanding it's just for development testing i rather skipped them): Placing "O_OBJECT" into a source file (*.cpp) does *NOT* work, the MOC does not (easily - and I don't see that in CMakeLists.txt and NO: just write a header ;-) proceed them, thus there's no MetaObject processing and thus there's no qobject_cast'ing to that particular type. Signals and slots of the base class will of course continue to work, but it's not possible to add new ones or shadow the present. dynamic_cast might actually work on gcc, but that's due to some gcc magic and particular linker calls on symbol exports - it's absolutely not reliable. In general dynamic_casting across libraries does *not* work - if you want to do this (outside Qt happyland), you add some nice hints (eg. a type id) on the actual type and then just static_cast them according to the type (and then you can cast down to determine the patch - or you just move to Qt happyland =) To hook onto that: iirc you required Trojitá -fPIC for kontact inclusion: was there a particular reason (from kontact side) or just because (you?) tuned up GCC parameters until dynamic_cast'ing worked in order to make trojitá a shared library. Cheers, Thomas
