From b8f0bd20fd0aa6dee44ea49da3bb8fb8045b6714 Mon Sep 17 00:00:00 2001
From: Evan Laird <evanlaird@google.com>
Date: Tue, 12 Oct 2021 15:08:33 -0400
Subject: [PATCH] Remove instance fields from MobileSignalController

MobileSignalController (MSC) was storing a few fields as instance
members on itself, and thus would lose that historical information and
make it so logs could not give the full picture of what the state was in
the past

This change makes a couple of changes to make things simpler:

1. Move all telephony data onto the MobileState object, making it so the
   full decision-making state object is stored at every historical level
2. Attach MobileState and MobileStatus directly so MobileState reads its
   fields directly from the update
3. Log only the used fields from ServiceState and SignalStrength

Things are in a much better state here, but the next step should be to
formalize the difference between the fields that get set directly from
MobileStatus, and the ones that are updated in MSC#updateTelephony

Test: atest SystemUITests
Bug: 197851948
Change-Id: Ia35a13ee36b8e41b9ca86506de56f98de4891006
---
 .../connectivity/MobileSignalController.java  | 105 ++++++---------
 .../statusbar/connectivity/MobileState.kt     | 125 +++++++++++++++---
 2 files changed, 150 insertions(+), 80 deletions(-)

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileSignalController.java b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileSignalController.java
index ddb3efe66a800..9ae7ea2bdded9 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileSignalController.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileSignalController.java
@@ -33,7 +33,6 @@ import android.telephony.ServiceState;
 import android.telephony.SignalStrength;
 import android.telephony.SubscriptionInfo;
 import android.telephony.SubscriptionManager;
-import android.telephony.TelephonyDisplayInfo;
 import android.telephony.TelephonyManager;
 import android.telephony.ims.ImsException;
 import android.telephony.ims.ImsMmTelManager;
@@ -47,7 +46,6 @@ import android.util.Log;
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.settingslib.AccessibilityContentDescriptions;
 import com.android.settingslib.SignalIcon.MobileIconGroup;
-import com.android.settingslib.Utils;
 import com.android.settingslib.graph.SignalDrawable;
 import com.android.settingslib.mobile.MobileMappings.Config;
 import com.android.settingslib.mobile.MobileStatusTracker;
@@ -89,15 +87,6 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
     final SubscriptionInfo mSubscriptionInfo;
     private Map<String, MobileIconGroup> mNetworkToIconLookup;
 
-    // Since some pieces of the phone state are interdependent we store it locally,
-    // this could potentially become part of MobileState for simplification/complication
-    // of code.
-    private int mDataState = TelephonyManager.DATA_DISCONNECTED;
-    private TelephonyDisplayInfo mTelephonyDisplayInfo =
-            new TelephonyDisplayInfo(TelephonyManager.NETWORK_TYPE_UNKNOWN,
-                    TelephonyDisplayInfo.OVERRIDE_NETWORK_TYPE_NONE);
-    private ServiceState mServiceState;
-    private SignalStrength mSignalStrength;
     private int mLastLevel;
     private MobileIconGroup mDefaultIcons;
     private Config mConfig;
@@ -464,16 +453,8 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
         return new MobileState();
     }
 
-    private boolean isCdma() {
-        return (mSignalStrength != null) && !mSignalStrength.isGsm();
-    }
-
-    public boolean isEmergencyOnly() {
-        return (mServiceState != null && mServiceState.isEmergencyOnly());
-    }
-
     public boolean isInService() {
-        return Utils.isInService(mServiceState);
+        return mCurrentState.isInService();
     }
 
     String getNetworkNameForCarrierWiFi() {
@@ -481,15 +462,15 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
     }
 
     private boolean isRoaming() {
-        // During a carrier change, roaming indications need to be supressed.
+        // During a carrier change, roaming indications need to be suppressed.
         if (isCarrierNetworkChangeActive()) {
             return false;
         }
-        if (isCdma()) {
+        if (mCurrentState.isCdma()) {
             return mPhone.getCdmaEnhancedRoamingIndicatorDisplayNumber()
                     != TelephonyManager.ERI_OFF;
         } else {
-            return mServiceState != null && mServiceState.getRoaming();
+            return mCurrentState.isRoaming();
         }
     }
 
@@ -581,27 +562,29 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
     }
 
     private void updateMobileStatus(MobileStatus mobileStatus) {
-        mCurrentState.activityIn = mobileStatus.activityIn;
-        mCurrentState.activityOut = mobileStatus.activityOut;
-        mCurrentState.dataSim = mobileStatus.dataSim;
-        mCurrentState.carrierNetworkChangeMode = mobileStatus.carrierNetworkChangeMode;
-        mDataState = mobileStatus.dataState;
+        int lastVoiceState = mCurrentState.getVoiceServiceState();
+        mCurrentState.setFromMobileStatus(mobileStatus);
+
         notifyMobileLevelChangeIfNecessary(mobileStatus.signalStrength);
-        mSignalStrength = mobileStatus.signalStrength;
-        mTelephonyDisplayInfo = mobileStatus.telephonyDisplayInfo;
-        int lastVoiceState = mServiceState != null ? mServiceState.getState() : -1;
-        mServiceState = mobileStatus.serviceState;
-        int currentVoiceState = mServiceState != null ? mServiceState.getState() : -1;
+        if (mProviderModelBehavior) {
+            maybeNotifyCallStateChanged(lastVoiceState);
+        }
+    }
+
+    /** Call state changed is only applicable when provider model behavior is true */
+    private void maybeNotifyCallStateChanged(int lastVoiceState) {
+        int currentVoiceState = mCurrentState.getVoiceServiceState();
+        if (lastVoiceState == currentVoiceState) {
+            return;
+        }
         // Only update the no calling Status in the below scenarios
         // 1. The first valid voice state has been received
         // 2. The voice state has been changed and either the last or current state is
         //    ServiceState.STATE_IN_SERVICE
-        if (mProviderModelBehavior
-                && lastVoiceState != currentVoiceState
-                && (lastVoiceState == -1
-                        || (lastVoiceState == ServiceState.STATE_IN_SERVICE
-                                || currentVoiceState == ServiceState.STATE_IN_SERVICE))) {
-            boolean isNoCalling = currentVoiceState != ServiceState.STATE_IN_SERVICE;
+        if (lastVoiceState == -1
+                || (lastVoiceState == ServiceState.STATE_IN_SERVICE
+                        || currentVoiceState == ServiceState.STATE_IN_SERVICE)) {
+            boolean isNoCalling = mCurrentState.isNoCalling();
             isNoCalling &= !hideNoCalling();
             IconState statusIcon = new IconState(isNoCalling,
                     R.drawable.ic_qs_no_calling_sms,
@@ -611,7 +594,7 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
     }
 
     void updateNoCallingState() {
-        int currentVoiceState = mServiceState != null ? mServiceState.getState() : -1;
+        int currentVoiceState = mCurrentState.getVoiceServiceState();
         boolean isNoCalling = currentVoiceState != ServiceState.STATE_IN_SERVICE;
         isNoCalling &= !hideNoCalling();
         IconState statusIcon = new IconState(isNoCalling,
@@ -639,8 +622,7 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
     }
 
     void refreshCallIndicator(SignalCallback callback) {
-        boolean isNoCalling = mServiceState != null
-                && mServiceState.getState() != ServiceState.STATE_IN_SERVICE;
+        boolean isNoCalling = mCurrentState.isNoCalling();
         isNoCalling &= !hideNoCalling();
         IconState statusIcon = new IconState(isNoCalling,
                 R.drawable.ic_qs_no_calling_sms,
@@ -732,30 +714,30 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
     }
 
     /**
-     * Updates the current state based on mServiceState, mSignalStrength, mDataState,
-     * mTelephonyDisplayInfo, and mSimState.  It should be called any time one of these is updated.
+     * Updates the current state based on ServiceState, SignalStrength, DataState,
+     * TelephonyDisplayInfo, and sim state.  It should be called any time one of these is updated.
      * This will call listeners if necessary.
      */
     private void updateTelephony() {
         if (Log.isLoggable(mTag, Log.DEBUG)) {
             Log.d(mTag, "updateTelephonySignalStrength: hasService="
-                    + Utils.isInService(mServiceState) + " ss=" + mSignalStrength
-                    + " displayInfo=" + mTelephonyDisplayInfo);
+                    + mCurrentState.isInService()
+                    + " ss=" + mCurrentState.signalStrength
+                    + " displayInfo=" + mCurrentState.telephonyDisplayInfo);
         }
         checkDefaultData();
-        mCurrentState.connected = Utils.isInService(mServiceState) && mSignalStrength != null;
+        mCurrentState.connected = mCurrentState.isInService();
         if (mCurrentState.connected) {
-            mCurrentState.level = getSignalLevel(mSignalStrength);
+            mCurrentState.level = getSignalLevel(mCurrentState.signalStrength);
         }
 
-        String iconKey = getIconKey(mTelephonyDisplayInfo);
+        String iconKey = getIconKey(mCurrentState.telephonyDisplayInfo);
         if (mNetworkToIconLookup.get(iconKey) != null) {
             mCurrentState.iconGroup = mNetworkToIconLookup.get(iconKey);
         } else {
             mCurrentState.iconGroup = mDefaultIcons;
         }
-        mCurrentState.dataConnected = mCurrentState.connected
-                && mDataState == TelephonyManager.DATA_CONNECTED;
+        mCurrentState.dataConnected = mCurrentState.isDataConnected();
 
         mCurrentState.roaming = isRoaming();
         if (isCarrierNetworkChangeActive()) {
@@ -767,20 +749,20 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
                 mCurrentState.iconGroup = TelephonyIcons.DATA_DISABLED;
             }
         }
-        if (isEmergencyOnly() != mCurrentState.isEmergency) {
-            mCurrentState.isEmergency = isEmergencyOnly();
+        if (mCurrentState.isEmergencyOnly() != mCurrentState.isEmergency) {
+            mCurrentState.isEmergency = mCurrentState.isEmergencyOnly();
             mNetworkController.recalculateEmergency();
         }
         // Fill in the network name if we think we have it.
-        if (mCurrentState.networkName.equals(mNetworkNameDefault) && mServiceState != null
-                && !TextUtils.isEmpty(mServiceState.getOperatorAlphaShort())) {
-            mCurrentState.networkName = mServiceState.getOperatorAlphaShort();
+        if (mCurrentState.networkName.equals(mNetworkNameDefault)
+                && !TextUtils.isEmpty(mCurrentState.getOperatorAlphaShort())) {
+            mCurrentState.networkName = mCurrentState.getOperatorAlphaShort();
         }
         // If this is the data subscription, update the currentState data name
-        if (mCurrentState.networkNameData.equals(mNetworkNameDefault) && mServiceState != null
+        if (mCurrentState.networkNameData.equals(mNetworkNameDefault)
                 && mCurrentState.dataSim
-                && !TextUtils.isEmpty(mServiceState.getOperatorAlphaShort())) {
-            mCurrentState.networkNameData = mServiceState.getOperatorAlphaShort();
+                && !TextUtils.isEmpty(mCurrentState.getOperatorAlphaShort())) {
+            mCurrentState.networkNameData = mCurrentState.getOperatorAlphaShort();
         }
 
         notifyListenersIfNecessary();
@@ -832,10 +814,6 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
         pw.println("  mSubscription=" + mSubscriptionInfo + ",");
         pw.println("  mProviderModelSetting=" + mProviderModelSetting + ",");
         pw.println("  mProviderModelBehavior=" + mProviderModelBehavior + ",");
-        pw.println("  mServiceState=" + mServiceState + ",");
-        pw.println("  mSignalStrength=" + mSignalStrength + ",");
-        pw.println("  mTelephonyDisplayInfo=" + mTelephonyDisplayInfo + ",");
-        pw.println("  mDataState=" + mDataState + ",");
         pw.println("  mInflateSignalStrengths=" + mInflateSignalStrengths + ",");
         pw.println("  isDataDisabled=" + isDataDisabled() + ",");
         pw.println("  mNetworkToIconLookup=" + mNetworkToIconLookup + ",");
@@ -880,5 +858,4 @@ public class MobileSignalController extends SignalController<MobileState, Mobile
             icon = iconState;
         }
     }
-
 }
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileState.kt b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileState.kt
index e8d053010e7fc..8a3b00662900b 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileState.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/MobileState.kt
@@ -16,6 +16,12 @@
 
 package com.android.systemui.statusbar.connectivity
 
+import android.telephony.ServiceState
+import android.telephony.SignalStrength
+import android.telephony.TelephonyDisplayInfo
+import android.telephony.TelephonyManager
+import com.android.settingslib.Utils
+import com.android.settingslib.mobile.MobileStatusTracker.MobileStatus
 import com.android.settingslib.mobile.TelephonyIcons
 import java.lang.IllegalArgumentException
 
@@ -33,14 +39,20 @@ internal class MobileState(
     @JvmField var isDefault: Boolean = false,
     @JvmField var userSetup: Boolean = false,
     @JvmField var roaming: Boolean = false,
+    @JvmField var dataState: Int = TelephonyManager.DATA_DISCONNECTED,
     // Tracks the on/off state of the defaultDataSubscription
     @JvmField var defaultDataOff: Boolean = false
 ) : ConnectivityState() {
 
+    @JvmField var telephonyDisplayInfo = TelephonyDisplayInfo(TelephonyManager.NETWORK_TYPE_UNKNOWN,
+            TelephonyDisplayInfo.OVERRIDE_NETWORK_TYPE_NONE)
+    @JvmField var serviceState: ServiceState? = null
+    @JvmField var signalStrength: SignalStrength? = null
+
     /** @return true if this state is disabled or not default data */
     val isDataDisabledOrNotDefault: Boolean
-        get() = (iconGroup === TelephonyIcons.DATA_DISABLED
-                || iconGroup === TelephonyIcons.NOT_DEFAULT_DATA) && userSetup
+        get() = (iconGroup === TelephonyIcons.DATA_DISABLED ||
+                iconGroup === TelephonyIcons.NOT_DEFAULT_DATA) && userSetup
 
     /** @return if this state is considered to have inbound activity */
     fun hasActivityIn(): Boolean {
@@ -72,24 +84,82 @@ internal class MobileState(
         isDefault = o.isDefault
         userSetup = o.userSetup
         roaming = o.roaming
+        dataState = o.dataState
         defaultDataOff = o.defaultDataOff
+
+        telephonyDisplayInfo = o.telephonyDisplayInfo
+        serviceState = o.serviceState
+        signalStrength = o.signalStrength
+    }
+
+    fun isDataConnected(): Boolean {
+        return connected && dataState == TelephonyManager.DATA_CONNECTED
+    }
+
+    /** @return the current voice service state, or -1 if null */
+    fun getVoiceServiceState(): Int {
+        return serviceState?.state ?: -1
+    }
+
+    fun isNoCalling(): Boolean {
+        return serviceState?.state != ServiceState.STATE_IN_SERVICE
+    }
+
+    fun getOperatorAlphaShort(): String {
+        return serviceState?.operatorAlphaShort ?: ""
+    }
+
+    fun isCdma(): Boolean {
+        return signalStrength != null && !signalStrength!!.isGsm
+    }
+
+    fun isEmergencyOnly(): Boolean {
+        return serviceState != null && serviceState!!.isEmergencyOnly
+    }
+
+    fun isInService(): Boolean {
+        return Utils.isInService(serviceState)
+    }
+
+    fun isRoaming(): Boolean {
+        return serviceState != null && serviceState!!.roaming
+    }
+
+    fun setFromMobileStatus(mobileStatus: MobileStatus) {
+        activityIn = mobileStatus.activityIn
+        activityOut = mobileStatus.activityOut
+        dataSim = mobileStatus.dataSim
+        carrierNetworkChangeMode = mobileStatus.carrierNetworkChangeMode
+        dataState = mobileStatus.dataState
+        signalStrength = mobileStatus.signalStrength
+        telephonyDisplayInfo = mobileStatus.telephonyDisplayInfo
+        serviceState = mobileStatus.serviceState
     }
 
     override fun toString(builder: StringBuilder) {
-        builder.append("connected=$connected,")
-                .append(',')
-                .append("dataSim=$dataSim,")
-                .append("networkName=$networkName,")
-                .append("networkNameData=$networkNameData,")
-                .append("dataConnected=$dataConnected,")
-                .append("roaming=$roaming,")
-                .append("isDefault=$isDefault,")
-                .append("isEmergency=$isEmergency,")
-                .append("airplaneMode=$airplaneMode,")
-                .append("carrierNetworkChangeMode=$carrierNetworkChangeMode,")
-                .append("userSetup=$userSetup,")
-                .append("defaultDataOff=$defaultDataOff,")
-                .append("showQuickSettingsRatIcon=${showQuickSettingsRatIcon()}")
+        super.toString(builder)
+        builder.append(',')
+        builder.append("dataSim=$dataSim,")
+        builder.append("networkName=$networkName,")
+        builder.append("networkNameData=$networkNameData,")
+        builder.append("dataConnected=$dataConnected,")
+        builder.append("roaming=$roaming,")
+        builder.append("isDefault=$isDefault,")
+        builder.append("isEmergency=$isEmergency,")
+        builder.append("airplaneMode=$airplaneMode,")
+        builder.append("carrierNetworkChangeMode=$carrierNetworkChangeMode,")
+        builder.append("userSetup=$userSetup,")
+        builder.append("dataState=$dataState,")
+        builder.append("defaultDataOff=$defaultDataOff,")
+
+        // Computed properties
+        builder.append("showQuickSettingsRatIcon=${showQuickSettingsRatIcon()},")
+        builder.append("voiceServiceState=${getVoiceServiceState()},")
+        builder.append("isInService=${isInService()},")
+
+        builder.append("serviceState=${serviceState?.minLog() ?: "(null)"},")
+        builder.append("signalStrength=${signalStrength?.minLog() ?: "(null)"},")
+        builder.append("displayInfo=$telephonyDisplayInfo")
     }
 
     override fun equals(other: Any?): Boolean {
@@ -109,7 +179,11 @@ internal class MobileState(
         if (isDefault != other.isDefault) return false
         if (userSetup != other.userSetup) return false
         if (roaming != other.roaming) return false
+        if (dataState != other.dataState) return false
         if (defaultDataOff != other.defaultDataOff) return false
+        if (telephonyDisplayInfo != other.telephonyDisplayInfo) return false
+        if (serviceState != other.serviceState) return false
+        if (signalStrength != other.signalStrength) return false
 
         return true
     }
@@ -126,7 +200,26 @@ internal class MobileState(
         result = 31 * result + isDefault.hashCode()
         result = 31 * result + userSetup.hashCode()
         result = 31 * result + roaming.hashCode()
+        result = 31 * result + dataState
         result = 31 * result + defaultDataOff.hashCode()
+        result = 31 * result + telephonyDisplayInfo.hashCode()
+        result = 31 * result + (serviceState?.hashCode() ?: 0)
+        result = 31 * result + (signalStrength?.hashCode() ?: 0)
         return result
     }
 }
+
+/** toString() is a little more verbose than we need. Just log the fields we read */
+private fun ServiceState.minLog(): String {
+    return "serviceState={" +
+            "state=$state," +
+            "isEmergencyOnly=$isEmergencyOnly," +
+            "roaming=$roaming," +
+            "operatorNameAlphaShort=$operatorAlphaShort}"
+}
+
+private fun SignalStrength.minLog(): String {
+    return "signalStrength={" +
+            "isGsm=$isGsm," +
+            "level=$level}"
+}