From 2381c773b9f42af4c9722f231526d3fdfc8cb108 Mon Sep 17 00:00:00 2001 From: Marja Hassinen Date: Tue, 7 Dec 2010 11:59:21 +0200 Subject: provider: "Multiple subscribe" and "unsubscribe without subscribe" are no longer errors. Sending these messages does no actual harm; the intention of the client is clear. "Multiple subscribe" can actually happen when the provider is not running when the subscriber starts. Contextkit recovers from that situation, and treating "multiple subscribe" as an error provides little value. --- .../customer-tests/service/servicetest.cpp | 13 ++++++++++++- .../subscription/subscriptiontests.cpp | 21 ++++++++++++--------- libcontextprovider/src/propertyadaptor.cpp | 22 +++++++++++----------- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/libcontextprovider/customer-tests/service/servicetest.cpp b/libcontextprovider/customer-tests/service/servicetest.cpp index c27b80ac..378f6bbf 100644 --- a/libcontextprovider/customer-tests/service/servicetest.cpp +++ b/libcontextprovider/customer-tests/service/servicetest.cpp @@ -179,14 +179,25 @@ void ServiceTest::multiStart() QString expected = "Subscribe returned: Unknown"; QCOMPARE(actual.simplified(), expected.simplified()); + // Start listening to signals (we already have one subscriber) + QSignalSpy firstSpy(property, + SIGNAL(firstSubscriberAppeared(const QString&))); + QSignalSpy lastSpy(property, + SIGNAL(firstSubscriberAppeared(const QString&))); + // Test: start the service again (even though it's started) service->start(); // Expected result: the service is still there, and remembers the client actual = writeToClient("subscribe service Test.Property\n"); - expected = "Subscribe error: org.maemo.contextkit.Error.MultipleSubscribe"; + expected = "Subscribe returned: Unknown"; QCOMPARE(actual.simplified(), expected.simplified()); + // (so we don't get a signal) + QTest::qWait(1000); + QCOMPARE(firstSpy.count(), 0); + QCOMPARE(lastSpy.count(), 0); + delete service; delete property; } diff --git a/libcontextprovider/customer-tests/subscription/subscriptiontests.cpp b/libcontextprovider/customer-tests/subscription/subscriptiontests.cpp index 335f604e..e610524b 100644 --- a/libcontextprovider/customer-tests/subscription/subscriptiontests.cpp +++ b/libcontextprovider/customer-tests/subscription/subscriptiontests.cpp @@ -258,16 +258,19 @@ void SubscriptionTests::multiSubscribe() // Doing this only in init() is not enough; doesn't stop the test case. QVERIFY(clientStarted); + test_int->setValue(567); + // Ask the client to call Subscribe for a property twice. The - // property exists and is currently unknown since we haven't set a - // value for it. - writeToClient("subscribe service1 Test.Int\n"); + // property exists and has a value. + QString actual1 = writeToClient("subscribe service1 Test.Int\n"); - QString actual = writeToClient("subscribe service1 Test.Int\n"); + QString actual2 = writeToClient("subscribe service1 Test.Int\n"); - // Expected result: Unsubscribe returns a D-Bus error - QString expected("Subscribe error: org.maemo.contextkit.Error.MultipleSubscribe"); - QCOMPARE(actual.simplified(), expected.simplified()); + // Expected result: both Subscribes return the same value. + QString expected("Subscribe returned: int:567"); + + QCOMPARE(actual1.simplified(), expected.simplified()); + QCOMPARE(actual2.simplified(), expected.simplified()); } void SubscriptionTests::illegalUnsubscribe() @@ -281,8 +284,8 @@ void SubscriptionTests::illegalUnsubscribe() // since we haven't set a value for it. QString actual = writeToClient("unsubscribe service1 Test.Int\n"); - // Expected result: Unsubscribe returns a D-Bus error - QString expected("Unsubscribe error: org.maemo.contextkit.Error.IllegalUnsubscribe"); + // Expected result: Unsubscribe succeeds. + QString expected("Unsubscribe called"); QCOMPARE(actual.simplified(), expected.simplified()); } diff --git a/libcontextprovider/src/propertyadaptor.cpp b/libcontextprovider/src/propertyadaptor.cpp index f82b3055..e90a44d4 100644 --- a/libcontextprovider/src/propertyadaptor.cpp +++ b/libcontextprovider/src/propertyadaptor.cpp @@ -65,8 +65,8 @@ void PropertyAdaptor::Subscribe(const QDBusMessage &msg, QVariantList& values, q { contextDebug() << "Subscribe called"; - // Store the information of the subscription. For each client, we - // record how many times the client has subscribed. + // Store the information of the subscription. For each property, we record + // which clients have subscribed. QString client = msg.service(); if (clientServiceNames.contains(client) == false) { @@ -77,11 +77,12 @@ void PropertyAdaptor::Subscribe(const QDBusMessage &msg, QVariantList& values, q serviceWatcher.addWatchedService(client); } else { - contextWarning() << "Client" << client << "subscribed to property" << propertyPrivate->key << "multiple times"; - QDBusMessage error = msg.createErrorReply("org.maemo.contextkit.Error.MultipleSubscribe", - "Subscribing to " + propertyPrivate->key + " multiple times"); - connection->send(error); + contextDebug() << "Client" << client << "subscribed to property" << propertyPrivate->key << "multiple times"; } + // Don't treat the "client sends 2 Subscribe calls" as an error. When the + // provider is not running (e.g., during boot), the subscriber might send 2 + // Subscribe calls: one of them before the provider is running, and the + // other when it notices that the provider has started. // Construct the return values Get(values, timestamp); @@ -100,12 +101,11 @@ void PropertyAdaptor::Unsubscribe(const QDBusMessage &msg) serviceWatcher.removeWatchedService(client); } else { - contextWarning() << "Client" << client << "unsubscribed from property" << + contextDebug() << "Client" << client << "unsubscribed from property" << propertyPrivate->key << "without subscribing"; - QDBusMessage error = msg.createErrorReply("org.maemo.contextkit.Error.IllegalUnsubscribe", - "Unsubscribing from a non-subscribed property " - + propertyPrivate->key); - connection->send(error); + // Don't treat the "client sends Unsubscribe for a non-subscribed + // property" as an error. The intention of the client is to not be + // subscribed, so there's nothing to do. } } -- cgit v1.2.3