On Sunday 11 August 2013 10:22:27 Jan Kundrát wrote: > 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. >
Done.
> >> - 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.
>
Right, it working. Now I moved PluginInterface to Plugins
namespace too.
> >> - 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.
>
I already moved plugins code to Plugins subdir, to Plugins
namespace and to Plugins library.
> > What about renaming PluginLoader to PluginManager?
>
> +1
>
Done.
> >> 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.
>
Both plugins passes information in signals params. This is
unified. But only in ComposeWidget.cpp code I need sender()
because pointer of Job is removed from queue. This is needed
because ComposeWidget had sync API for completion and it was not
easy to rewrite code to use new async addressbook job without
sender().
> >> - 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.
>
Ok, then I will combine commits back to one, so it will not break
compilation or something else.
> >> 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.
>
Ok.
> >>> - 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.
>
Ok.
> >> 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?
>
Ok, will look at it.
> >> 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.
>
Ok, this make sense.
> >> - 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.
>
Yes, it is random.
> 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.
>
Ok, then I set default plugins to qsettings one and
abookaddressbook one.
And then I remove description from trojita settings.
> 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.
>
I do not like this automagic idea of selecting plugins. I bet
that in some specific situation & configuration it will not work as
user expect...
> >> - 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.
>
Ok.
> > 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.
>
I only need unique qstring identifier for each account. And this
string needs to be passed to each PasswordJob operation.
So until multiple account support is there, I can use unique
string "1" (now it is username). And then when multiple accounts
will be there you will pass account identifier instead "1". Ok?
> > 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).
>
I know rebase & amend (already using it :-)) But I do not want to
change git history too much, because this breaking sharing code
between two locations...
> >> - 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.
>
Now I added cmake_dependent_option so it is not possible to
enable KDE and QT5 together.
> > 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.
>
Now with option and cmake_dependent_option it is possible to
choose which plugin(s) do you want to comple and also if you want
to disable compiling trojita executable. So you configurations 1)
2) and 3) are possible.
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
