Hello, On Friday 12 July 2013 01:28:21 Thomas Lübking wrote: > Hi Pali > > Following some comments on the branch. > I read the diff and wrote them in a train so it's rather brief > ;-) > > On a general note, anyc API in a loop of course sucks but it > might still worth trying to get rid of the nested event loops >
Now I do not see easy way how to get rid off synchronous calls in
trojita code, so I wrapped new async code to local loops.
> Cheers,
> Thomas
>
> --
> Q_ASSERT(sender());
> QLineEdit *toEdit = static_cast<QLineEdit*>(sender());
> - QStringList contacts =
> m_mainWindow->addressBook()->complete(text, QStringList(),
> m_completionCount);
> +
> + AddressbookJob *job = m_completionRequests.take(toEdit);
> +++ // TODO this can lead to a race where never ajob
> completes in time. +++ // should check whether old jobs can
> still reasonably complete? + if (job) {
Job should finish (successfull or failed), it is up to plugin. I
think that it is better to do not show data and not to show old
data.
> + disconnect(job, 0, 0, 0);
> + job->stop();
> +++ // TODO how do the get off the hash?
QLineEdit is asociated with job. So other option could be to move
job pointer to new class inhered by QLineEdit.
> + job->deleteLater();
> + }
> +
> + AddressbookInterface *addressbook =
> Common::PluginLoader::instance()->addressbook();
> + if (!addressbook || !(addressbook->features() &
> AddressbookInterface::FeatureCompletion))
> --
> namespace Ui
> {
> @@ -103,6 +104,8 @@ private slots:
>
> void setUiWidgetsEnabled(const bool enabled);
> +++ // TODO completionDone
> + void completeDone(const NameList &completion =
> NameList()); +
Fixed.
> +++ // TODO QMap is gonna perform better here
> + QHash <QLineEdit *, AddressbookJob *>
> m_completionRequests; +
>
I thought that QHash is better for pointers (integers).
> --
> - } else {
> - setToolTip(link + tr("<hr/><b>Address
> Book:</b><br/>") + identities);
> +
> + AddressbookInterface *addressbook =
> Common::PluginLoader::instance()->addressbook();
> + if (!addressbook || !(addressbook->features() &
> AddressbookInterface::FeaturePrettyNames)) {
> +++ // TODO have a fast string now and the special version
> later. + setToolTip(m_link);
Changed
> + return;
> + }
> +
> + m_toolTipJob =
> addressbook->requestPrettyNamesForAddress(addr.mailbox +
> QLatin1Char('@') + addr.host);
> --
> + return;
> + }
> +
> + bool first = true;
> + QString identities;
> +++ // TODO \n is pointless and last entry will have extra
> break. test count()
No, it will not have. <br> is added before entry. But I simplified
this code now.
> + Q_FOREACH(const QString &str, matchingIdentities) {
> + if (first)
> + first = false;
> + else
> + identities += QLatin1String("<br/>\n");
> --
> +++ b/src/Gui/EnvelopeView.h
> @@ -24,6 +24,10 @@
>
> #include <QModelIndex>
> #include <QLabel>
> +++ //TODO QWeakPointer
> +#include <QPointer>
> +
> +class QEventLoop;
> +class AddressbookJob;
>
> --
> #endif
> +
> + saveSignalCount = 0;
> +
> + connect(general, SIGNAL(saved()), this,
> SLOT(slotAccept())); +++ // TODO prefix, not post
> + saveSignalCount++;
> + connect(imap, SIGNAL(saved()), this, SLOT(slotAccept()));
> + saveSignalCount++;
> + connect(cache, SIGNAL(saved()), this,
> SLOT(slotAccept())); + saveSignalCount++;
Fixed
> --
> +
> +void SettingsDialog::slotAccept()
> +{
> + disconnect(sender(), SIGNAL(saved()), this,
> SLOT(slotAccept())); + saveSignalCount--;
> +++ // TODO QT5, test isConnected
Why is needed to test if is connected? QDialog::accept() is
called after all (above) widgets emit signal saved.
> + if (saveSignalCount > 0)
> + return;
> QDialog::accept();
> }
>
> --
> +
> + for (int i = 0; i < addressbookPlugins.size(); ++i) {
> + const QPair<QString, QString> &plugin =
> addressbookPlugins.at(i); +
> addressbookBox->addItem(plugin.first + QLatin1String(" - ") +
> plugin.second, plugin.first);
> + if (addressbookPlugin == plugin.first)
> +++ // TODO "addressbookIndex < 0 &&"
> + addressbookIndex = i;
> + }
> +
Fixed
> + for (int i = 0; i < passwordPlugins.size(); ++i) {
> + const QPair<QString, QString> &plugin =
> passwordPlugins.at(i); +
> passwordBox->addItem(plugin.first + QLatin1String(" - ") +
> plugin.second, plugin.first);
> +++ // TODO "addressbookIndex < 0 &&"
> + if (passwordIndex == plugin.first)
> + passwordIndex = i;
> + }
> +
> + addressbookBox->addItem(tr("None - Disable addressbook"),
> QLatin1String("None"));
> --
Fixed
> - QSettings s;
> - QString user =
> s.value(Common::SettingsNames::imapUserKey).toString(); -
> QString pass =
> s.value(Common::SettingsNames::imapPassKey).toString(); +
> const QString &user =
> QSettings().value(Common::SettingsNames::imapUserKey).toString
> (); +
> +++ // TODO cache the password?
Why to cache password? Also in kwallet you can change password.
> + PasswordInterface *password =
> Common::PluginLoader::instance()->password();
> + if (password) {
> + PasswordJob *job = password->requestPassword(user,
> "imap"); + if (job) {
> + connect(job, SIGNAL(passwordAvailable(QString)),
> this, SLOT(authenticationContinue(QString)));
> --
> +void TrojitaPluginJob::start()
> +{
> + if (m_running)
> + return;
> + m_running = true;
> +++ // TODO QMetaObject::invokeMethod
> + QTimer::singleShot(0, this, SLOT(doStart()));
> +}
> +
Why to use QMetaObject::invokeMethod instead QTimer::singleShot?
> +
> +protected:
> + TrojitaPluginJob(QObject *parent);
> +
> +protected slots:
> +++ // TODO slots don't have to be virtual
> + /** @short Reimplement starting job */
> + virtual void doStart() = 0;
> +
> + /** @short Reimplement stopping job */
> + virtual void doStop() = 0;
> --
I need to make sure that plugins implement doStart and doStop
methods. And it is not possible to force that without pure
virtual methods.
> +
> +void KDEAddressbook::openContactWindow(const QString &email,
> const QString &displayName)
> +{
> + // check if running: qdbus org.kde.kaddressbook
> + // if not start: kaddressbook
> +++ // TODO this *severely* blocks! process starting and
> dbus introspection
> + if
> (!QDBusConnection::sessionBus().interface()->isServiceRegister
> ed("org.kde.kaddressbook").value()) + {
> + if (QProcess::execute("kaddressbook") != 0)
> + return;
> + }
> +
> +++ // TODO forking can take time and this might fail
> + if
> (!QDBusConnection::sessionBus().interface()->isServiceRegister
> ed("org.kde.kaddressbook").value()) + return;
> +
> + QDBusInterface ifaceIntrospect("org.kde.kaddressbook",
> "/KAddressBook", "org.freedesktop.DBus.Introspectable");
> +
I refactored this dbus code, and now using only async qt dbus
functions.
> --
> + return;
> + }
> + m_isOpening = true;
> + delete m_wallet;
> + // TODO: How to tell application name??
> +++ // TODO fixed string - the information is worthless
> anyway and the dialog to be removed
m_wallet is not dialog but single object for accessing kwallet.
it can be reused (if wallet was not closed) for next kwallet
access, so I dot no removing it. Dialog is shown by some kde
process and I do not know how to tell it application name which
accessing wallet.
> + m_wallet =
> KWallet::Wallet::openWallet(KWallet::Wallet::NetworkWallet(),
> 0, KWallet::Wallet::Asynchronous);
> + connect(m_wallet, SIGNAL(walletOpened(bool)), this,
> SLOT(walletOpened(bool)));
> +}
> +
> +void KWalletManager::walletOpened(bool opened)
> --
> +
> +class NullAddressbookCompletionJob : public
> AddressbookCompletionJob +{
> + Q_OBJECT
> +
> +++ // TODO make abook internal to support overlay and
> merge? - drop this one in case
> +public:
> + NullAddressbookCompletionJob(const QString &, const
> QStringList &, int, QObject *parent) :
> AddressbookCompletionJob(parent) {} +
> +public slots:
> + virtual void doStart()
This NullAddressbook plugin was first plugin (which report
hardcoded values) to test if API and code which using that API
working fine. NullAddressbook is only for developing/testing.
I commited all patches to my pali-gsoc-merge branch.
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
