On Monday, 2013-07-08, Pali Rohár wrote: > On Thursday 04 July 2013 18:38:40 Kevin Krammer wrote: > > Hi, > > > > I had a look at the current state of the integration clone. > > > > TrojitaPluginJob.h > > > > - autoDelete() and isRunning() should be const and normal > > public method (not slots). > > Ok, changed.
const You could keep setAutoDelete a slot, I was only referring to the two getter methods :) > > === > > > > Plugins > > > > I take it name() is used as an identifier and not displayed to > > the user? > > Name is internal trojita plugin name (identifier). It is stored in > config file, so trojita know which plugin will load (at startup). > > But in settings dialog is visible to user: > name() + " - " + description() > > When specified plugin name is not found in setting dialog is: > name() + " (not found)" > > I think it is also usefull for user to see plugin name. I would advise against using unstranslatable strings in the user interface. A better approach is to have an identifier (used internally) and a name (displayed to the user). > > === > > > > KDE AddressBook > > > > - using deprecated KABC API parts, e.g. KABC::AddressBook. > > Should use Akonadi API instead > > KABC::AddressBook still working fine (on KDE4.10) and is faster on > my notebook than akonadi. I do not want hard dependency in > Trojita on akonadi which slow down notebook-like devices. Well, the plugin is called "KDE", suggesting its purpose is KDE integration :) This is also the topic of the GSoC project. Now, using the "KDE Addressbook" means using Akonadi, because this is what KDE's main addressbook interface, KAddressBook, is using. This can be done using the Akonadi API or compatibility plugins for KResource based API (KABC::AddressBook). Naturally the direct approach not only allows more flexibility, it also removes the need for a compatibility layer that has a hard time[1] mapping a synchronous API (KResource) onto an asynchronous system (Akonadi). You can trust me on the latter because I am the author of said compatibility plugins :) Using them will also increase the memory usage of the application because in order to fulfil the API contract (load all contacts) it will, well, load *all* contacts. > > - calling KAddressBook for adding/editing is probably not a > > good idea either. There is a contact editor widget and > > related UI components in kdepimlibs > > Hm, I did not know that. I will look at it. One of the classes is called ContactEditor [2]. > > === > > > > EnvelopeView.cpp > > > > - contains a rather hackish nested event loop in > > htmlizeAddresses(). nested event loops are evil, source of > > tons of hard to fix crashes in old KDE apps > > You are right, but I think this event loop is correct and could > not crash. Rewrite method htmlizeAddresses() in correct way is > really hard, so I decided to port old code with local event loop > now. How do you protect against reentrance? If htmlizeAddresses() is called while the nested event loop is running, a new instance will be assigned to the m_loop member. The first job to return will exit and delete the second instance, the first loop will be "lost". If you really need to fake synchronous behavior I would strongly adise on creating a JobRunner that takes the job and runs it and collects its result. For the only safe way of doing it see [1]. Cheers, Kevin [1] needs to run asynchronous operations in a secondary thread to ensure it can really block the calling thread on blocking API calls. [2] http://api.kde.org/4.x-api/kdepimlibs- apidocs/akonadi/contact/html/classAkonadi_1_1ContactEditor.html -- Kevin Krammer, KDE developer, xdg-utils developer KDE user support, developer mentoring
signature.asc
Description: This is a digitally signed message part.
