[Syncevolution-issues] [Bug 1360] creating client configurations

http://bugs.meego.com/show_bug.cgi?id=1360 --- Comment #1 from jingke <jingke.zhang(a)intel.com&gt; 2010-04-26 19:39:18 PDT --- Go on copying the long comments. Comment #11 From pohly 2010-01-11 10:56:08 PST (-) [reply] ------- (In reply to comment #10) That had been merged into master, but I think we need to discuss it further, therefore I reverted master to the state before that merge. Comments about the patch: builtin templates were removed. I consider this feature useful for cases where someone compiles just a single binary that he wants to copy around (like I recently did with the Android binary). Please preserve builtin templates for *servers*. For clients we can rely on the templates in the filesystem. The file system templates should be minimal, like the Funambol and ScheduleWorld templates were: - no comments - no properties which have the default value The term "finger" doesn't describe to me what the values are. Perhaps "fingerprint" would be better? Can you explain what that property does and what possible values are? Jussi has to review that, because it is likely that he will have to parse those values in the sync-UI. You updated the unit test at the end of the patch series. If tests have to be updated, then this should be done together with the patch that breaks them. ------- Comment #12 From Chen Congwu 2010-01-11 20:40:40 PST (-) [reply] ------- (In reply to comment #11) Sorry, maybe too hurry.. What servers do you want to preserve in particular? All HTTP servers including SyncEvolution? What about servers provided over Bluetooth such as Nokia N900? I hope the template system should be complete and not specific for server or client. The supporting two-solution approach will introduce code complexity and runtime penalty. If you really want a standalone binary, we can add *one* special built-in default template for your tweaking. I thought about the good things about the previous built-in template approach: each time we upgrade the configuration system, we only need to change one place in the code without remembering updating all template files manually. However I think this is an acceptable compromise. ACK. "Finger: the matching string for this template, it is a comma separated string with each string modeled as: "Manufacture_Model". The first substring is also used as the unique name to identify this template so that user can select the template by this name." Perhaps adding some examples would be better? See: Nokia 7210c: Nokia_7210c SyncEvolution server: SyncEvolutionServer, SyncEvolution ScheduleWorld: ScheduleWorld,default SyncEvolution client: SyncEvolutionClient, SyncEvolution ACK. ------- Comment #13 From pohly 2010-01-11 23:52:19 PST (-) [reply] ------- (In reply to comment #12) Yes. Basically what is needed for the traditional HTTP SyncML client use case. No. Code complexity - yes, to some extend. Runtime penalty - that smells like unnecessary optimization, beware of that pitfall. Is it really that difficult? The code supported it before already and it wasn't that horrible, was it? There was some duplication because the Funambol and ScheduleWorld templates were built-in as well as in the files. That was because at some point we had icons for them. Without icons, we don't need the on-disk copy of templates and could remove them. I'd prefer to have the full set. What if <Manufacturer> or <Model> contain commas and/or underscores? Is "SyncEvolution" the manufacturer or the model here? ------- Comment #14 From Chen Congwu 2010-01-12 00:15:17 PST (-) [reply] ------- (In reply to comment #13) So every time we later add a new HTTP server, we add it in the code; for mobile devices we add it via template file out of source. What do you expect from this? I see no necessary or even improvement via this approach. It just adds maintenance overhead. > > > > No. > > > I hope the template system should be complete and not specific for server or > > client. The supporting two-solution approach will introduce code complexity and > > runtime penalty. > > Code complexity - yes, to some extend. Runtime penalty - that smells like > unnecessary optimization, beware of that pitfall. > Is it really that difficult? The code supported it before already and it wasn't > that horrible, was it? It's not complex. I just seems to me not necessary. I am still not convinced so far. If you insist, I of course can follow your approach. Not considered so far. If there is really such strange manufacture or model, we can fix that later. You can think something like that. ------- Comment #15 From pohly 2010-01-12 02:46:33 PST (-) [reply] ------- (In reply to comment #14) Yes. A useable single-file installation of SyncEvolution. I consider this a valuable feature. If you want to implement this in a different way, say, by defining all templates as files and importing a subset (or even all) into the binary at compile-time, then that would also be a viable solution. The code looking for templates could use the install location if it is available, otherwise use the built-in defaults. In that case it would be either/or instead of "first files, then builtin". If you use an internal abstraction layer for accessing templates, then it's even fairly clean. Once we role out 1.0 final and users start adding template files for their own phones, they don't have the option of modifying the source code of the syncevo-dbus-server or the sync-ui. The question of parsing these strings is only relevant if we reliably need to decompose the strings into <manufacturer> and <model>. This might not be necessary at all. A fuzzy string match might do just fine. For a hierarchical view of templates, we could split at _ (*any* underscore, regardless how many there are) and treat the individual words as "most general" to "most specific". That would also work for future extensions, like Nokia_N7120c_v1, Nokia_N7120c_v2. ------- Comment #16 From Chen Congwu 2010-01-12 04:52:52 PST (-) [reply] ------- (In reply to comment #15) Okay, I see. I was thinking something different thus resulting a different approach. I think users will not add their own templates (do we have any command line/ui option for this?), thus the templates is maintained by us.Why do normal user need their own template? They only need their only configurations. With this assumption, I compromised a little to make the finger(print) string more technical to help a better match, with this approach: Nokia_N7210 will never match to LG_N7210c; Nokia_N700 match with Nokia_N7000 will have less score than Nokia_N700 match with Nokia_N701 I consider these features useful. ------- Comment #17 From Chen Congwu 2010-01-13 23:38:25 PST (-) [reply] ------- (In reply to comment #16) I have just updated the patch for your comments expect below. Patrick, what do you think about this? If you are happy with my approach, you may have a look and merge now. ------- Comment #18 From pohly 2010-01-14 06:12:52 PST (-) [reply] ------- (In reply to comment #16) > > > A useable single-file installation of SyncEvolution. I consider this a valuable > > feature. > Okay, I see. > > > > > What if <Manufacturer> or <Model> contain commas and/or underscores? > > > Not considered so far. If there is really such strange manufacture or model, we > > > can fix that later. > > > > Once we role out 1.0 final and users start adding template files for their own > > phones, they don't have the option of modifying the source code of the > > syncevo-dbus-server or the sync-ui. > > > > The question of parsing these strings is only relevant if we reliably need to > > decompose the strings into <manufacturer> and <model>. This might not be > > necessary at all. A fuzzy string match might do just fine. For a hierarchical > > view of templates, we could split at _ (*any* underscore, regardless how many > > there are) and treat the individual words as "most general" to "most specific". > > That would also work for future extensions, like Nokia_N7120c_v1, > > Nokia_N7120c_v2. > I was thinking something different thus resulting a different approach. I think > users will not add their own templates (do we have any command line/ui option > for this?), thus the templates is maintained by us.Why do normal user need > their own template? They only need their only configurations. True for users. I was thinking of distributors who might want to add additional templates. Not at all? What if Company Foo and Bar both sell the identical device XYZ and we only have a template for Foo_XYZ? I have no idea how well this matching will work in practice. Some real examples: ./syncevolution --template ?Nokia_N7210c Available configuration templates: Nokia_7210c = Nokia 7210c phone 4 Nokia = Default templates for nokia phone 2 ServerDefault = server side default template 1 SyncEvolutionServer = SyncEvolution server side template 1 ./syncevolution --template ?7210c Available configuration templates: Nokia = Default templates for nokia phone 1 Nokia_7210c = Nokia 7210c phone 1 ServerDefault = server side default template 1 SyncEvolutionServer = SyncEvolution server side template 1 IMHO the Nokia_7210c template should get a higher score in the second case. Overall the whole matching code seems way to complicated to me. A "longest common subsequence" match and a score defined as "length of subsequence"/"length of fingerprint in template" might provide more intuitive results. I have an implementation of LCS lying around somewhere. Ping me if you want it. Congwu, can you add the real templates together with this patch and add unit tests for the code to the Cmdline tests? This is a lot of complex code that at least I don't understand just by reading it. This is useful regardless how we really implement the matching. To run with a controlled set of file templates you could copy those as we do for PIM data and then run the tests with a ScopedEnvChange templates("SYNCEVOLUTION_TEMPLATE_DIR", "testcases/templates"). Regarding naming of the top-level src/templates/[client|server] directories: this is ambiguous. Is a "server template" something to be used *by* a SyncML client or by a SyncML server *for* a client? Traditionally, SyncEvolution had "server templates" which were used by a client. Your patch reverses the meaning by putting the Funambol/ScheduleWorld templates into a "client" directory. Perhaps we can avoid the ambiguity by using "clients" and "servers" (plural)? "servers" makes it more obvious that the templates are describing servers, to be used by one client. Same the other way around for one server supporting multiple clients. (In reply to comment #17) For general comments, see above. Regarding the code, here's a diff for changes that I would find useful: diff --git a/README b/README index c80fd34..7de4391 100644 --- a/README +++ b/README @@ -320,7 +320,7 @@ a list of valid values. sources, so if you want to change a source property of just one specific sync source, then use "--configure --source-property ... <server> <source>". ---template|-l <server name>|default|? +--template|-l <server name>|default|?|?<device> Can be used to select from one of the built-in default configurations for known SyncML servers. Defaults to the <server> name, so --template only has to be specified when creating multiple different configurations @@ -330,6 +330,13 @@ a list of valid values. Each template contains a pseudo-random device ID. Therefore setting the "deviceId" sync property is only necessary when manually recreating a configuration or when a more descriptive name is desired. + The available templates for different known SyncML servers are listed + when using a single question mark instead of a template name. When using + the ?<device> format, a fuzzy search for a template that might be suitable + for talking to such a device is done. The matching works best when using + <device> = <Manufacturer>_<Model>. The output in this mode gives the + template name followed by a short description and a rating how well the + template matches the device (higher is better). --status|-t The changes made to local data since the last synchronization are diff --git a/src/syncevo-dbus-server.cpp b/src/syncevo-dbus-server.cpp index 24ac449..aeddc21 100644 --- a/src/syncevo-dbus-server.cpp +++ b/src/syncevo-dbus-server.cpp @@ -2657,8 +2657,8 @@ void Connection::process(const Caller_t &caller, SyncConfig::ConfigList servers = SyncConfig::getConfigs(); BOOST_FOREACH(const SyncConfig::ConfigList::value_type &server, servers) { - SyncContext context(server.first); - if (context.getSyncURL() == serverID) { + SyncConfig config(server.first); + if (config.getSyncURL() == serverID) { config = server.first; break; } @@ -2672,9 +2672,12 @@ void Connection::process(const Caller_t &caller, string btAddr = id->second.substr(0, id->second.find("+")); BOOST_FOREACH(const SyncConfig::ConfigList::value_type &server, servers) { - SyncContext context(server.first); - string url = context.getSyncURL(); - SE_LOG_INFO (NULL, NULL, "matching against %s",url.c_str()); + SyncConfig config(server.first); + string url = config.getSyncURL(); + SE_LOG_DEBUG(NULL, NULL, "matching against %s",url.c_str()); + // this will not match: + // syncURL = obex-bt://mac-address, http://some.ip.address:port + // syncURL = http://mac-address+channel if (url.find ("obex-bt://") ==0 && url.substr(strlen("obex-bt://"), url.npos) == btAddr) { config = server.first; break; diff --git a/src/syncevo/SyncConfig.cpp b/src/syncevo/SyncConfig.cpp index 9c3a795..342056b 100644 --- a/src/syncevo/SyncConfig.cpp +++ b/src/syncevo/SyncConfig.cpp @@ -386,23 +386,33 @@ SyncConfig::TemplateList SyncConfig::getBuiltInTemplates() result.addDefaultTemplate("Google", "http://m.google.com/sync"); result.addDefaultTemplate("ZYB", "http://www.zyb.com"); result.addDefaultTemplate("Mobical", "http://www.mobical.net"); + // Why is this called SyncEvolution*Client*? + // It is the config for a SyncEvolution server and thus should be + // name "SyncEvolution" to be consistent with the other template names. result.addDefaultTemplate("SyncEvolutionClient", "http://www.syncevolution.org"); result.sort (TemplateDescription::compare_op); return result; } -SyncConfig::TemplateList SyncConfig::matchPeerTemplates(const DeviceList &peers) +static string SyncEvolutionTemplateDir() { - TemplateList result; - // match against all possible templates without any assumption on directory - // layout, the match is entirely based on the metadata .template.ini string templateDir(TEMPLATE_DIR); const char *envvar = getenv("SYNCEVOLUTION_TEMPLATE_DIR"); if (envvar) { templateDir = envvar; } + return templateDir; +} + +SyncConfig::TemplateList SyncConfig::matchPeerTemplates(const DeviceList &peers) +{ + TemplateList result; + // match against all possible templates without any assumption on directory + // layout, the match is entirely based on the metadata .template.ini + string templateDir(SyncEvolutionTemplateDir()); + std::queue <std::string, std::list<std::string> > directories; if (isDir(templateDir)) { directories.push (templateDir); @@ -447,11 +457,7 @@ boost::shared_ptr<SyncConfig> SyncConfig::createPeerTemplate(const string &serve } // case insensitive search for read-only file template config - string templateConfig(TEMPLATE_DIR); - const char *envvar = getenv("SYNCEVOLUTION_TEMPLATE_DIR"); - if (envvar) { - templateConfig = envvar; - } + string templateConfig(SyncEvolutionTemplateDir()); // before starting another fuzzy match process, first try to load the // template directly taking the parameter as the path @@ -683,7 +689,8 @@ boost::shared_ptr<SyncConfig> SyncConfig::createPeerTemplate(const string &serve } else if (boost::iequals(server, "syncevolutionclient")) { config->setSyncURL("http://yourserver:port"); config->setWebURL("http://www.syncevolution.org"); - config->setConsumerReady(true); + // No normal consumer will run a SyncEvolution HTTP server. + // config->setConsumerReady(true); source = config->getSyncSourceConfig("addressbook"); source->setURI("addressbook"); source = config->getSyncSourceConfig("calendar"); @@ -1997,7 +2004,7 @@ int TemplateConfig::serverModeMatch (SyncConfig::MatchMode mode) return BEST_MATCH; } -/* +/** * fingerprint = Manufacture_Mod * Mod = ModPre([a-z]*)ModMid([0-9]*)ModLast([a-z]*) * 7210c = "" 7210 c diff --git a/src/syncevo/SyncConfig.h b/src/syncevo/SyncConfig.h index 9af8742..fb23b67 100644 --- a/src/syncevo/SyncConfig.h +++ b/src/syncevo/SyncConfig.h @@ -1588,9 +1588,11 @@ class PersistentSyncSourceConfig : public SyncSourceConfig { virtual const char* getSupportedTypes() const { return ""; } }; -/* +/** * Representing a template configuration node - * */ + * + * PO: what is a template configuration node? + */ class TemplateConfig { boost::shared_ptr<FileConfigNode> m_metaNode; ------- Comment #19 From Chen Congwu 2010-01-15 00:20:05 PST (-) [reply] ------- (In reply to comment #18) I was inaccurate. It will show up with a low matching level so that users can select it by themselves via UI. Eventualy, we will update the fingerprint for Foo_XYZ to also cover Bar_XYZ. I thought about this before, it was because "7210c" is ambiguous to the matching, I took this as manufacture field by default. If you don't know the manufacture, you can use ./syncevolution -l \?_7210c Available configuration templates: Nokia_7210c = Nokia 7210c phone 4 Funambol = http://my.funambol.com 2 Nokia = Default templates for nokia phone 2 ScheduleWorld = http://sync.scheduleworld.com 2 ServerDefault = server side default template 2 SyncEvolutionServer = SyncEvolution server side template 2 You might be right.. LCS might be a good simple matching while this complicated solution provides improvement possibilities. There are several parameters you can tune with to come out with a better matching if we later found useful. What do you mean "real templates" here? Why do I need to add them? I agree most except blow: How can we match with HTTP based SyncURLs? Is http://mac-address+channel has any meaning? How can we match the mac address against an ip address for this case: http://some.ip.address:port ------- Comment #20 From pohly 2010-01-15 01:52:02 PST (-) [reply] ------- (In reply to comment #19) You are very optimistic about our abilities to keep the database updated ;-) I'm a bit more pessimistic, based on my experience with maintaining a similar database of CD writers in MakeCD. I therefore think that the code should provide a better score for Bar_XYZ already when only Foo_XYZ is listed. I think the code should handle this, not the user. Where do you see an advantage of the "intelligent" matching compared to a "dumb" LCS? Perhaps we can do some thought experiments to compare the two approaches without actually implementing them. I mean the src/templates that were in a later patch. Add them in the same patch that also adds the code and you can implement unit testing also in that patch. We can't. My point was that we wanted to turn syncURL into a list of urls for peers which can be contacted via different transports. This new code should cope with such a config by extracting the obex+bt:// urls and comparing against those. Probably we need some general-purpose utility code in SyncConfig to return "syncURL" not as a plain string, but a list of urls. -- Configure bugmail: http://bugs.meego.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching the assignee of the bug. You are watching someone on the CC list of the bug.