[trojita] Re: GSoC merge request

[Thomas Lübking]

On Montag, 29. Juli 2013 11:01:44 CEST, Jan Kundrát wrote:

On Wednesday, 24 July 2013 11:08:09 CEST, Pali Rohár wrote:

I rebased pali-gsoc-merge branch again with squashed fixes. It contains replaced QEventLoop code, fixed hardcoded libraries in CMakeLists.txt for building KDE plugins and other small fixes.

First part of comments ;-)
(I'll try to read the rest somewhen tohinght)

+
+void PluginLoader::reloadPlugins()
+{
+    delete m_addressbook;
+    delete m_password;

I recommend to have a parameter (flag) to hint what plugin to reload to avoid 
unreqired (net) I/O
to recreate the same Addressbook just because the user changed the password 
backend.

+
+    AddressbookJob *job = m_completionRequests.take(toEdit);
+
+    if (job) {
+        disconnect(job, 0, 0, 0);
+        job->stop();
+        job->deleteLater();
+    }

The completion race (you type faster than the backend responds and never get 
any response) is still open.
What about having a job queue[2], only replace the recent job and move the 
recent job to oldjob once oldjob
returns?
This way really slow backends (ie. remote ones) would for sure send data from 
time to time, yet we don't
spam them with long request chains.


+void EnvelopeView::checkAddressesWork()
"checkAddressesKnown()"?

Also, why not launch the jobs for all addresses in parallel?
(Ideally condense addresses before or in ::addToSet(), there could be multiple 
entries)

+    }
+
+    checkAddressesWorkDone();
"finishAdressesKnownCheck()" / "finishCheckAdressesKnown()"? - 
"checkAddressesWorkDone()" sounds like a signal

+    QTimer::singleShot(0, this, SLOT(checkAddressesWork()));
*cough*



+void ImapPage::reloadPasswords()
this reloads only one password, doesn't?

@@ -405,10 +589,28 @@ void ImapPage::save(QSettings &s)
        s.setValue(SettingsNames::imapProcessKey, processPath->text());
    }
    s.setValue(SettingsNames::imapUserKey, imapUser->text());
-    s.setValue(SettingsNames::imapPassKey, imapPass->text());
    s.setValue(SettingsNames::imapStartOffline, startOffline->isChecked());
    s.setValue(SettingsNames::imapEnableId, imapEnableId->isChecked());
    s.setValue(SettingsNames::imapBlacklistedCapabilities, 
imapCapabilitiesBlacklist->text().split(QChar(' ')));
+
+    PasswordInterface *password = Common::PluginLoader::instance()->password();
+    if (password) {
+        PasswordJob *job = password->storePassword(imapUser->text(), 
QLatin1String("imap"), imapPass->text());
+        if (job) {
+            connect(job, SIGNAL(passwordStored()), this, SIGNAL(saved()));
+            connect(job, SIGNAL(error(PasswordJob::Error)), this, 
SLOT(slotStorePasswordFailed()));
+            job->setAutoDelete(true);
+            job->start();
+            return;
+        }
+    }
+    emit saved();

You've to be careful here - if "save()" is called in relation to a global 
application exit, the job may not finish in time.
It must inhibit quit or run sync (eg. catch aboutToQuit, check if the job is 
still up, then some invoke some waitForFinished)

+void OutgoingPage::reloadPasswords()
singular as well?

+- void OutgoingPage::save(QSettings &s)
+    PasswordInterface *password = Common::PluginLoader::instance()->password();
same problem here.

+class KDEAddressbook : public AddressbookInterface
Kevin is better suited to comment on this, but if KABC is deprecated, this should perhaps better not 
read "KDEAddressbook", to prevent any confusion of this being "the default KDE 
Addressbook" implementation -> s/KDE/KABC/g

Cheers,
Thomas