Bug 52755 - SyncEvolution code cleanup
Summary: SyncEvolution code cleanup
Status: RESOLVED MOVED
Alias: None
Product: SyncEvolution
Classification: Unclassified
Component: SyncEvolution (show other bugs)
Version: unspecified
Hardware: All All
: medium enhancement
Assignee: SyncEvolution Community
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-26 19:00 UTC by SyncEvolution Community
Modified: 2018-10-13 12:46 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Patrick Ohly 2012-07-29 18:36:00 UTC


---- Reported by jingke.zhang@intel.com 2010-04-26 19:00:45 +0000 ----

This is from http://bugzilla.moblin.org/show_bug.cgi?id=3472

   Description  From  pohly   2009-06-12 07:57:10 PST   (-) [reply]

This issue is about several aspects of the SyncEvolution source code. It could 
be cleaned up in several ways. All of these changes are not strictly necessary
(minor severity) but should be done at some point (priority 2).

SyncItem::setData() now exists in a variant which takes a std::string. Check
all calls of the form setData(str.c_str(), str.size()) and replace them with
setData(str).

ConfigNode has template methods for set/getProperty(). Some old code reads
strings and converts them instead of using the template methods. Change that
code.

Remove "using namespace std" and add "std::" wherever necessary.

All SyncEvolution symbols should be in their own namespace. I suggest SyncEvo.
I don't like that Emacs indents members of namespaces:
namespace SyncEvo {
    int foo;
}

I suggest using some defines like G_BEGIN_DECLS and G_END_DECLS instead, in
particular in header files. Details to be discussed.

For macros we should use the SE_ prefix (not done consistently either!).

The word EvolutionSyncClient and EvolutionSyncSource come from the Funambol
SyncClient and SyncSource classes. Nowadays SyncEvolutionClient and
SyncEvolutionSource seem more natural. Rename the classes and the files?!

------- Comment #1 From pohly 2009-06-18 23:56:03 PST (-) [reply] -------

(In reply to comment #0)
> The word EvolutionSyncClient and EvolutionSyncSource come from the Funambol
> SyncClient and SyncSource classes. Nowadays SyncEvolutionClient and
> SyncEvolutionSource seem more natural. Rename the classes and the files?!

Perhaps abandon "SyncClient" entirely. At some point we will have a SyncML
server which is likely to be using the same class. "SyncEvolutionSession"?

------- Comment #2 From pohly 2009-07-20 06:41:40 PST (-) [reply] -------

Not all code compiles cleanly on all platforms: https://bugs.meego.com/show_bug.cgi?id=4555

------- Comment #3 From pohly 2009-10-05 06:08:55 PST (-) [reply] -------

The externally visible part is done for 0.9.1. Rescheduling the rest for later.

(In reply to comment #0)
> SyncItem::setData() now exists in a variant which takes a std::string. Check
> all calls of the form setData(str.c_str(), str.size()) and replace them with
> setData(str).

TBD.

> ConfigNode has template methods for set/getProperty(). Some old code reads
> strings and converts them instead of using the template methods. Change that
> code.

TDB.

> Remove "using namespace std" and add "std::" wherever necessary.

TBD.

> All SyncEvolution symbols should be in their own namespace. I suggest SyncEvo.
> I don't like that Emacs indents members of namespaces:
> namespace SyncEvo {
>     int foo;
> }
>
> I suggest using some defines like G_BEGIN_DECLS and G_END_DECLS instead, in
> particular in header files.

Done, using SE_BEGIN/END_CXX.

> For macros we should use the SE_ prefix (not done consistently either!).

TBD.

> The word EvolutionSyncClient and EvolutionSyncSource come from the Funambol
> SyncClient and SyncSource classes. Nowadays SyncEvolutionClient and
> SyncEvolutionSource seem more natural. Rename the classes and the files?!

Done:

commit 71fbf32c941ab17355483028a6f529354e58e18e
Author: Patrick Ohly <patrick.ohly@intel.com>
Date:   Mon Oct 5 14:49:32 2009 +0200

    files and classes renamed, include statements cleaned up

    The intention is to get rid of the historic and inconsistent
    naming of some classes and their corresponding files:
    * EvolutionSyncClient = class derived from Funambol's SyncClient,
    * SyncEvolutionConfig = SyncEvolution's config

    With the strict 'namespace SyncEvo' and the syncevo/ path prefix for
    most header files it is no longer necessary to have "SyncEvolution" or
    "Evolution" in the names. This patch thus renames as follows:
      EvolutionSyncClient => SyncContext
      EvolutionSmartPtr => SmartPtr
      SyncEvolutionCmdline => Cmdline
      SyncEvolutionConfig => SyncConfig
      SyncEvolutionUtil => util

    The former EvolutionSyncClient always had a role that went beyond just
    running a sync, for example it also provided config access. With the
    upcoming server support it also won't be just a client. Thus the new
    name "SyncContext".

    The 'syncevo/' prefix is used throughout the code now.

    removed whenever the prefix made it clear that the file belongs
    to SyncEvolution. This helps finding incorrect include paths.

    Quotes should be used exclusively for SyncEvolution files which don't
    have a specific prefix yet (test.h, config.h) to help identifying
    them.



---- Additional Comments From murrayc@murrayc.com 2011-06-08 11:17:40 +0000 ----

As a small first step, the dbus-server code should be split up into one-file-per-class in a sub-directory, to make it easier to maintain the code. Chris has started that here:
https://meego.gitorious.org/meego-middleware/syncevolution/commits/dbus-server-reorganization



---- Additional Comments From murrayc@murrayc.com 2011-06-08 11:25:32 +0000 ----

I suspect that we should also use GDBus (part of glib) instead of libdbus (via libgdbus), because it fixes some general threading problems vaguely hinted at here: http://developer.gnome.org/gio/stable/ch29.html. It would also remove that (copied) code from syncevolution, letting us depend on glib's maintenance of that layer. Using that "standard" C high-level API for D-Bus would also make the code more readable for the typical GTK+/GNOME developer, making it easier to get improvements from the community.



---- Additional Comments From patrick.ohly@intel.com 2011-06-08 14:00:10 +0000 ----

(In reply to comment #2)
> I suspect that we should also use GDBus (part of glib) instead of libdbus (via
> libgdbus)

As mentioned on chat: I'd like to have a fallback for the latest Ubuntu LTS and Debian Stable.

Whether that fallback is the current libgdbus copy or a copy of the glib gdbus doesn't matter.



---- Additional Comments From blixtra@gmail.com 2011-07-14 13:46:18 +0000 ----

(In reply to comment #1)
> As a small first step, the dbus-server code should be split up into
> one-file-per-class in a sub-directory, to make it easier to maintain the code.
> Chris has started that here:
> https://meego.gitorious.org/meego-middleware/syncevolution/commits/dbus-server-reorganization         

I've now gotten to a point where this branch has the same test results as master, currently 3 failures and 3 errors. Thus, I'd say it can be merged once the 1.2 release is out.

The remaining issue is that when configured with both the --enable-static and --disable-shared it does not compile unless one adds -ldl to *_LDFLAGS. However, when using this flag the backends somehow don't get registered. The first such error encountered when running the test is...

======================================================================
ERROR: testCredentialsRight (__main__.TestConnection)
TestConnection.testCredentialsRight - send correct credentials
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/chris/checkout/meego/syncevolution/test/test-dbus.py", line 2258, in testCredentialsRight
    self.setupConfig()
  File "/home/chris/checkout/meego/syncevolution/test/test-dbus.py", line 2151, in setupConfig
    self.session.SetConfig(False, False, self.config, utf8_strings=True)
  File "/usr/lib/python2.7/site-packages/dbus/proxies.py", line 140, in __call__
    **keywords)
  File "/usr/lib/python2.7/site-packages/dbus/connection.py", line 630, in call_blocking
    message, timeout)
DBusException: org.syncevolution.InvalidCall: invalid value 'file' for property 'backend': 'not one of the valid values (virtual, calendar = events, addressbook = contacts, todo = tasks, memo = memos = notes)'
======================================================================

I've been looking into this but have yet to find a fix.



---- Additional Comments From blixtra@gmail.com 2011-07-14 13:47:53 +0000 ----

(In reply to comment #4)
> (In reply to comment #1)
> > As a small first step, the dbus-server code should be split up into
> > one-file-per-class in a sub-directory, to make it easier to maintain the code.
> > Chris has started that here:
> > https://meego.gitorious.org/meego-middleware/syncevolution/commits/dbus-server-reorganization         
> 
> I've now gotten to a point where this branch has the same test results as
> master, currently 3 failures and 3 errors. Thus, I'd say it can be merged once
> the 1.2 release is out.

Hmm. That should be 2 errors. Sorry.



---- Additional Comments From blixtra@gmail.com 2011-08-01 10:50:45 +0000 ----

Patrick fixed up backend registration issue when configuring with --disable-shared from comment #4 and I've added a few more minor cleanups as well. I think dbus-server-reorganization branch should be ready to merge once 1.2 is out and a final rebase is done.



--- Bug imported by patrick.ohly@gmx.de 2012-07-29 20:36 UTC  ---

This bug was previously known as _bug_ 1351 at https://bugs.meego.com/show_bug.cgi?id=1351
Comment 1 GitLab Migration User 2018-10-13 12:46:05 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/SyncEvolution/syncevolution/issues/163.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.