Merge lp:~kaijanmaki/dbus-cpp/read-only-properties-changed-fix into lp:dbus-cpp

[Antti Kaijanmäki]

Fix read-only property PropertiesChanged updates.

When we have a read-only property which receives a PropertiesChanged signal, we must use the Super::set() as plain set() will fail on the read-only check.

[Antti Kaijanmäki (kaijanmaki)]

The test-case passes after making the change, but now that the possible exceptions are not silently discarded I'm seeing this during the test run:
[ RUN ] Service.AddingServiceAndObjectAndCallingIntoItSucceeds
void core::dbus::Property<T>::handle_changed(const core::dbus::types::Variant&) [with PropertyType = test::Service::Properties::ReadOnly]: Mismatch between expected and actual type reported by iterator:
         Expected: uint32
         Actual: invalid
void core::dbus::Property<T>::handle_changed(const core::dbus::types::Variant&) [with PropertyType = test::Service::Properties::ReadOnly]: Mismatch between expected and actual type reported by iterator:
         Expected: uint32
         Actual: invalid

That must be because of the fact that the Variant is consumed by the first caller of Variant::as<>().

But to emphasize: The test passes and everything seems to work properly even though we now get that exception caught.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:56
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kaijanmaki/dbus-cpp/read-only-properties-changed-fix/+merge/221839/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/dbus-cpp-ci/162/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-armhf-ci/2
        deb: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-armhf-ci/2/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-i386-ci/2

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dbus-cpp-ci/162/rebuild

review: Needs Fixing (continuous-integration)

[Thomas Voß (thomas-voss)]

review: Needs Fixing

[lp:~kaijanmaki/dbus-cpp/read-only-properties-changed-fix]

57. By Antti Kaijanmäki on 2014-06-10

review fixes.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:57
http://jenkins.qa.ubuntu.com/job/dbus-cpp-ci/171/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-armhf-ci/11
        deb: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-armhf-ci/11/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-i386-ci/11

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dbus-cpp-ci/171/rebuild

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

Could you please file a bug for the wrong behavior with variants. Other than that: LGTM.

review: Approve

=== modified file 'include/core/dbus/impl/property.h'
--- include/core/dbus/impl/property.h	2014-01-20 22:04:16 +0000
+++ include/core/dbus/impl/property.h	2014-06-10 08:44:31 +0000
@@ -161,10 +161,14 @@
     try
     {
         auto value = arg.as<typename PropertyType::ValueType>();
-        set(value);
+        Super::set(value);
+    }
+    catch (const std::exception &e){
+        std::cout << __PRETTY_FUNCTION__ << ": " << e.what() << std::endl;
     }
     catch (...)
     {
+        std::cout << __PRETTY_FUNCTION__ << ": " << "Unknown exception." << std::endl;
     }
 }
 }

=== modified file 'tests/service_test.cpp'
--- tests/service_test.cpp	2014-02-18 06:43:55 +0000
+++ tests/service_test.cpp	2014-06-10 08:44:31 +0000
@@ -82,11 +82,25 @@
             auto writable_property = skeleton->get_property<test::Service::Properties::Dummy>();
             writable_property->set(expected_value);
 
-            skeleton->install_method_handler<test::Service::Method>([bus, skeleton, expected_value](const dbus::Message::Ptr& msg)
+            auto readonly_property = skeleton->get_property<test::Service::Properties::ReadOnly>();
+            readonly_property->set(7);
+
+            skeleton->install_method_handler<test::Service::Method>([bus, skeleton, &readonly_property, expected_value](const dbus::Message::Ptr& msg)
             {
                 auto reply = dbus::Message::make_method_return(msg);
                 reply->writer() << expected_value;
                 bus->send(reply);
+
+                readonly_property->set(expected_value);
+                auto changed_signal = skeleton->get_signal<core::dbus::interfaces::Properties::Signals::PropertiesChanged>();
+                core::dbus::interfaces::Properties::Signals::PropertiesChanged::ArgumentType
+                        args("this.is.unlikely.to.exist.Service",
+                             {{test::Service::Properties::ReadOnly::name(),
+                               core::dbus::types::TypedVariant<test::Service::Properties::ReadOnly::ValueType>(expected_value)}},
+                             {});
+                skeleton->emit_signal<core::dbus::interfaces::Properties::Signals::PropertiesChanged, core::dbus::interfaces::Properties::Signals::PropertiesChanged::ArgumentType>(args);
+                changed_signal->emit(args);
+
                 skeleton->emit_signal<test::Service::Signals::Dummy, int64_t>(expected_value);
             });
 
@@ -118,6 +132,14 @@
             {
                 std::cout << "Dummy property changed: " << d << std::endl;
             });
+
+            auto readonly_property = stub->get_property<test::Service::Properties::ReadOnly>();
+            EXPECT_EQ(readonly_property->get(), 7);
+            std::uint32_t changed_value = 0;
+            readonly_property->changed().connect([&changed_value](std::uint32_t value){
+                changed_value = value;
+            });
+
             auto signal = stub->get_signal<test::Service::Signals::Dummy>();
             int64_t received_signal_value = -1;
             signal->connect([bus, &received_signal_value](const int32_t& value)
@@ -144,6 +166,8 @@
                 t.join();
 
             EXPECT_EQ(expected_value, received_signal_value);
+            EXPECT_EQ(expected_value, readonly_property->get());
+            EXPECT_EQ(changed_value, expected_value);
 
             return ::testing::Test::HasFailure() ? core::posix::exit::Status::failure : core::posix::exit::Status::success;
         };

=== modified file 'tests/test_service.h'
--- tests/test_service.h	2014-03-05 06:58:15 +0000
+++ tests/test_service.h	2014-06-10 08:44:31 +0000
@@ -68,6 +68,18 @@
             static const bool readable = true;
             static const bool writable = true;
         };
+
+        struct ReadOnly
+        {
+            inline static std::string name()
+            {
+                return "ReadOnly";
+            };
+            typedef Service Interface;
+            typedef std::uint32_t ValueType;
+            static const bool readable = true;
+            static const bool writable = false;
+        };
     };
 
     struct Interfaces