Cleanup CPU plugin

Summary:
Addressing code review feedback from previous 3 diffs.

* Don't return draft when using immer
* Move functions out of component to module level
* Make executeShell async and simplify callsites
* Add onStopMonitor to cleanup function
* Add uniform spacing by adding Layout.Container around titles
* Disable searchbar on both tables
* Use Toolbar

Reviewed By: mweststrate

Differential Revision: D28091565

fbshipit-source-id: 533d2491e6f48a9eaaa64b1a6cf76eea2e7189a5

desktop/plugins/public/cpu/index.tsx

@@ -18,6 +18,7 @@ import {
   DetailSidebar,
   DataTable,
   DataTableColumn,
+  Toolbar,
 } from 'flipper-plugin';
 import adb from 'adbkit';
 import TemperatureTable from './TemperatureTable';
@@ -51,8 +52,6 @@ type CPUState = {
   displayCPUDetail: boolean;
 };
 
-type ShellCallBack = (output: string) => any;
-
 // check if str is a number
 function isNormalInteger(str: string) {
   const n = Math.floor(Number(str));
@@ -75,12 +74,15 @@ function formatFrequency(freq: number) {
 export function devicePlugin(client: PluginClient<{}, {}>) {
   const device = client.device;
 
-  const executeShell = (callback: ShellCallBack, command: string) => {
+  const executeShell = async (command: string) => {
-    return (device.realDevice as any).adb
+    return new Promise<string>((resolve, reject) => {
+      (device.realDevice as any).adb
         .shell(device.serial, command)
         .then(adb.util.readAll)
         .then(function (output: {toString: () => {trim: () => string}}) {
-        return callback(output.toString().trim());
+          resolve(output.toString().trim());
+        })
+        .catch((e: unknown) => reject(e));
     });
   };
 
@@ -96,12 +98,13 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     displayCPUDetail: true,
   });
 
-  const updateCoreFrequency: (core: number, type: string) => Promise<void> = (
+  const updateCoreFrequency: (
     core: number,
     type: string,
-  ) => {
+  ) => Promise<void> = async (core: number, type: string) => {
-    return new Promise((resolve, _reject) => {
+    const output = await executeShell(
-      executeShell((output: string) => {
+      'cat /sys/devices/system/cpu/cpu' + core + '/cpufreq/' + type,
+    );
     cpuState.update((draft) => {
       const newFreq = isNormalInteger(output) ? parseInt(output, 10) : -1;
       // update table only if frequency changed
@@ -112,18 +115,17 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
           draft.cpuFreq[core][type] = -2;
         }
       }
-          return draft;
-        });
-        resolve();
-      }, 'cat /sys/devices/system/cpu/cpu' + core + '/cpufreq/' + type);
     });
   };
 
-  const updateAvailableFrequencies: (core: number) => Promise<void> = (
+  const updateAvailableFrequencies: (core: number) => Promise<void> = async (
     core: number,
   ) => {
-    return new Promise((resolve, _reject) => {
+    const output = await executeShell(
-      executeShell((output: string) => {
+      'cat /sys/devices/system/cpu/cpu' +
+        core +
+        '/cpufreq/scaling_available_frequencies',
+    );
     cpuState.update((draft) => {
       const freqs = output.split(' ').map((num: string) => {
         return parseInt(num, 10);
@@ -133,40 +135,33 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
       if (maxFreq > 0 && freqs.indexOf(maxFreq) == -1) {
         freqs.push(maxFreq); // always add scaling max to available frequencies
       }
-          return draft;
-        });
-        resolve();
-      }, 'cat /sys/devices/system/cpu/cpu' + core + '/cpufreq/scaling_available_frequencies');
     });
   };
 
-  const updateCoreGovernor: (core: number) => Promise<void> = (
+  const updateCoreGovernor: (core: number) => Promise<void> = async (
     core: number,
   ) => {
-    return new Promise((resolve, _reject) => {
+    const output = await executeShell(
-      executeShell((output: string) => {
+      'cat /sys/devices/system/cpu/cpu' + core + '/cpufreq/scaling_governor',
+    );
     cpuState.update((draft) => {
       if (output.toLowerCase().includes('no such file')) {
         draft.cpuFreq[core].scaling_governor = 'N/A';
       } else {
         draft.cpuFreq[core].scaling_governor = output;
       }
-          return draft;
-        });
-        resolve();
-      }, 'cat /sys/devices/system/cpu/cpu' + core + '/cpufreq/scaling_governor');
     });
   };
 
-  const readAvailableGovernors: (core: number) => Promise<string[]> = (
+  const readAvailableGovernors: (core: number) => Promise<string[]> = async (
     core: number,
   ) => {
-    return new Promise((resolve, _reject) => {
+    const output = await executeShell(
-      executeShell((output: string) => {
+      'cat /sys/devices/system/cpu/cpu' +
-        // draft.cpuFreq[core].scaling_available_governors = output.split(' ');
+        core +
-        resolve(output.split(' '));
+        '/cpufreq/scaling_available_governors',
-      }, 'cat /sys/devices/system/cpu/cpu' + core + '/cpufreq/scaling_available_governors');
+    );
-    });
+    return output.split(' ');
   };
 
   const readCoreFrequency = async (core: number) => {
@@ -184,8 +179,8 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     return Promise.all(promises).then(() => {});
   };
 
-  const updateHardwareInfo = () => {
+  const updateHardwareInfo = async () => {
-    executeShell((output: string) => {
+    const output = await executeShell('getprop ro.board.platform');
     let hwInfo = '';
     if (
       output.startsWith('msm') ||
@@ -194,13 +189,12 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     ) {
       hwInfo = 'QUALCOMM ' + output.toUpperCase();
     } else if (output.startsWith('exynos')) {
-        executeShell((output: string) => {
+      const chipname = await executeShell('getprop ro.chipname');
-          if (output != null) {
+      if (chipname != null) {
         cpuState.update((draft) => {
-              draft.hardwareInfo = 'SAMSUMG ' + output.toUpperCase();
+          draft.hardwareInfo = 'SAMSUMG ' + chipname.toUpperCase();
         });
       }
-        }, 'getprop ro.chipname');
       return;
     } else if (output.startsWith('mt')) {
       hwInfo = 'MEDIATEK ' + output.toUpperCase();
@@ -215,19 +209,16 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     }
     cpuState.update((draft) => {
       draft.hardwareInfo = hwInfo;
-        return draft;
     });
-    }, 'getprop ro.board.platform');
   };
 
-  const readThermalZones = () => {
+  const readThermalZones = async () => {
     const thermal_dir = '/sys/class/thermal/';
     const map = {};
-    executeShell(async (output: string) => {
+    const output = await executeShell('ls ' + thermal_dir);
     if (output.toLowerCase().includes('permission denied')) {
       cpuState.update((draft) => {
         draft.thermalAccessible = false;
-          return draft;
       });
       return;
     }
@@ -245,20 +236,18 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     cpuState.update((draft) => {
       draft.temperatureMap = map;
       draft.thermalAccessible = true;
-        return draft;
     });
     if (cpuState.get().displayThermalInfo) {
       setTimeout(readThermalZones, 1000);
     }
-    }, 'ls ' + thermal_dir);
   };
 
-  const readThermalZone = (path: string, dir: string, map: any) => {
+  const readThermalZone = async (path: string, dir: string, map: any) => {
-    return executeShell((type: string) => {
+    const type = await executeShell('cat ' + path + '/type');
     if (type.length == 0) {
       return;
     }
-      return executeShell((temp: string) => {
+    const temp = await executeShell('cat ' + path + '/temp');
     if (Number.isNaN(Number(temp))) {
       return;
     }
@@ -266,8 +255,6 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
       path: dir,
       temp: parseInt(temp, 10),
     };
-      }, 'cat ' + path + '/temp');
-    }, 'cat ' + path + '/type');
   };
 
   const onStartMonitor = () => {
@@ -309,11 +296,11 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     intervalID = null;
     cpuState.update((draft) => {
       draft.monitoring = false;
-      return draft;
     });
   };
 
   const cleanup = () => {
+    onStopMonitor();
     cpuState.update((draft) => {
       for (let i = 0; i < draft.cpuCount; ++i) {
         draft.cpuFreq[i].scaling_cur_freq = -1;
@@ -334,7 +321,6 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     cpuState.update((draft) => {
       draft.displayThermalInfo = !draft.displayThermalInfo;
       draft.displayCPUDetail = false;
-      return draft;
     });
   };
 
@@ -342,12 +328,11 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
     cpuState.update((draft) => {
       draft.displayCPUDetail = !draft.displayCPUDetail;
       draft.displayThermalInfo = false;
-      return draft;
     });
   };
 
   // check how many cores we have on this device
-  executeShell((output: string) => {
+  executeShell('cat /sys/devices/system/cpu/possible').then((output) => {
     const idx = output.indexOf('-');
     const cpuFreq = [];
     const count = parseInt(output.substring(idx + 1), 10) + 1;
@@ -374,7 +359,7 @@ export function devicePlugin(client: PluginClient<{}, {}>) {
       displayThermalInfo: false,
       displayCPUDetail: true,
     });
-  }, 'cat /sys/devices/system/cpu/possible');
+  });
 
   client.onDeactivate(() => cleanup());
   client.onActivate(() => {
@@ -415,7 +400,157 @@ const cpuSidebarColumns: DataTableColumn[] = [
   },
 ];
 
-const getRowStyle = (freq: CPUFrequency) => {
+export function Component() {
+  const instance = usePlugin(devicePlugin);
+  const {
+    onStartMonitor,
+    onStopMonitor,
+    toggleCPUSidebar,
+    toggleThermalSidebar,
+  } = instance;
+
+  const cpuState = useValue(instance.cpuState);
+
+  const [selectedIds, setSelectedIds] = useState<number[]>([]);
+
+  const sidebarRows = (id: number) => {
+    let availableFreqTitle = 'Scaling Available Frequencies';
+    const selected = cpuState.cpuFreq[id];
+    if (selected.scaling_available_freqs.length > 0) {
+      availableFreqTitle +=
+        ' (' + selected.scaling_available_freqs.length.toString() + ')';
+    }
+
+    const keys = [availableFreqTitle, 'Scaling Available Governors'];
+
+    const vals = [
+      buildAvailableFreqList(selected),
+      buildAvailableGovList(selected),
+    ];
+    return keys.map<any>((key, idx) => {
+      return buildSidebarRow(key, vals[idx]);
+    });
+  };
+
+  const renderCPUSidebar = () => {
+    if (!cpuState.displayCPUDetail || selectedIds.length == 0) {
+      return null;
+    }
+    const id = selectedIds[0];
+    return (
+      <DetailSidebar width={500}>
+        <Layout.Container pad>
+          <Typography.Title>CPU Details: CPU_{id}</Typography.Title>
+          <DataTable
+            records={sidebarRows(id)}
+            columns={cpuSidebarColumns}
+            scrollable={false}
+            searchbar={false}
+          />
+        </Layout.Container>
+      </DetailSidebar>
+    );
+  };
+
+  const renderThermalSidebar = () => {
+    if (!cpuState.displayThermalInfo) {
+      return null;
+    }
+    return (
+      <DetailSidebar width={500}>
+        <Panel
+          pad={theme.space.small}
+          title="Thermal Information"
+          collapsible={false}>
+          {cpuState.thermalAccessible ? (
+            <TemperatureTable temperatureMap={cpuState.temperatureMap} />
+          ) : (
+            'Temperature information not accessible on this device.'
+          )}
+        </Panel>
+      </DetailSidebar>
+    );
+  };
+
+  const setSelected = useCallback((selected: any) => {
+    setSelectedIds(selected ? [selected.core] : []);
+  }, []);
+
+  return (
+    <Layout.Container pad>
+      <Typography.Title>CPU Info</Typography.Title>
+      <Toolbar>
+        {cpuState.monitoring ? (
+          <Button onClick={onStopMonitor} icon={<PauseCircleOutlined />}>
+            Pause
+          </Button>
+        ) : (
+          <Button onClick={onStartMonitor} icon={<PlayCircleOutlined />}>
+            Start
+          </Button>
+        )}
+        &nbsp; {cpuState.hardwareInfo}
+        <Switch
+          checked={cpuState.displayThermalInfo}
+          onClick={toggleThermalSidebar}
+        />
+        Thermal Information
+        <Switch
+          onClick={toggleCPUSidebar}
+          checked={cpuState.displayCPUDetail}
+        />
+        CPU Details
+        {cpuState.displayCPUDetail &&
+          selectedIds.length == 0 &&
+          ' (Please select a core in the table below)'}
+      </Toolbar>
+
+      <DataTable
+        records={frequencyRows(cpuState.cpuFreq)}
+        columns={columns}
+        scrollable={false}
+        onSelect={setSelected}
+        onRowStyle={getRowStyle}
+        searchbar={false}
+      />
+      {renderCPUSidebar()}
+      {renderThermalSidebar()}
+    </Layout.Container>
+  );
+}
+
+function buildAvailableGovList(freq: CPUFrequency): string {
+  if (freq.scaling_available_governors.length == 0) {
+    return 'N/A';
+  }
+  return freq.scaling_available_governors.join(', ');
+}
+
+function buildSidebarRow(key: string, val: any) {
+  return {
+    key: key,
+    value: val,
+  };
+}
+
+function buildRow(freq: CPUFrequency) {
+  return {
+    core: freq.cpu_id,
+    cpu_id: `CPU_${freq.cpu_id}`,
+    scaling_cur_freq: formatFrequency(freq.scaling_cur_freq),
+    scaling_min_freq: formatFrequency(freq.scaling_min_freq),
+    scaling_max_freq: formatFrequency(freq.scaling_max_freq),
+    cpuinfo_min_freq: formatFrequency(freq.cpuinfo_min_freq),
+    cpuinfo_max_freq: formatFrequency(freq.cpuinfo_max_freq),
+    scaling_governor: freq.scaling_governor,
+  };
+}
+
+function frequencyRows(cpuFreqs: Array<CPUFrequency>) {
+  return cpuFreqs.map(buildRow);
+}
+
+function getRowStyle(freq: CPUFrequency) {
   if (freq.scaling_cur_freq == -2) {
     return {
       backgroundColor: theme.backgroundWash,
@@ -443,39 +578,9 @@ const getRowStyle = (freq: CPUFrequency) => {
       fontWeight: 700,
     };
   }
-};
+}
 
-export function Component() {
+function buildAvailableFreqList(freq: CPUFrequency) {
-  const instance = usePlugin(devicePlugin);
-  const {
-    onStartMonitor,
-    onStopMonitor,
-    toggleCPUSidebar,
-    toggleThermalSidebar,
-  } = instance;
-
-  const cpuState = useValue(instance.cpuState);
-
-  const [selectedIds, setSelectedIds] = useState<number[]>([]);
-
-  const buildRow = (freq: CPUFrequency) => {
-    return {
-      core: freq.cpu_id,
-      cpu_id: `CPU_${freq.cpu_id}`,
-      scaling_cur_freq: formatFrequency(freq.scaling_cur_freq),
-      scaling_min_freq: formatFrequency(freq.scaling_min_freq),
-      scaling_max_freq: formatFrequency(freq.scaling_max_freq),
-      cpuinfo_min_freq: formatFrequency(freq.cpuinfo_min_freq),
-      cpuinfo_max_freq: formatFrequency(freq.cpuinfo_max_freq),
-      scaling_governor: freq.scaling_governor,
-    };
-  };
-
-  const frequencyRows = (cpuFreqs: Array<CPUFrequency>) => {
-    return cpuFreqs.map(buildRow);
-  };
-
-  const buildAvailableFreqList = (freq: CPUFrequency) => {
   if (freq.scaling_available_freqs.length == 0) {
     return <Typography.Text>N/A</Typography.Text>;
   }
@@ -508,120 +613,4 @@ export function Component() {
       })}
     </Typography.Text>
   );
-  };
-
-  const buildAvailableGovList = (freq: CPUFrequency): string => {
-    if (freq.scaling_available_governors.length == 0) {
-      return 'N/A';
-    }
-    return freq.scaling_available_governors.join(', ');
-  };
-
-  const buildSidebarRow = (key: string, val: any) => {
-    return {
-      key: key,
-      value: val,
-    };
-  };
-
-  const sidebarRows = (id: number) => {
-    let availableFreqTitle = 'Scaling Available Frequencies';
-    const selected = cpuState.cpuFreq[id];
-    if (selected.scaling_available_freqs.length > 0) {
-      availableFreqTitle +=
-        ' (' + selected.scaling_available_freqs.length.toString() + ')';
-    }
-
-    const keys = [availableFreqTitle, 'Scaling Available Governors'];
-
-    const vals = [
-      buildAvailableFreqList(selected),
-      buildAvailableGovList(selected),
-    ];
-    return keys.map<any>((key, idx) => {
-      return buildSidebarRow(key, vals[idx]);
-    });
-  };
-
-  const renderCPUSidebar = () => {
-    if (!cpuState.displayCPUDetail || selectedIds.length == 0) {
-      return null;
-    }
-    const id = selectedIds[0];
-    return (
-      <DetailSidebar width={500}>
-        <Typography.Title>CPU Details: CPU_{id}</Typography.Title>
-        <DataTable
-          records={sidebarRows(id)}
-          columns={cpuSidebarColumns}
-          scrollable={false}
-        />
-      </DetailSidebar>
-    );
-  };
-
-  const renderThermalSidebar = () => {
-    if (!cpuState.displayThermalInfo) {
-      return null;
-    }
-    return (
-      <DetailSidebar width={500}>
-        <Panel
-          pad={theme.space.small}
-          title="Thermal Information"
-          collapsible={false}>
-          {cpuState.thermalAccessible ? (
-            <TemperatureTable temperatureMap={cpuState.temperatureMap} />
-          ) : (
-            'Temperature information not accessible on this device.'
-          )}
-        </Panel>
-      </DetailSidebar>
-    );
-  };
-
-  const setSelected = useCallback((selected: any) => {
-    setSelectedIds(selected ? [selected.core] : []);
-  }, []);
-
-  return (
-    <Layout.Container pad={theme.space.medium}>
-      <Typography.Title>CPU Info</Typography.Title>
-      <Layout.Horizontal gap={theme.space.small} center>
-        {cpuState.monitoring ? (
-          <Button onClick={onStopMonitor} icon={<PauseCircleOutlined />}>
-            Pause
-          </Button>
-        ) : (
-          <Button onClick={onStartMonitor} icon={<PlayCircleOutlined />}>
-            Start
-          </Button>
-        )}
-        &nbsp; {cpuState.hardwareInfo}
-        <Switch
-          checked={cpuState.displayThermalInfo}
-          onClick={toggleThermalSidebar}
-        />
-        Thermal Information
-        <Switch
-          onClick={toggleCPUSidebar}
-          checked={cpuState.displayCPUDetail}
-        />
-        CPU Details
-        {cpuState.displayCPUDetail &&
-          selectedIds.length == 0 &&
-          ' (Please select a core in the table below)'}
-      </Layout.Horizontal>
-
-      <DataTable
-        records={frequencyRows(cpuState.cpuFreq)}
-        columns={columns}
-        scrollable={false}
-        onSelect={setSelected}
-        onRowStyle={getRowStyle}
-      />
-      {renderCPUSidebar()}
-      {renderThermalSidebar()}
-    </Layout.Container>
-  );
 }