Merge lp:~mir-team/mir/fix-1336548 into lp:mir

[Robert Carr]

BasicSurface::configure: Ensures result is properly initialized
(LP: #1336548)

BasicSurface::configure: Ensures result is properly initialized

See bug

[Daniel d'Andrada (dandrader)]

For consistency, shouldn't you remove the line "result = value;" from "case mir_surface_attrib_swapinterval:"?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1740
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2050/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/788
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/794
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/787
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/571
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/571/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/569
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/569/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2399
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2399/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1941
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9128

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2050/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Focus not being gettable was somewhat intentional in that it has no use (would be redundant) if you also had a function to ask which surface has focus (which would be more useful).

But I see no harm in this new form either.

review: Approve

[Alan Griffiths (alan-griffiths)]

*Needs Discussion*

Isn't the underlying problem that focus isn't really an attribute? It isn't something that we get a client request to set that then needs validation by a configurator?

AFAICS someone simply used the attribute transport mechanism as a convenience for sending a focus event to the client without clearly separating that mechanism from the attribute setting protocol.

review: Needs Information

[Robert Carr (robertcarr)]

I don't think there is an underlying problem in this case so much as just a function that fails to uphold its contract due to developer error and lack of test.

"It isn't something that we get a client request to set that then needs validation by a configurator?"

I'm not sure this is a a good definition for a surface attribute. For things like mir_surface_attrib_state this already becomes inaccurate, i.e. the wm may cause you to become maximized and it has nothing to do with a client request.

I think of surface attributes client+server surface properties which exist on a spectrum of server/client writable.

[Daniel van Vugt (vanvugt)]

I think the fundamental problem is that MirSurfaceEvent only presently implements attributes that are stored within each surface. Maybe we can try to be more flexible in future and also support attributes that are not /part/ of the surface object, like focus should be.

In fact there is no problem with MirSurfaceEvent at all. I think it would just be nice to redesign the server side to not store/emit focus from the surface object itself. Focus is a feature of the shell.

All for another day... those discussions are related but don't need to block a change as simple as this one.

[Daniel d'Andrada (dandrader)]

It fixes the bug.

review: Approve

[Alan Griffiths (alan-griffiths)]

> All for another day... those discussions are related but don't need to block a
> change as simple as this one.

yep

review: Approve

[Alan Griffiths (alan-griffiths)]

Autolanding job was failing with...

bzrlib.errors.UnknownErrorFromSmartServer: Server sent an unexpected error: ('error', 'GhostRevisionsHaveNoRevno', 'Could not determine revno for {<email address hidden>} because its ancestry shows a ghost at {tarmac-20140616214108-3x1aupo3am5qe439}')

merged devel in the hope of resolving

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-07-04 04:43:42 +0000
+++ src/server/scene/basic_surface.cpp	2014-07-07 09:21:42 +0000
@@ -449,14 +449,13 @@
 
 int ms::BasicSurface::configure(MirSurfaceAttrib attrib, int value)
 {
-    int result = 0;
     bool allow_dropping = false;
 
     /*
      * TODO: In future, query the shell implementation for the subset of
      *       attributes/types it implements.
      */
-    value = configurator->select_attribute_value(*this, attrib, value);
+    int result = value = configurator->select_attribute_value(*this, attrib, value);
     switch (attrib)
     {
     case mir_surface_attrib_type:

=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp	2014-06-27 13:40:34 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp	2014-07-07 09:21:42 +0000
@@ -510,6 +510,38 @@
     }, std::logic_error);
 }
 
+TEST_F(BasicSurfaceTest, configure_returns_value_set_by_configurator)
+{
+    using namespace testing;
+    
+    struct FocusSwappingConfigurator : public StubSurfaceConfigurator
+    {
+        int select_attribute_value(ms::Surface const&, MirSurfaceAttrib attrib, int value) override 
+        {
+            if (attrib != mir_surface_attrib_focus)
+                return value;
+            else if (value == mir_surface_focused)
+                return mir_surface_unfocused;
+            else
+                return mir_surface_focused;
+        }
+    };
+
+    ms::BasicSurface surface{
+        name,
+        rect,
+        false,
+        mock_buffer_stream,
+        std::shared_ptr<mi::InputChannel>(),
+        mt::fake_shared(mock_sender),
+        std::make_shared<FocusSwappingConfigurator>(),
+        nullptr,
+        report};
+    
+    EXPECT_EQ(mir_surface_unfocused, surface.configure(mir_surface_attrib_focus, mir_surface_focused));
+    EXPECT_EQ(mir_surface_focused, surface.configure(mir_surface_attrib_focus, mir_surface_unfocused));
+}
+
 TEST_F(BasicSurfaceTest, calls_send_event_on_consume)
 {
     using namespace ::testing;