Merge lp:~mterry/unity-system-compositor/wake-screen-on-press into lp:unity-system-compositor

[Michael Terry]

Wake screen immediately upon a power button press, not its release.

Wake screen immediately upon a power button press, not its release.

This is a break out fix noticed in bug 1446298.

Android turns on the screen immediately upon receiving a press (and turns it off upon receiving a release like we do). This makes it feel snappier and seems like the correct thing to do (what are we waiting for anyway?).

So I've made USC turn the screen on if the display is off when the power is pressed. I've also removed a method that is no longer used (since we are never in a position where we simply toggle the screen anymore -- we always know a specific target state).

I only tested this patch on vivid's USC (not trunk's). Some files changed names and such, but the code was the same. It was faster than before (didn't wait a full long-press time), but still surprisingly non-immediate. Why would that be?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:208
http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ci/206/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-amd64-ci/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-armhf-ci/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-i386-ci/18/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-system-compositor-ci/206/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:208
http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ci/209/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-amd64-ci/46/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-armhf-ci/46/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-i386-ci/21/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-system-compositor-ci/209/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Tests need update.

[lp:~mterry/unity-system-compositor/wake-screen-on-press]

209. By Michael Terry on 2015-04-29

Fix test

210. By Michael Terry on 2015-04-29

Merge from trunk

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:210
http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ci/210/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-amd64-ci/47
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-armhf-ci/47
        deb: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-armhf-ci/47/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-vivid-i386-ci/22

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-system-compositor-ci/210/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Seems to do what's intended.

I'm not sure who's the right authority on whether it is a good idea, so not TAing.

review: Approve

[Alan Griffiths (alan-griffiths)]

Nobody seems to object.

=== modified file 'src/mir_screen.cpp'
--- src/mir_screen.cpp	2015-03-16 10:24:45 +0000
+++ src/mir_screen.cpp	2015-04-29 14:39:31 +0000
@@ -79,13 +79,10 @@
     enable_inactivity_timers_l(enable);
 }
 
-void usc::MirScreen::toggle_screen_power_mode(PowerStateChangeReason reason)
+MirPowerMode usc::MirScreen::get_screen_power_mode()
 {
     std::lock_guard<std::mutex> lock{guard};
-    MirPowerMode new_mode = (current_power_mode == MirPowerMode::mir_power_mode_on) ?
-            MirPowerMode::mir_power_mode_off : MirPowerMode::mir_power_mode_on;
-
-    set_screen_power_mode_l(new_mode, reason);
+    return current_power_mode;
 }
 
 void usc::MirScreen::set_screen_power_mode(MirPowerMode mode, PowerStateChangeReason reason)

=== modified file 'src/mir_screen.h'
--- src/mir_screen.h	2015-03-16 10:24:45 +0000
+++ src/mir_screen.h	2015-04-29 14:39:31 +0000
@@ -51,9 +51,9 @@
     ~MirScreen();
 
     void enable_inactivity_timers(bool enable) override;
-    void toggle_screen_power_mode(PowerStateChangeReason reason) override;
     void keep_display_on_temporarily() override;
 
+    MirPowerMode get_screen_power_mode() override;
     void set_screen_power_mode(MirPowerMode mode, PowerStateChangeReason reason) override;
     void keep_display_on(bool on) override;
     void set_brightness(int brightness) override;

=== modified file 'src/screen.h'
--- src/screen.h	2015-02-18 14:40:29 +0000
+++ src/screen.h	2015-04-29 14:39:31 +0000
@@ -33,9 +33,9 @@
     virtual ~Screen() = default;
 
     virtual void enable_inactivity_timers(bool enable) = 0;
-    virtual void toggle_screen_power_mode(PowerStateChangeReason reason) = 0;
     virtual void keep_display_on_temporarily() = 0;
 
+    virtual MirPowerMode get_screen_power_mode() = 0;
     virtual void set_screen_power_mode(MirPowerMode mode, PowerStateChangeReason reason) = 0;
     virtual void keep_display_on(bool on) = 0;
     virtual void set_brightness(int brightness) = 0;

=== modified file 'src/screen_event_handler.cpp'
--- src/screen_event_handler.cpp	2015-04-07 13:36:48 +0000
+++ src/screen_event_handler.cpp	2015-04-29 14:39:31 +0000
@@ -35,6 +35,7 @@
       shutdown_timeout{shutdown_timeout},
       shutdown{shutdown},
       long_press_detected{false},
+      mode_at_press_start{MirPowerMode::mir_power_mode_off},
       shutdown_alarm{alarm_factory->create_alarm([this]{ shutdown_alarm_notification(); })},
       long_press_alarm{alarm_factory->create_alarm([this]{ long_press_notification(); })}
 {
@@ -77,6 +78,14 @@
 void usc::ScreenEventHandler::power_key_down()
 {
     std::lock_guard<std::mutex> lock{guard};
+
+    mode_at_press_start = screen->get_screen_power_mode();
+    if (mode_at_press_start == MirPowerMode::mir_power_mode_off)
+    {
+        screen->set_screen_power_mode(
+            MirPowerMode::mir_power_mode_on, PowerStateChangeReason::power_key);
+    }
+
     screen->enable_inactivity_timers(false);
     long_press_detected = false;
     long_press_alarm->reschedule_in(power_key_ignore_timeout);
@@ -88,9 +97,11 @@
     std::lock_guard<std::mutex> lock{guard};
     shutdown_alarm->cancel();
     long_press_alarm->cancel();
-    if (!long_press_detected)
+
+    if (mode_at_press_start == MirPowerMode::mir_power_mode_on && !long_press_detected)
     {
-        screen->toggle_screen_power_mode(PowerStateChangeReason::power_key);
+        screen->set_screen_power_mode(
+            MirPowerMode::mir_power_mode_off, PowerStateChangeReason::power_key);
     }
 }
 
@@ -103,6 +114,9 @@
 
 void usc::ScreenEventHandler::long_press_notification()
 {
+    // We know the screen is already on after power_key_down(), but we turn the
+    // screen on here to ensure that it is also at full brightness for the
+    // presumed system power dialog that will appear.
     screen->set_screen_power_mode(
         MirPowerMode::mir_power_mode_on, PowerStateChangeReason::power_key);
     long_press_detected = true;

=== modified file 'src/screen_event_handler.h'
--- src/screen_event_handler.h	2015-04-07 13:36:48 +0000
+++ src/screen_event_handler.h	2015-04-29 14:39:31 +0000
@@ -65,6 +65,7 @@
     std::function<void()> const shutdown;
 
     std::atomic<bool> long_press_detected;
+    std::atomic<MirPowerMode> mode_at_press_start;
     std::unique_ptr<mir::time::Alarm> shutdown_alarm;
     std::unique_ptr<mir::time::Alarm> long_press_alarm;
 };

=== modified file 'tests/integration-tests/test_unity_screen_service.cpp'
--- tests/integration-tests/test_unity_screen_service.cpp	2015-03-18 13:06:10 +0000
+++ tests/integration-tests/test_unity_screen_service.cpp	2015-04-29 14:39:31 +0000
@@ -40,9 +40,9 @@
 struct MockScreen : usc::Screen
 {
     MOCK_METHOD1(enable_inactivity_timers, void(bool enable));
-    MOCK_METHOD1(toggle_screen_power_mode, void(PowerStateChangeReason reason));
     MOCK_METHOD0(keep_display_on_temporarily, void());
 
+    MOCK_METHOD0(get_screen_power_mode, MirPowerMode());
     MOCK_METHOD2(set_screen_power_mode, void(MirPowerMode mode, PowerStateChangeReason reason));
     MOCK_METHOD1(keep_display_on, void(bool on));
     MOCK_METHOD1(set_brightness, void(int brightness));

=== modified file 'tests/unit-tests/test_mir_screen.cpp'
--- tests/unit-tests/test_mir_screen.cpp	2015-03-16 10:24:45 +0000
+++ tests/unit-tests/test_mir_screen.cpp	2015-04-29 14:39:31 +0000
@@ -298,18 +298,20 @@
     timer->advance_by(power_off_timeout);
 }
 
-TEST_F(AMirScreen, toggle_screen_power_mode_from_on_to_off)
+TEST_F(AMirScreen, set_screen_power_mode_from_on_to_off)
 {
     expect_screen_is_turned_off();
-    mir_screen.toggle_screen_power_mode(PowerStateChangeReason::power_key);
+    mir_screen.set_screen_power_mode(MirPowerMode::mir_power_mode_off,
+                                     PowerStateChangeReason::power_key);
 }
 
-TEST_F(AMirScreen, toggle_screen_power_mode_from_off_to_on)
+TEST_F(AMirScreen, set_screen_power_mode_from_off_to_on)
 {
     turn_screen_off();
 
     expect_screen_is_turned_on();
-    mir_screen.toggle_screen_power_mode(PowerStateChangeReason::power_key);
+    mir_screen.set_screen_power_mode(MirPowerMode::mir_power_mode_on,
+                                     PowerStateChangeReason::power_key);
 }
 
 TEST_F(AMirScreen, sets_hardware_brightness)
@@ -359,7 +361,8 @@
     mir_screen.register_power_state_change_handler(handler);
 
     auto const toggle_reason = PowerStateChangeReason::power_key;
-    mir_screen.toggle_screen_power_mode(PowerStateChangeReason::power_key);
+    mir_screen.set_screen_power_mode(MirPowerMode::mir_power_mode_off,
+                                     PowerStateChangeReason::power_key);
 
     EXPECT_THAT(handler_reason, Eq(toggle_reason));
     EXPECT_THAT(handler_mode, Eq(MirPowerMode::mir_power_mode_off));

=== modified file 'tests/unit-tests/test_screen_event_handler.cpp'
--- tests/unit-tests/test_screen_event_handler.cpp	2015-04-07 13:36:48 +0000
+++ tests/unit-tests/test_screen_event_handler.cpp	2015-04-29 14:39:31 +0000
@@ -36,9 +36,9 @@
 struct MockScreen : usc::Screen
 {
     MOCK_METHOD1(enable_inactivity_timers, void(bool enable));
-    MOCK_METHOD1(toggle_screen_power_mode, void(PowerStateChangeReason reason));
     MOCK_METHOD0(keep_display_on_temporarily, void());
 
+    MirPowerMode get_screen_power_mode() override {return mock_mode;}
     MOCK_METHOD2(set_screen_power_mode, void(MirPowerMode mode, PowerStateChangeReason reason));
     MOCK_METHOD1(keep_display_on, void(bool on));
     MOCK_METHOD1(set_brightness, void(int brightness));
@@ -48,6 +48,8 @@
     MOCK_METHOD1(set_touch_visualization_enabled, void(bool enabled));
     MOCK_METHOD1(register_power_state_change_handler, void(
                  usc::PowerStateChangeHandler const& handler));
+
+    MirPowerMode mock_mode = MirPowerMode::mir_power_mode_off;
 };
 
 struct AScreenEventHandler : testing::Test
@@ -101,7 +103,7 @@
 
 }
 
-TEST_F(AScreenEventHandler, turns_screen_on_on_long_press)
+TEST_F(AScreenEventHandler, turns_screen_on_immediately_on_press)
 {
     auto const long_press_duration = power_key_ignore_timeout;
 
@@ -110,7 +112,6 @@
                                       PowerStateChangeReason::power_key));
 
     press_power_key();
-    timer.advance_by(long_press_duration);
 }
 
 TEST_F(AScreenEventHandler, shuts_down_system_when_power_key_pressed_for_long_enough)
@@ -158,28 +159,34 @@
     move_pointer();
 }
 
-TEST_F(AScreenEventHandler, toggles_screen_mode_on_normal_press_release)
+TEST_F(AScreenEventHandler, sets_screen_mode_off_normal_press_release)
 {
     std::chrono::milliseconds const normal_press_duration{100};
 
     EXPECT_CALL(mock_screen,
-                toggle_screen_power_mode(PowerStateChangeReason::power_key));
+                set_screen_power_mode(MirPowerMode::mir_power_mode_off,
+                                      PowerStateChangeReason::power_key));
 
+    mock_screen.mock_mode = MirPowerMode::mir_power_mode_on;
     press_power_key();
     timer.advance_by(normal_press_duration);
     release_power_key();
 }
 
-TEST_F(AScreenEventHandler, does_not_toggle_screen_mode_on_long_press_release)
+TEST_F(AScreenEventHandler, does_not_set_screen_mode_off_long_press_release)
 {
     using namespace testing;
 
     auto const long_press_duration = power_key_ignore_timeout;
 
     EXPECT_CALL(mock_screen,
-                toggle_screen_power_mode(_))
+                set_screen_power_mode(MirPowerMode::mir_power_mode_on,
+                                      PowerStateChangeReason::power_key));
+    EXPECT_CALL(mock_screen,
+                set_screen_power_mode(MirPowerMode::mir_power_mode_off, _))
         .Times(0);
 
+    mock_screen.mock_mode = MirPowerMode::mir_power_mode_on;
     press_power_key();
     timer.advance_by(long_press_duration);
     release_power_key();