Merge "InputMapper refactor: refactor device enable/disable method" into main

3 files changed, 34 insertions, 37 deletions

diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp
index bacc7203b6..49998a0789 100644
--- a/services/inputflinger/reader/InputDevice.cpp
+++ b/services/inputflinger/reader/InputDevice.cpp
@@ -66,23 +66,33 @@ bool InputDevice::isEnabled() {
     return enabled;
 }
 
-std::list<NotifyArgs> InputDevice::setEnabled(bool enabled, nsecs_t when) {
-    std::list<NotifyArgs> out;
-    if (enabled && mAssociatedDisplayPort && !mAssociatedViewport) {
-        ALOGW("Cannot enable input device %s because it is associated with port %" PRIu8 ", "
-              "but the corresponding viewport is not found",
-              getName().c_str(), *mAssociatedDisplayPort);
-        enabled = false;
+std::list<NotifyArgs> InputDevice::updateEnableState(nsecs_t when,
+                                                     const InputReaderConfiguration& readerConfig) {
+    // If the device was explicitly disabled by the user, it would be present in the
+    // "disabledDevices" list. This device should be disabled.
+    bool enable = readerConfig.disabledDevices.find(mId) == readerConfig.disabledDevices.end();
+
+    // If a device is associated with a specific display but there is no
+    // associated DisplayViewport, don't enable the device.
+    if (enable && (mAssociatedDisplayPort || mAssociatedDisplayUniqueId) && !mAssociatedViewport) {
+        const std::string desc = mAssociatedDisplayPort
+                ? "port " + std::to_string(*mAssociatedDisplayPort)
+                : "uniqueId " + *mAssociatedDisplayUniqueId;
+        ALOGW("Cannot enable input device %s because it is associated "
+              "with %s, but the corresponding viewport is not found",
+              getName().c_str(), desc.c_str());
+        enable = false;
     }
 
-    if (isEnabled() == enabled) {
+    std::list<NotifyArgs> out;
+    if (isEnabled() == enable) {
         return out;
     }
 
     // When resetting some devices, the driver needs to be queried to ensure that a proper reset is
     // performed. The querying must happen when the device is enabled, so we reset after enabling
     // but before disabling the device. See MultiTouchMotionAccumulator::reset for more information.
-    if (enabled) {
+    if (enable) {
         for_each_subdevice([](auto& context) { context.enableDevice(); });
         out += reset(when);
     } else {
@@ -235,15 +245,6 @@ std::list<NotifyArgs> InputDevice::configure(nsecs_t when,
             }
         }
 
-        if (changes.test(Change::ENABLED_STATE)) {
-            // Do not execute this code on the first configure, because 'setEnabled' would call
-            // InputMapper::reset, and you can't reset a mapper before it has been configured.
-            // The mappers are configured for the first time at the bottom of this function.
-            auto it = readerConfig.disabledDevices.find(mId);
-            bool enabled = it == readerConfig.disabledDevices.end();
-            out += setEnabled(enabled, when);
-        }
-
         if (!changes.any() || changes.test(Change::DISPLAY_INFO)) {
             // In most situations, no port or name will be specified.
             mAssociatedDisplayPort = std::nullopt;
@@ -267,12 +268,8 @@ std::list<NotifyArgs> InputDevice::configure(nsecs_t when,
                 }
             }
 
-            // If the device was explicitly disabled by the user, it would be present in the
-            // "disabledDevices" list. If it is associated with a specific display, and it was not
-            // explicitly disabled, then enable/disable the device based on whether we can find the
-            // corresponding viewport.
-            bool enabled =
-                    (readerConfig.disabledDevices.find(mId) == readerConfig.disabledDevices.end());
+            // If it is associated with a specific display, then find the corresponding viewport
+            // which will be used to enable/disable the device.
             if (mAssociatedDisplayPort) {
                 mAssociatedViewport =
                         readerConfig.getDisplayViewportByPort(*mAssociatedDisplayPort);
@@ -280,7 +277,6 @@ std::list<NotifyArgs> InputDevice::configure(nsecs_t when,
                     ALOGW("Input device %s should be associated with display on port %" PRIu8 ", "
                           "but the corresponding viewport is not found.",
                           getName().c_str(), *mAssociatedDisplayPort);
-                    enabled = false;
                 }
             } else if (mAssociatedDisplayUniqueId != std::nullopt) {
                 mAssociatedViewport =
@@ -289,16 +285,17 @@ std::list<NotifyArgs> InputDevice::configure(nsecs_t when,
                     ALOGW("Input device %s should be associated with display %s but the "
                           "corresponding viewport cannot be found",
                           getName().c_str(), mAssociatedDisplayUniqueId->c_str());
-                    enabled = false;
                 }
             }
+        }
 
-            if (changes.any()) {
-                // For first-time configuration, only allow device to be disabled after mappers have
-                // finished configuring. This is because we need to read some of the properties from
-                // the device's open fd.
-                out += setEnabled(enabled, when);
-            }
+        if (changes.test(Change::ENABLED_STATE) || changes.test(Change::DISPLAY_INFO)) {
+            // Whether a device is enabled can depend on the display association,
+            // so update the enabled state when there is a change in display info.
+            // NOTE: The first configuration of a mapper must happen with the device enabled.
+            // Do not execute this code on the first configure to prevent mappers
+            // from being configured with the device disabled.
+            out += updateEnableState(when, readerConfig);
         }
 
         for_each_mapper([this, when, &readerConfig, changes, &out](InputMapper& mapper) {
@@ -309,9 +306,7 @@ std::list<NotifyArgs> InputDevice::configure(nsecs_t when,
         // If a device is just plugged but it might be disabled, we need to update some info like
         // axis range of touch from each InputMapper first, then disable it.
         if (!changes.any()) {
-            out += setEnabled(readerConfig.disabledDevices.find(mId) ==
-                                      readerConfig.disabledDevices.end(),
-                              when);
+            out += updateEnableState(when, readerConfig);
         }
     }
     return out;
diff --git a/services/inputflinger/reader/include/InputDevice.h b/services/inputflinger/reader/include/InputDevice.h
index 1cbcbf47b4..ec37cdcf02 100644
--- a/services/inputflinger/reader/include/InputDevice.h
+++ b/services/inputflinger/reader/include/InputDevice.h
@@ -77,7 +77,6 @@ public:
     inline bool isIgnored() { return !getMapperCount() && !mController; }
 
     bool isEnabled();
-    [[nodiscard]] std::list<NotifyArgs> setEnabled(bool enabled, nsecs_t when);
 
     void dump(std::string& dump, const std::string& eventHubDevStr);
     void addEmptyEventHubDevice(int32_t eventHubId);
@@ -206,6 +205,9 @@ private:
     std::vector<std::unique_ptr<InputMapper>> createMappers(
             InputDeviceContext& contextPtr, const InputReaderConfiguration& readerConfig);
 
+    [[nodiscard]] std::list<NotifyArgs> updateEnableState(
+            nsecs_t when, const InputReaderConfiguration& readerConfig);
+
     PropertyMap mConfiguration;
 
     // Runs logic post a `process` call. This can be used to update the generated `NotifyArgs` as
diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp
index 9dda0a82c4..9d2aea215a 100644
--- a/services/inputflinger/tests/InputReader_test.cpp
+++ b/services/inputflinger/tests/InputReader_test.cpp
@@ -2728,7 +2728,7 @@ TEST_F(InputDeviceTest, NotWakeDevice_DoesNotRemoveExistingWakeFlagFromProcessNo
 
 // A single input device is associated with a specific display. Check that:
 // 1. Device is disabled if the viewport corresponding to the associated display is not found
-// 2. Device is disabled when setEnabled API is called
+// 2. Device is disabled when configure API is called
 TEST_F(InputDeviceTest, Configure_AssignsDisplayPort) {
     mDevice->addMapper<FakeInputMapper>(EVENTHUB_ID, mFakePolicy->getReaderConfiguration(),
                                         AINPUT_SOURCE_TOUCHSCREEN);