[Syncevolution-issues] [Bug 6378] automatic sync

> A task in the list is checked when the interval has passed and then > it has to be checked whether there is an existing same task in > the scheduling queue of auto sync manager. Once actived, it is put in. Without looking at the code I don't understand this. What is the task list? What is the scheduling queue? How are entries in both related? It shouldn't be necessary to read the code to understand the commit message.

+ /** + * A single task for automatic sync. + * Each task contains one peer name, peer duration and peer url. + * Once it is loaded by AutoSyncManager, auto sync manager will do sync + * according to these informations. + */ + class AutoSyncTask + { + public: + string m_peer; + unsigned int m_peerDuration; + string m_url; I can see that a task has these members, that doesn't have to be documented ;-) But what do they do resp. what do they contain? I can guess the answers, but others might not know, and I shouldn't have to guess.

+ /** map to contain all peers that enable automatic sync */ + typedef std::map<unsigned int, boost::shared_ptr<AutoSyncTaskList> > PeerMap; + PeerMap m_peerMap; Maps what to an task list?

+ /** + * It reads all peers which are enabled to do auto sync and store them in + * the m_peerMap and then add timeout sources in the main loop to schedule + * auto sync tasks. + */ + void init(); When a config is modified, is this reinitialized? I think a brute-force approach is sufficient: just wipe out all pending tasks and recreate them based on the config. This can (should?) be done at the end of a Session which made config changes, in case that the owner of that session makes multiple config changes.

+/** * Implements the main org.syncevolution.Server interface. * * All objects created by it get a reference to the creating @@ -693,11 +910,17 @@ class DBusServer : public DBusObjectHelper DBusConnectionPtr m_connmanConn; friend class Session; + friend class AutoSyncManager; Why is it necessary to let AutoSyncManager break the encapsulation of DBusServer?

+ /** two timers to record when the http and bt are present */ + Timer m_httpTimer; + Timer m_btTimer; Not when - how long, right? - :m_httpPresence (false), m_btPresence (false), m_initiated (false), m_server (server) + :m_httpPresence (false), m_btPresence (false), m_initiated (false), m_server (server), + m_httpTimer(15*60*100), m_btTimer(15*60*100) Is this supposed to be 15 minutes? Then it has to be 15*60*1000 (Timer takes milliseconds). This constant should be configurable, for tuning purposes.

+ if(httpChanged) { + m_httpTimer.reset(); + } + if(btChanged) { + m_btTimer.reset(); + } Again, what do these timers count? Shouldn't they count time since the transports become available? This looks like they count time since the last change.

+bool AutoSyncManager::taskLikelyToRun(const AutoSyncTask &syncTask) +{ + DBusServer::PresenceStatus &status = m_server.getPresenceStatus(); + // avoid doing any checking of task list if http and bt presence are false + if(!status.getHttpPresence() && !status.getBtPresence()) { + return false; + } + + if(boost::istarts_with(syncTask.m_url, "http") && status.getHttpPresence()) { + // don't add duplicate tasks + if(!findTask(syncTask)) { + Timer& timer = status.getHttpTimer(); + // if the time peer have been around is longer than 'autoSyncPeerDuration', + // then add it into task list + if(timer.timeout(syncTask.m_peerDuration * 60 * 100)) { + return true; + } + } else if (boost::istarts_with(syncTask.m_url, "obex-bt") && status.getBtPresence()) { + // don't add duplicate tasks + if(!findTask(syncTask)) { + return true; + } + } + } + return false; +} This looks wrong. The check for obex-bt is inside the HTTP check?