On Thu, Aug 18, 2011 at 4:03 PM, Patrick Ohly <[email protected]> wrote:
> On Mo, 2011-08-15 at 15:05 +0200, Chris Kühl wrote:
>> Also, I think I'm pretty much done Bluetooth work. I've commit my
>> changes to the bluetooth-device-id branch[1] and updated the bug[2].
>
> Would it make sense to squash the development history? I don't know how
> many different commits would make sense. Right now I am only looking at
> one diff that includes everything.
>
Absolutely. I've squashed it into 2 commits; one for the server code
and another for the tests. You can find it in the
bluetooth-device-id-merge-prep branch.
> It adds a TODO:
> + if(hasPnpInfoService(uuids)) {
> + // TODO: Get the actual vendor and product ids.
> + DBusClientCall1<ServiceDict> discoverServices(*this,
> +
> "DiscoverServices");
>
> What does that mean? Is there work left to do and is it tracked in
> bugs.meego.com?
Oop. That's old. Removed now
>
> + if(pos != std::string::npos)
> + {
>
> Not my favorite bracket style, but so be it...
>
Think I've changed them all to open on the same line as the if
statement now. [Grumbles about function and if statement braces being
different. ;)]
> +static string SyncEvolutionDataDir()
> +{
> + string dataDir(DATA_DIR);
> + const char *envvar = getenv("SYNCEVOLUTION_DATA_DIR");
> + if (envvar) {
> + dataDir = envvar;
> + }
> + return dataDir;
> +}
>
> This new env variable needs to be documented in README.rst. A better place
> for this
> function might be in util.h/cpp.
>
Ok I've updated README.rst and moved it to utils.
This is just a copy of SyncEvolutionTemplateDir which is static in
SyncConfig.cpp. I'll make a separate commit to do the same with that,
too.
>
> +static bool getPnpInfoNamesFromValues(const std::string &vendorValue,
> std::string &vendorName,
> + const std::string &productValue,
> std::string &productName)
> +{
> + static GKeyFile *bt_key_vals = NULL;
> +
> + if(!bt_key_vals) {
> + bt_key_vals = g_key_file_new();
> + GError *err = NULL;
> + string filePath(SyncEvolutionDataDir() + "/bluetooth_products.ini");
>
> The syncevo-dbus-server has a feature where it watches files that make
> up the running process and restarts when any of those change. System
> daemons do that via package scripts, but for daemons started in a
> session that doesn't work. The SyncEvolution mechanism currently works
> for the executable and shared libraries, automatically found from
> scanning /proc/self/maps.
>
> Data files are loaded anew when needed (XML config files, for example).
> Doing that for every getPnpInfoNamesFromValues() might be a bit often.
> Perhaps the result can be cached in the D-Bus session instance instead
> of in a static variable?
>
> Or you could manually add that file to the watch list. See
> DBusServer::run().
>
Ah, yes. I did notice that code but had forgotten about it.
> + if(vendor)
> + vendorName = vendor;
> + else
> + // We at least need a vendor id match.
> + return false;
>
> Now *that's* the (non-)bracket style which I once declared unacceptable
> for SyncEvolution ;-) I know, I should replace the reference to the
> Funambol coding style in the HACKING document with some actual rules.
> Please, always use brackets even around single statements.
>
Sure, I should be done now.
> + /** callback of 'DiscoverServices' method. The serve detals are
> retrieved */
> + void discoverServicesCb(const ServiceDict &serviceDict, const
> std::string &error);
>
> Typo. Really minor. Consider it a feeble attempt at demonstrating that I
> have read the patch.
>
Thanks. Fixed.
> for(syncDevIt = m_syncDevices.begin(); syncDevIt != m_syncDevices.end();
> ++syncDevIt) {
> if(boost::equals(syncDevIt->m_deviceId, deviceId)) {
> device = *syncDevIt;
> + if(syncDevIt->m_pnpInformation)
> + {
>
> Switching coding style in the middle of some other code looks weird.
>
What can I say? I like variety. ;) Fixed.
> --- a/test/test-dbus.py
> +++ b/test/test-dbus.py
> @@ -53,6 +53,10 @@ monitor = ["dbus-monitor"]
> # primarily for XDG files, but also other temporary files
> xdg_root = "temp-test-dbus"
> configName = "dbus_unittest"
> +# for bluetooth tests. replace with values from your test device.
> +bt_device_mac = "D4:5D:42:73:E4:6C"
> +bt_device_fingerprint = "Nokia 5230"
> +bt_device_name = "Nokia 5230a"
>
> Replace how? By editing the script? I know that I have set a bad
> precedent by using the same approach for enabling gdb, but that is rare.
> A better mechanism for the device would be useful - command line
> parameter or env variable? Or perhaps better simulate a desktop
> environment with various paired Bluetooth devices by registering a mock
> org.bluez implementation on the D-Bus session bus and using it in
> syncevo-dbus-server when a certain env variable is set? See
> DBUS_TEST_CONNMAN for an example.
>
Ok, I see. I'll get this changed. Not in right now, though.
> There are some white space changes in test-dbus.py. I assume that that's
> cleanup work which will be a separate patch.
>
Oops. I've removed those changes. These are in my test-dbus branch anyway.
Cheers,
Chris
_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution
