Hello, On Friday 12 July 2013 21:39:21 Jan Kundrát wrote: > 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. >
Kevin suggested that. To have async behaviour and also make sure
that start() method does not emit job finished signals (which
simplify usage).
> - Please prefer member initialization within the constructor,
> not its body, like this:
>
> Foo::Foo(): member(0), something("else")
> {
> // more stuff
> }
>
I using above code Foo::Foo(): member(0), something("else") {}
> - 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?
>
Ok, changed.
> - The individual enums within the
> AddressbookInterface::Feature should be doxygened as well.
>
Added doxygen info to all new enums.
> - I'd presonally pick "plugin" instead of "pluginloader" for
> the QSettings keys.
>
Changed.
> - 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.
> - 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.
>
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.
> - 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?
> - 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
> }
>
My current code allows that library can support more plugins via
one object. But I do not know if it is usefull or not...
> - 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.
> - In 6fe8ad615d58c27987f9a7a969e692b4bd96337a, you're using a
> "QAction *action"; could you please give it a more specific
> name?
>
Ok, I renamed it to actionContactEditor. Also in file
src/Gui/SettingsDialog.cpp
> - 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.
>
Ok, I will look at it and try to remove local event loop.
> - Please don't touch src/XtConnect.
>
It reading passwords from config file, so I changed code to use
Password plugin. So you want to revert this change?
> - 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?
>
Ok, I will try to do it.
> - Why the QVariant::type comparisons in the setings dialog?
>
There is no QVariant::type in last commit. I already refactored
code and removed it.
> - Are you sure that 'if (passwordPlugin == "None")' really
> does what you want here? What about l10n?
>
Yes. Plugin has name and description. Name is only internal
identifier and must not be transtaled. Only Description is visible
to user and it should be translated. Password plugin with "none"
name is special string which means that password plugin is not
used.
> - 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?
>
When passwords are loading passwords input field are disabled.
> - Coding style: try to wrap overly long lines (iirc 130 chars)
> like the one in
> b/src/Plugins/ClearTextPassword/ClearTextPassword.cpp (and
> others)
>
Fixed in all files.
> - 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)
>
This is already done.
> - New code shall build with -DQT_NO_CAST_FROM_ACII (i.e. use
> QLatin1String around string literals)
>
Fixed all my new code.
> - 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?
> - babbe972343a7f57f32a44d13d61035360ecd4b0 looks like it
> should be squashed somewhere (as well as
> a848f045d7838e6a39bc290d8e3427a611470ed4 and maybe cople more
> of them)
>
All new commits (after 8d9dbe51ac25281b51a493302421c97da24d62ca)
are not squashed yet. I will squash them and rebase branch after
fixing all problems. Also I will fix commit messages then.
> - 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.
>
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.
> - 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 will create also akonadi plugin, so Trojita will have have
plugins for both APIs. KABC is part of KDE api, so I think it
will not be removed from KDE4. KABC has also plugin which use
addressbook from akonadi.
> - 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.)
>
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?
> 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
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
