From 1f457399b820d9c5c606263893011d2af3f65f50 Mon Sep 17 00:00:00 2001 From: Jonathan Nobels Date: Wed, 13 Mar 2024 16:42:57 -0400 Subject: [PATCH] android: code review feedback and stylistic improvements (#200) Updates tailscale/corp#18202 Review feedback and stylistic improvements. Signed-off-by: Jonathan Nobels Co-authored-by: Andrea Gottardo --- .../java/com/tailscale/ipn/MainActivity.kt | 32 +++++--- .../ipn/ui/localapi/LocalAPIClient.kt | 4 +- .../com/tailscale/ipn/ui/notifier/Notifier.kt | 7 +- .../tailscale/ipn/ui/service/IpnManager.kt | 40 +++++----- .../com/tailscale/ipn/ui/service/IpnModel.kt | 76 ++++++++----------- cmd/localapiservice/localapishim.go | 14 ++-- 6 files changed, 83 insertions(+), 90 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/MainActivity.kt b/android/src/main/java/com/tailscale/ipn/MainActivity.kt index 2322f29..2e7884b 100644 --- a/android/src/main/java/com/tailscale/ipn/MainActivity.kt +++ b/android/src/main/java/com/tailscale/ipn/MainActivity.kt @@ -3,10 +3,10 @@ package com.tailscale.ipn -import android.content.Intent -import android.net.Uri import android.content.Context +import android.content.Intent import android.content.RestrictionsManager +import android.net.Uri import android.os.Bundle import androidx.activity.ComponentActivity import androidx.activity.compose.setContent @@ -37,7 +37,7 @@ import kotlinx.coroutines.launch class MainActivity : ComponentActivity() { - private val manager = IpnManager() + private val manager = IpnManager(lifecycleScope) override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -57,20 +57,31 @@ class MainActivity : ComponentActivity() { val settingsNav = SettingsNav( onNavigateToBugReport = { navController.navigate("bugReport") }, onNavigateToAbout = { navController.navigate("about") } + ) composable("main") { - MainView(viewModel = MainViewModel(manager.model, manager.actions), navigation = mainViewNav) + MainView( + viewModel = MainViewModel(manager.model, manager), + navigation = mainViewNav + ) } composable("settings") { - Settings(SettingsViewModel(manager.model, manager.actions, settingsNav)) + Settings(SettingsViewModel(manager.model, manager, settingsNav)) } composable("exitNodes") { ExitNodePicker(ExitNodePickerViewModel(manager.model)) } - composable("peerDetails/{nodeId}", arguments = listOf(navArgument("nodeId") { type = NavType.StringType })) { - PeerDetails(PeerDetailsViewModel(manager.model, nodeId = it.arguments?.getString("nodeId") - ?: "")) + composable( + "peerDetails/{nodeId}", + arguments = listOf(navArgument("nodeId") { type = NavType.StringType }) + ) { + PeerDetails( + PeerDetailsViewModel( + manager.model, nodeId = it.arguments?.getString("nodeId") + ?: "" + ) + ) } composable("bugReport") { BugReportView(BugReportViewModel(manager.apiClient)) @@ -97,7 +108,7 @@ class MainActivity : ComponentActivity() { } } - fun login(url: String) { + private fun login(url: String) { // (jonathan) TODO: This is functional, but the navigation doesn't quite work // as expected. There's probably a better built in way to do this. This will // unblock in dev for the time being though. @@ -108,7 +119,8 @@ class MainActivity : ComponentActivity() { override fun onResume() { super.onResume() - val restrictionsManager = this.getSystemService(Context.RESTRICTIONS_SERVICE) as RestrictionsManager + val restrictionsManager = + this.getSystemService(Context.RESTRICTIONS_SERVICE) as RestrictionsManager manager.mdmSettings = MDMSettings(restrictionsManager) } } diff --git a/android/src/main/java/com/tailscale/ipn/ui/localapi/LocalAPIClient.kt b/android/src/main/java/com/tailscale/ipn/ui/localapi/LocalAPIClient.kt index d5db0ef..ead26eb 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/localapi/LocalAPIClient.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/localapi/LocalAPIClient.kt @@ -59,11 +59,11 @@ class LocalApiClient(private val scope: CoroutineScope) { // This is called from the JNI layer to publish localAPIResponses. This should execute on the // same thread that called doRequest. @Suppress("unused") - fun onResponse(response: String, cookie: String) { + fun onResponse(response: ByteArray, cookie: String) { requests.remove(cookie)?.let { request -> Log.d("LocalApiClient", "Response for request:${request.path} cookie:${request.cookie}") // The response handler will invoked internally by the request parser - request.parser(response.encodeToByteArray()) + request.parser(response) } ?: { Log.e("LocalApiClient", "Received response for unknown request: $cookie") } } diff --git a/android/src/main/java/com/tailscale/ipn/ui/notifier/Notifier.kt b/android/src/main/java/com/tailscale/ipn/ui/notifier/Notifier.kt index 3e2d7a4..e4d7f05 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/notifier/Notifier.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/notifier/Notifier.kt @@ -68,7 +68,7 @@ class Notifier() { // Starts an IPN Bus watcher. **This is blocking** and will not return until // the watcher is stopped and must be executed in a suitable coroutine scope such - // as Dispatchers.IO + // as Dispatchers.IO private external fun startIPNBusWatcher(sessionId: String, mask: Int) // Stops an IPN Bus watcher @@ -87,7 +87,7 @@ class Notifier() { // Watch the IPN bus for notifications // Notifications will be passed to the caller via the callback until // the caller calls unwatchIPNBus with the sessionId returned from this call. - fun watchIPNBus(mask: Int, callback: NotifierCallback): String { + private fun watchIPNBus(mask: Int, callback: NotifierCallback): String { val sessionId = generateSessionId() val watcher = Watcher(sessionId, mask, callback) watchers[sessionId] = watcher @@ -104,7 +104,7 @@ class Notifier() { // Cancels the watcher with the given sessionId. No errors are thrown or // indicated for invalid sessionIds. - fun unwatchIPNBus(sessionId: String) { + private fun unwatchIPNBus(sessionId: String) { stopIPNBusWatcher(sessionId) } @@ -147,4 +147,3 @@ class Notifier() { Log.d("Notifier", "Notifier created") } } - diff --git a/android/src/main/java/com/tailscale/ipn/ui/service/IpnManager.kt b/android/src/main/java/com/tailscale/ipn/ui/service/IpnManager.kt index 41f8e75..adfb90d 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/service/IpnManager.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/service/IpnManager.kt @@ -12,53 +12,49 @@ import com.tailscale.ipn.ui.localapi.LocalApiClient import com.tailscale.ipn.ui.model.Ipn import com.tailscale.ipn.ui.notifier.Notifier import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.SupervisorJob typealias PrefChangeCallback = (Result) -> Unit // Abstracts the actions that can be taken by the UI so that the concept of an IPNManager // itself is hidden from the viewModel implementations. -data class IpnActions( - val startVPN: () -> Unit, - val stopVPN: () -> Unit, - val login: () -> Unit, - val logout: () -> Unit, - val updatePrefs: (Ipn.MaskedPrefs, PrefChangeCallback) -> Unit -) +interface IpnActions { + fun startVPN() + fun stopVPN() + fun login() + fun logout() + fun updatePrefs(prefs: Ipn.MaskedPrefs, callback: PrefChangeCallback) +} -class IpnManager { +class IpnManager(scope: CoroutineScope) : IpnActions { private var notifier = Notifier() - private var scope = CoroutineScope(SupervisorJob() + Dispatchers.Main) var apiClient = LocalApiClient(scope) var mdmSettings = MDMSettings() val model = IpnModel(notifier, apiClient, scope) - val actions = IpnActions( - startVPN = { startVPN() }, - stopVPN = { stopVPN() }, - login = { apiClient.startLoginInteractive() }, - logout = { apiClient.logout() }, - updatePrefs = { prefs, callback -> updatePrefs(prefs, callback) } - ) - - fun startVPN() { + override fun startVPN() { val context = App.getApplication().applicationContext val intent = Intent(context, IPNReceiver::class.java) intent.action = IPNReceiver.INTENT_CONNECT_VPN context.sendBroadcast(intent) } - fun stopVPN() { + override fun stopVPN() { val context = App.getApplication().applicationContext val intent = Intent(context, IPNReceiver::class.java) intent.action = IPNReceiver.INTENT_DISCONNECT_VPN context.sendBroadcast(intent) + } + + override fun login() { + apiClient.startLoginInteractive() + } + override fun logout() { + apiClient.logout() } - fun updatePrefs(prefs: Ipn.MaskedPrefs, callback: PrefChangeCallback) { + override fun updatePrefs(prefs: Ipn.MaskedPrefs, callback: PrefChangeCallback) { apiClient.editPrefs(prefs) { result -> result.success?.let { callback(Result.success(true)) diff --git a/android/src/main/java/com/tailscale/ipn/ui/service/IpnModel.kt b/android/src/main/java/com/tailscale/ipn/ui/service/IpnModel.kt index 3af7c48..257aa1d 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/service/IpnModel.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/service/IpnModel.kt @@ -14,6 +14,13 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.launch +/** + * Provides a way to expose a MutableStateFlow as an immutable StateFlow. + */ +fun StateFlow.set(v: T) { + (this as MutableStateFlow).value = v +} + class IpnModel( notifier: Notifier, val apiClient: LocalApiClient, @@ -21,31 +28,16 @@ class IpnModel( ) { private var notifierSessions: MutableList = mutableListOf() - - private val _state: MutableStateFlow = MutableStateFlow(Ipn.State.NoState) - private val _netmap: MutableStateFlow = MutableStateFlow(null) - protected val _prefs: MutableStateFlow = MutableStateFlow(null) - private val _engineStatus: MutableStateFlow = MutableStateFlow(null) - private val _tailFSShares: MutableStateFlow?> = MutableStateFlow(null) - private val _browseToURL: MutableStateFlow = MutableStateFlow(null) - private val _loginFinished: MutableStateFlow = MutableStateFlow(null) - private val _version: MutableStateFlow = MutableStateFlow(null) - - private val _loggedInUser: MutableStateFlow = MutableStateFlow(null) - private val _loginProfiles: MutableStateFlow?> = - MutableStateFlow(null) - - - val state: StateFlow = _state - val netmap: StateFlow = _netmap - val prefs: StateFlow = _prefs - val engineStatus: StateFlow = _engineStatus - val tailFSShares: StateFlow?> = _tailFSShares - val browseToURL: StateFlow = _browseToURL - val loginFinished: StateFlow = _loginFinished - val version: StateFlow = _version - val loggedInUser: StateFlow = _loggedInUser - val loginProfiles: StateFlow?> = _loginProfiles + val state: StateFlow = MutableStateFlow(Ipn.State.NoState) + val netmap: StateFlow = MutableStateFlow(null) + val prefs: StateFlow = MutableStateFlow(null) + val engineStatus: StateFlow = MutableStateFlow(null) + val tailFSShares: StateFlow?> = MutableStateFlow(null) + val browseToURL: StateFlow = MutableStateFlow(null) + val loginFinished: StateFlow = MutableStateFlow(null) + val version: StateFlow = MutableStateFlow(null) + val loggedInUser: StateFlow = MutableStateFlow(null) + val loginProfiles: StateFlow?> = MutableStateFlow(null) val isUsingExitNode: Boolean get() { @@ -58,41 +50,35 @@ class IpnModel( LocalApiClient.isReady.await() apiClient.getProfiles { result -> - result.success?.let { users -> _loginProfiles.value = users } + result.success?.let(loginProfiles::set) ?: run { Log.e("IpnManager", "Error loading profiles: ${result.error}") } } apiClient.getCurrentProfile { result -> - result.success?.let { user -> _loggedInUser.value = user } - ?: run { Log.e("IpnManager", "Error loading profiles: ${result.error}") } + result.success?.let(loggedInUser::set) + ?: run { Log.e("IpnManager", "Error loading current profile: ${result.error}") } } } private fun onNotifyChange(notify: Ipn.Notify) { - notify.State?.let { state -> + notify.State?.let { s -> // Refresh the user profiles if we're transitioning out of the // NeedsLogin state. - if (_state.value == Ipn.State.NeedsLogin) { + if (state.value == Ipn.State.NeedsLogin) { scope.launch { loadUserProfiles() } } - Log.d("IpnModel", "State changed: $state") - _state.value = Ipn.State.fromInt(state) + Log.d("IpnModel", "State changed: $s") + state.set(Ipn.State.fromInt(s)) } - notify.NetMap?.let { netmap -> _netmap.value = netmap } - - notify.Prefs?.let { prefs -> _prefs.value = prefs } - - notify.Engine?.let { engine -> _engineStatus.value = engine } - - notify.TailFSShares?.let { shares -> _tailFSShares.value = shares } - - notify.BrowseToURL?.let { url -> _browseToURL.value = url } - - notify.LoginFinished?.let { message -> _loginFinished.value = message.property } - - notify.Version?.let { version -> _version.value = version } + notify.NetMap?.let(netmap::set) + notify.Prefs?.let(prefs::set) + notify.Engine?.let(engineStatus::set) + notify.TailFSShares?.let(tailFSShares::set) + notify.BrowseToURL?.let(browseToURL::set) + notify.LoginFinished?.let { loginFinished.set(it.property) } + notify.Version?.let(version::set) } init { diff --git a/cmd/localapiservice/localapishim.go b/cmd/localapiservice/localapishim.go index a0b8553..ef40e7a 100644 --- a/cmd/localapiservice/localapishim.go +++ b/cmd/localapiservice/localapishim.go @@ -66,17 +66,17 @@ func Java_com_tailscale_ipn_ui_localapi_LocalApiClient_doRequest( resp := doLocalAPIRequest(pathStr, methodStr, bodyArray) - jrespBody := jni.JavaString(jenv, resp) + jrespBody := jni.NewByteArray(jenv, resp) respBody := jni.Value(jrespBody) cookie := jni.Value(jcookie) - onResponse := jni.GetMethodID(jenv, shim.clientClass, "onResponse", "(Ljava/lang/String;Ljava/lang/String;)V") + onResponse := jni.GetMethodID(jenv, shim.clientClass, "onResponse", "([BLjava/lang/String;)V") jni.CallVoidMethod(jenv, jni.Object(cls), onResponse, respBody, cookie) } -func doLocalAPIRequest(path string, method string, body []byte) string { +func doLocalAPIRequest(path string, method string, body []byte) []byte { if shim.service == nil { - return "{\"error\":\"Not Ready\"}" + return []byte("{\"error\":\"Not Ready\"}") } ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) @@ -90,13 +90,13 @@ func doLocalAPIRequest(path string, method string, body []byte) string { defer r.Body().Close() if err != nil { - return "{\"error\":\"" + err.Error() + "\"}" + return []byte("{\"error\":\"" + err.Error() + "\"}") } respBytes, err := io.ReadAll(r.Body()) if err != nil { - return "{\"error\":\"" + err.Error() + "\"}" + return []byte("{\"error\":\"" + err.Error() + "\"}") } - return string(respBytes) + return respBytes } // Assign a localAPIService to our shim for handling incoming localapi requests from the Kotlin side.