diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index c20febe171..0d635babfe 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -51,6 +51,14 @@ Feature code layers as **UI → ViewModel → Repository → data source**, with **EventBus is a deliberate side-channel.** Long-running, cross-module signals (build/install lifecycle, editor events) are broadcast via GreenRobot EventBus (`@Subscribe(threadMode = ThreadMode.MAIN)`) and the `eventbus-events` module's shared event types. Treat it as the integration bus *between* subsystems; don't use it to replace a ViewModel's own state inside a single screen. +**App Links enter through a UI-less trampoline, not `MainActivity` directly.** `DeepLinkActivity` (`app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt`) is the sole `` holder for `https://appdevforall.org/device/open/project/...` (and the identical `www` subdomain). It never renders anything — it parses the URI into a `DeepLinkRequest` (project name plus an optional file/line/column), checks whether an editor is already on screen (`ActionContextProvider.getActivity()`, the live `EditorHandlerActivity` tracker -- not `IProjectManager`'s `workspace`, which stays null for the whole duration of a Gradle sync even while the editor is already open), and routes to `MainActivity` (nothing open) or the live, `singleTask` `EditorActivityKt`/`EditorHandlerActivity` (a project is open — reused via `onNewIntent`), then finishes itself. This avoids a visible flash of `MainActivity`'s real UI when the actual destination is the already-running editor. + +`EditorHandlerActivity.onNewIntent` then branches on `projectDirPath` (set as soon as a project starts opening) rather than `workspace` so the mid-sync case still matches correctly: **same project already open** — no project-wise work, just navigate to the requested file (`applyDeepLinkFileRequest`); **a different project is open** — the existing, unmodified `confirmProjectClose()` dialog runs (it also guards against a second confirm-close request overlapping a manual close or an in-flight save, and a *third* overlapping request supersedes the second's pending callback rather than being dropped), and only once the user actually confirms does an `onDestroy()`-triggered hand-off (`PendingDeepLinkOpen`, Koin-provided) start the new project — deliberately deferred to `onDestroy()`, not fired synchronously after `finish()`, so the new `PROJECT_PATH` can't race a `singleTask` re-delivery to the dying instance; `projectDirPath` **is still blank** — this instance never actually finished initializing a project (e.g. recreated after process death with no `PROJECT_PATH` extra), so `confirmProjectClose()` would silently no-op (`contentOrNull` is null); this case reuses the same `onDestroy()`-deferred hand-off instead of showing a close dialog for a project that was never really open; **nothing was open** — `MainActivity.openProject`/`EditorHandlerActivity.postProjectInit` apply the pending file request once the cold-opened project's sync succeeds. + +`DeepLinkActivity`'s "is a live editor already on screen" check (`ActionContextProvider.getActivity()`) is itself a heuristic, not a guarantee: Android can still spin up a genuinely new `EditorActivityKt` instance instead of delivering to the live one via `onNewIntent`. `BaseEditorActivity.onCreate` is `EXTRA_KEY`'s only other reader on the editor side for exactly this case — it compares the deep link's requested project name against whatever project the new instance actually ends up holding (explicit `PROJECT_PATH` extra, restored `savedInstanceState`, or the process-wide `ProjectManagerImpl` singleton's last-loaded project) and, on a mismatch, bounces back to `MainActivity` with the deep link forwarded rather than silently continuing to build editor UI for the wrong project. + +The optional file path is attacker-controllable (a URL segment), so it's resolved through `PathTraversal.resolveWithinDirectory`'s traversal/symlink guard rather than a bare `File` join, both when opening a file in the already-open project and when matching the requested project name to a directory under `Environment.PROJECTS_DIR` (`findValidProjectByName`). + ## Module Structure Strategy: **layer-and-subsystem based**, not feature-by-feature. The Gradle build has ~80 modules (`settings.gradle.kts`) plus three included composite builds. `app` is the integration point; the rest are libraries it composes. @@ -100,7 +108,7 @@ These structural facts shape every module. Day-to-day build *commands* live in ` > **Persistence policy (authoritative):** new relational/queryable persistence uses **Room** (`@Entity` + DAO + `RoomDatabase` with explicit migrations, provided via Koin). Non-relational settings use the **filesystem/preferences (DataStore)**. **Raw SQLite is the exception, not the default** — see [ADR 0001](docs/adr/0001-prefer-room-for-persistence.md). > -> **Recent Projects** is the reference example of the default: `app/src/main/java/com/itsaky/androidide/roomData/recentproject/` (`RecentProjectRoomDatabase`, `@Database version = 4` with migrations 1→4; `RecentProjectDao`; the `RecentProject` `@Entity` → table `recent_project_table`). It's provided via Koin in `di/AppModule.kt` and consumed by `MainViewModel`, `RecentProjectsViewModel`, `MainActivity`, `ProjectInfoBottomSheet`, and `ProjectCreationManager`. +> **Recent Projects** is the reference example of the default: `app/src/main/java/com/itsaky/androidide/roomData/recentproject/` (`RecentProjectRoomDatabase`, `@Database version = 4` with migrations 1→4; `RecentProjectDao`; the `RecentProject` `@Entity` → table `recent_project_table`). It's provided via Koin in `di/AppModule.kt` and consumed by `RecentProjectsViewModel`, `ProjectInfoBottomSheet`, and `ProjectCreationManager` directly, and by `MainActivity`/`EditorHandlerActivity` indirectly through `RecentProjectRepository` (`repositories/RecentProjectRepository.kt`) -- kept behind that interface, rather than injecting the DAO into those two Activities directly, per this section's own UI -> ViewModel -> Repository -> data source layering. > > **Raw SQLite is allowed only when** the database is prebuilt and opened read-only, the data is performance/allocation-critical and needs granular schema control, or the schema is shared across a process/component boundary. Current exceptions: symbol indexing (`lsp/indexing/SQLiteIndex.kt`), tooltips (`idetooltips/ToolTipManager.kt`), in-app/plugin help (`plugin-manager/.../documentation/PluginDocumentationManager.kt`), and documentation serving (`common/.../documentation/DocumentationContentSource.kt`, the one pipeline behind both the in-process WebView transport and `app/.../localWebServer/WebServer.kt`). The `androidx.room:*` strings in `editor`'s `GroovyAutoComplete` are autocomplete suggestions for the *user's* code, not CoGo persistence. > diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 521b7867a1..7222fbdafc 100755 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -1,370 +1,406 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt new file mode 100644 index 0000000000..b5ead707b0 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt @@ -0,0 +1,130 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.activities + +import android.app.Activity +import android.content.Intent +import android.os.Bundle +import android.widget.Toast +import com.itsaky.androidide.activities.editor.EditorActivityKt +import com.itsaky.androidide.analytics.DeepLinkDepth +import com.itsaky.androidide.analytics.DeepLinkMetric +import com.itsaky.androidide.analytics.DeepLinkOutcome +import com.itsaky.androidide.analytics.IAnalyticsManager +import com.itsaky.androidide.analytics.depth +import com.itsaky.androidide.api.ActionContextProvider +import com.itsaky.androidide.models.DeepLinkRequest +import com.itsaky.androidide.resources.R.string +import org.koin.android.ext.android.inject + +/** + * The sole `` holder for `https://appdevforall.org/device/open/project/...` (and the + * identical `www` subdomain) App Links. Never shows any UI -- it only parses the incoming + * [android.net.Uri], decides whether a project is already loaded, and hands off to whichever real + * activity owns that scenario: + * [MainActivity] if nothing is open yet, or the already-running [EditorActivityKt] (via its + * `singleTask` `onNewIntent`) if one is. + * + * Kept as a plain [Activity] (like [SplashActivity]), not [com.itsaky.androidide.app.BaseIDEActivity], + * since it never calls `setContentView` and has no theming needs of its own. + */ +class DeepLinkActivity : Activity() { + private val analyticsManager: IAnalyticsManager by inject() + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + + // A link can arrive before the IDE is usable -- a fresh install, or a Clear Data. Both targets + // below sit *past* SplashActivity and OnboardingActivity, which are the only things enforcing + // the terms, the permissions, the JDK and SDK install, the low-storage check and the x86 + // exit, so following the link now would land the user in an editor that cannot build, on a + // device the app is supposed to refuse to run on at all (ADFA-5067 review). + // + // The link is dropped rather than deferred: carrying a request through an onboarding that can + // take several minutes, and may not finish at all, is a lot of machinery for a rare case. The + // user is told, and sent to the normal entry point, which decides what they actually need -- + // storage, ABI and onboarding are SplashActivity's to enforce, not this activity's to repeat. + if (!isIdeSetupComplete()) { + // Depth is UNKNOWN rather than parsed: the readiness gate deliberately runs before the + // URI is looked at, and reordering it just to enrich a metric would put parsing ahead of + // the check that exists to stop this activity acting on anything at all. + analyticsManager.trackDeepLink(DeepLinkMetric(DeepLinkDepth.UNKNOWN, DeepLinkOutcome.SETUP_INCOMPLETE)) + Toast.makeText(this, getString(string.msg_deeplink_setup_incomplete), Toast.LENGTH_LONG).show() + // FLAG_ACTIVITY_NEW_TASK, matching the success branch below. A sender that starts this + // trampoline without it -- an in-app WebView host, another app's explicit intent, `am start` + // -- puts this activity in the *caller's* task, and an unflagged start here would run the + // whole terms/permissions/JDK-install onboarding inside that app's back stack, where + // back-press returns to them rather than exiting CoGo. + startActivity( + Intent(this, SplashActivity::class.java).apply { + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + }, + ) + finish() + return + } + + val request = DeepLinkRequest.parse(intent?.data) + if (request == null) { + // Counted, not just shown: this activity is exported, so a rise here is as likely to be + // another app poking it with an arbitrary Uri as it is a broken published link. + analyticsManager.trackDeepLink(DeepLinkMetric(DeepLinkDepth.UNKNOWN, DeepLinkOutcome.INVALID_LINK)) + // A Toast, not flashError -- this activity finishes immediately below, tearing down its + // window before a view-based Flashbar could ever render. + Toast.makeText(this, getString(string.msg_deeplink_invalid_link), Toast.LENGTH_LONG).show() + finish() + return + } + + // The one place every accepted link passes through, whichever activity ends up handling it. + // Paired with a terminal outcome logged wherever the request is finally resolved, so a link + // that is accepted here and then quietly goes nowhere shows up as a gap between the two. + analyticsManager.trackDeepLink(DeepLinkMetric(request.depth(), DeepLinkOutcome.RECEIVED, request.projectName)) + + // ActionContextProvider tracks the live EditorHandlerActivity instance (set in its onCreate + // and re-asserted in onResume, cleared in onDestroy) -- this reflects "is an editor instance + // already alive to hand this off to via onNewIntent", unlike IProjectManager's workspace, + // which stays null for the whole duration of a Gradle sync even while EditorActivityKt is + // already open. + val target = + if (ActionContextProvider.getLiveActivity() != null) { + EditorActivityKt::class.java + } else { + MainActivity::class.java + } + + startActivity( + Intent(this, target).apply { + putExtra(DeepLinkRequest.EXTRA_KEY, request) + // FLAG_ACTIVITY_CLEAR_TOP deliberately omitted: MainActivity has no special launch + // mode, so if an existing MainActivity instance sits lower in this task's back stack + // under a live EditorActivityKt - which ActionContextProvider.getLiveActivity() can miss + // even when that editor is alive (see its KDoc) - CLEAR_TOP would destroy that editor + // to clear the path down to MainActivity, discarding unsaved work with no prompt. + // Without it, this may at worst stack a redundant MainActivity instance, a harmless + // nuisance; EditorActivityKt is singleTask, so it always reuses its live instance via + // onNewIntent regardless of these flags. + addFlags( + Intent.FLAG_ACTIVITY_NEW_TASK or + Intent.FLAG_ACTIVITY_SINGLE_TOP, + ) + }, + ) + finish() + } +} diff --git a/app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt index 7f51981128..04220129e7 100755 --- a/app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt @@ -23,7 +23,10 @@ import android.os.Bundle import android.view.KeyEvent import android.view.View import androidx.activity.OnBackPressedCallback +import androidx.appcompat.app.AlertDialog +import androidx.core.content.IntentCompat import androidx.core.graphics.Insets +import androidx.core.os.BundleCompat import androidx.core.view.WindowInsetsCompat import androidx.core.view.isVisible import androidx.lifecycle.lifecycleScope @@ -34,9 +37,13 @@ import com.itsaky.androidide.FeedbackButtonManager import com.itsaky.androidide.R import com.itsaky.androidide.actions.ActionData import com.itsaky.androidide.activities.editor.EditorActivityKt +import com.itsaky.androidide.analytics.DeepLinkMetric +import com.itsaky.androidide.analytics.DeepLinkOutcome import com.itsaky.androidide.analytics.IAnalyticsManager +import com.itsaky.androidide.analytics.depth import com.itsaky.androidide.app.EdgeToEdgeIDEActivity import com.itsaky.androidide.databinding.ActivityMainBinding +import com.itsaky.androidide.deeplink.ConsumedRequests import com.itsaky.androidide.fragments.MainFragment import com.itsaky.androidide.fragments.RecentProjectsFragment import com.itsaky.androidide.idetooltips.TooltipManager @@ -44,8 +51,12 @@ import com.itsaky.androidide.idetooltips.TooltipTag.PROJECT_RECENT_TOP import com.itsaky.androidide.idetooltips.TooltipTag.SETUP_OVERVIEW import com.itsaky.androidide.localWebServer.ServerConfig import com.itsaky.androidide.localWebServer.WebServer +import com.itsaky.androidide.models.DeepLinkRequest +import com.itsaky.androidide.models.EditorIntentExtras +import com.itsaky.androidide.models.PendingFileRequest import com.itsaky.androidide.preferences.internal.GeneralPreferences -import com.itsaky.androidide.projects.ProjectManagerImpl +import com.itsaky.androidide.projects.IProjectManager +import com.itsaky.androidide.repositories.RecentProjectRepository import com.itsaky.androidide.resources.R.string import com.itsaky.androidide.roomData.recentproject.RecentProject import com.itsaky.androidide.shortcuts.IdeShortcutActions @@ -53,6 +64,7 @@ import com.itsaky.androidide.shortcuts.ShortcutContext import com.itsaky.androidide.shortcuts.ShortcutExecutionContext import com.itsaky.androidide.shortcuts.ShortcutManager import com.itsaky.androidide.templates.ITemplateProvider +import com.itsaky.androidide.utils.DeepLinkProjectLookup import com.itsaky.androidide.utils.DialogUtils import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.FeatureFlags @@ -60,11 +72,11 @@ import com.itsaky.androidide.utils.MainScreenActions import com.itsaky.androidide.utils.UrlManager import com.itsaky.androidide.utils.applyBottomWindowInsetsPadding import com.itsaky.androidide.utils.findValidProjects +import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashInfo -import com.itsaky.androidide.utils.getCreatedTime -import com.itsaky.androidide.utils.getLastModifiedTime import com.itsaky.androidide.utils.hasVisibleDialog -import com.itsaky.androidide.utils.readProjectLanguage +import com.itsaky.androidide.utils.recordProjectOpenedBookkeeping +import com.itsaky.androidide.utils.resolveDeepLinkProject import com.itsaky.androidide.viewmodel.MainViewModel import com.itsaky.androidide.viewmodel.MainViewModel.Companion.SCREEN_CLONE_REPO import com.itsaky.androidide.viewmodel.MainViewModel.Companion.SCREEN_DELETE_PROJECTS @@ -89,10 +101,45 @@ class MainActivity : EdgeToEdgeIDEActivity() { @Suppress("ktlint:standard:backing-property-naming") private var _binding: ActivityMainBinding? = null private val analyticsManager: IAnalyticsManager by inject() + private val recentProjectRepository: RecentProjectRepository by inject() private var feedbackButtonManager: FeedbackButtonManager? = null private var webServer: WebServer? = null private val shortcutManager by lazy { ShortcutManager(applicationContext) } + // Tracked so a slower, older deep-link resolve (still in flight when a second, faster-resolving + // deep link arrives) can tell it's been superseded -- see handleDeepLinkRequest. + private var latestDeepLinkRequest: DeepLinkRequest? = null + + // The last deep-link request actually opened (or, if GeneralPreferences.confirmProjectOpen is on, + // actually confirmed) -- see handleOpenProject/askProjectOpenPermission. Persisted via + // onSaveInstanceState rather than signalled by removing the Intent's own extra: a genuine process + // death redelivers the ORIGINAL, unmutated launch Intent (extras and all) once the user returns to + // the task, so an Intent-mutation-based "already handled" signal doesn't survive it and this same + // request force-reopens a project the user has since navigated away from. A config-change + // recreate, in contrast, preserves this field across the recreate but correctly leaves it unset + // if the recreate happens before the user actually responds to the confirm dialog, so that dialog + // (destroyed along with the old instance) gets a fresh retry on the new one instead of the link + // being silently dropped. + // Every request consumed in this task, not just the last one -- see the class for why one slot + // was not enough. + private val consumedDeepLinkRequests = ConsumedRequests() + + // The subset of consumedDeepLinkRequests recorded because the project could not be resolved, + // rather than because anything was actually opened. + // + // The two have to be told apart by onNewIntent's re-forward gate. That gate exists to stop a + // bounce loop: this activity opens a project, the editor decides the link names a different one + // and bounces it straight back, and without the gate its dialog goes right back up. But a request + // that failed to resolve never reached the editor at all, so no bounce can originate from it -- + // and DeepLinkRequest carries no nonce, so a genuinely new tap of the same URL is equal by value + // to the failed one. Tapping a link for a project that does not exist yet, creating it, and + // tapping again therefore died silently, forever, which is exactly the teacher-sends-a-student-a- + // link flow the feature is for (ADFA-5067 review). + // + // Persisted alongside consumedDeepLinkRequests: without that, the same tap-create-tap sequence + // across a process death lands back in the identical hole. + private val unresolvedDeepLinkRequests = ConsumedRequests() + private val onBackPressedCallback = object : OnBackPressedCallback(true) { override fun handleOnBackPressed() { @@ -127,7 +174,29 @@ class MainActivity : EdgeToEdgeIDEActivity() { // Start WebServer after installation is complete startWebServer() - if (savedInstanceState == null) { + val deepLinkRequest = + IntentCompat.getParcelableExtra(intent, DeepLinkRequest.EXTRA_KEY, DeepLinkRequest::class.java) + consumedDeepLinkRequests.restore( + savedInstanceState?.let { + BundleCompat.getParcelableArrayList(it, KEY_CONSUMED_DEEP_LINK_REQUESTS, DeepLinkRequest::class.java) + }, + ) + unresolvedDeepLinkRequests.restore( + savedInstanceState?.let { + BundleCompat.getParcelableArrayList(it, KEY_UNRESOLVED_DEEP_LINK_REQUESTS, DeepLinkRequest::class.java) + }, + ) + // A config change this activity doesn't declare (e.g. font scale, day/night) recreates it with + // savedInstanceState != null while handleDeepLinkRequest's resolve may still be in flight -- + // the old instance's lifecycleScope (and its coroutine) is cancelled with it. Gating solely on + // savedInstanceState == null would silently lose a not-yet-consumed request instead of + // retrying it on the new instance; comparing against consumedDeepLinkRequests (restored above) + // rather than just checking deepLinkRequest != null is what tells a genuinely new/not-yet-acted- + // on request apart from the system redelivering the same original launch Intent verbatim after + // this same request was already fully handled (see consumedDeepLinkRequests' own docs). + if (deepLinkRequest != null && deepLinkRequest !in consumedDeepLinkRequests) { + handleDeepLinkRequest(deepLinkRequest) + } else if (savedInstanceState == null) { openLastProject() } @@ -370,7 +439,12 @@ class MainActivity : EdgeToEdgeIDEActivity() { if (!GeneralPreferences.autoOpenProjects) return lifecycleScope.launch(Dispatchers.IO) { - val validProjects = findValidProjects(Environment.PROJECTS_DIR) + // projectsRoot(), not the raw static: findValidProjects takes a non-null File, and + // Environment.PROJECTS_DIR is assigned by the same unawaited loader coroutine SetupState's + // KDoc describes -- and is not volatile. A null read here throws + // Intrinsics.checkNotNullParameter inside a coroutine with no handler. The sibling call in + // RecentProjectsFragment is wrapped in try/catch(Throwable); this one was not. + val validProjects = findValidProjects(projectsRoot()) val lastOpenedPath = GeneralPreferences.lastOpenedProject val projectToOpen = @@ -397,47 +471,110 @@ class MainActivity : EdgeToEdgeIDEActivity() { } } - private fun handleOpenProject(root: File) { + // [deepLinkRequest] is the request this open is being performed FOR, threaded through from + // handleDeepLinkRequest (null for non-deep-link opens) the same way root/pendingFileRequest + // already are -- NOT re-read from latestDeepLinkRequest at consume time. That field tracks + // whatever request arrived most recently, so reading it when the user answers a confirm dialog + // could record a newer link B as consumed while actually opening this call's A, leaving A + // unconsumed (re-shown on the next recreate) and B silently dropped. + private fun handleOpenProject( + root: File, + pendingFileRequest: PendingFileRequest? = null, + deepLinkRequest: DeepLinkRequest? = null, + ) { if (GeneralPreferences.confirmProjectOpen) { - askProjectOpenPermission(root) + askProjectOpenPermission(root, pendingFileRequest, deepLinkRequest) return } - openProject(root) + // No confirmation gate -- opening happens immediately below, so this is "confirm time" for + // consumedDeepLinkRequests' purposes. + consumedDeepLinkRequests.add(deepLinkRequest) + openProject(root, pendingFileRequest = pendingFileRequest) } - private fun askProjectOpenPermission(root: File) { + // Tracked so a later overlapping request (e.g. two deep links arriving in quick succession while + // GeneralPreferences.confirmProjectOpen is enabled) dismisses the dialog already showing instead + // of stacking a second one underneath it -- letting both stack would let the user confirm the + // visible (later) one, then unknowingly tap the earlier one now exposed behind it, triggering a + // confusing second close-and-reopen inside the editor that just opened. Also dismissed in + // onDestroy() to avoid leaking its window. + private var activeOpenPermissionDialog: AlertDialog? = null + + // The request activeOpenPermissionDialog was raised for, so superseding or destroying the dialog + // can record THAT request rather than whichever one happens to be latest at the time. + private var activeOpenPermissionDialogRequest: DeepLinkRequest? = null + + // Whether activeOpenPermissionDialog (if any) came from a deep link -- see askProjectOpenPermission. + private var activeOpenPermissionDialogIsDeepLink = false + + private fun askProjectOpenPermission( + root: File, + pendingFileRequest: PendingFileRequest? = null, + deepLinkRequest: DeepLinkRequest? = null, + ) { + val isDeepLink = deepLinkRequest != null + // A deep link is an explicit, just-tapped user action and may always replace whatever's + // showing (including another deep link's own dialog, e.g. two links arriving in quick + // succession) -- but not the reverse: tryOpenLastProject's auto-open scan can complete + // moments after a deep link's dialog is already up, and silently yanking that away for an + // unrelated "open last project" prompt would be far more surprising than just dropping this + // slower, non-explicit request instead. + if (!isDeepLink && activeOpenPermissionDialogIsDeepLink && activeOpenPermissionDialog?.isShowing == true) { + return + } + // AlertDialog.dismiss() fires neither the negative button nor the OnCancel listener (and + // setCancelable(false) rules the latter out anyway), so the request the dialog being replaced + // was raised for would never be recorded. Its Intent is still the task's launch Intent, so any + // later recreate re-read it, found it unconsumed, and put the superseded dialog back up -- + // confirming it then switched the user to a project they had already moved past. Superseding a + // dialog IS an answer to it, so record it here. + if (activeOpenPermissionDialog?.isShowing == true) { + consumedDeepLinkRequests.add(activeOpenPermissionDialogRequest) + } + activeOpenPermissionDialog?.dismiss() + activeOpenPermissionDialogRequest = deepLinkRequest + activeOpenPermissionDialogIsDeepLink = isDeepLink val builder = DialogUtils.newMaterialDialogBuilder(this) builder.setTitle(string.title_confirm_open_project) builder.setMessage(getString(string.msg_confirm_open_project, root.absolutePath)) builder.setCancelable(false) - builder.setPositiveButton(string.yes) { _, _ -> openProject(root) } - builder.setNegativeButton(string.no, null) - builder.show() + builder.setPositiveButton(string.yes) { _, _ -> + // The user has now actually confirmed -- "confirm time" for consumedDeepLinkRequests' + // purposes, unlike merely having shown this dialog (see its own docs on why that + // distinction matters for a recreate that happens while this dialog is still up). + // deepLinkRequest, captured when this dialog was shown, is what gets recorded -- not + // latestDeepLinkRequest, which by answer time can already point at a NEWER link than the + // one this dialog was raised for (see handleOpenProject's doc). + consumedDeepLinkRequests.add(deepLinkRequest) + openProject(root, pendingFileRequest = pendingFileRequest) + } + // Consumed on decline too, not just on confirm. "No" is a decision the user made about this + // request, so leaving it unconsumed meant every later recreate of this activity -- a dark-mode + // toggle or a font-scale change, neither of which MainActivity declares in configChanges -- + // re-read the extra from the launch Intent and put the very same dialog back up, with no way + // to make it stop. + builder.setNegativeButton(string.no) { _, _ -> + consumedDeepLinkRequests.add(deepLinkRequest) + } + activeOpenPermissionDialog = builder.show() } internal fun openProject( root: File, project: RecentProject? = null, hasTemplateIssues: Boolean = false, + pendingFileRequest: PendingFileRequest? = null, ) { - ProjectManagerImpl.getInstance().projectPath = root.absolutePath - GeneralPreferences.lastOpenedProject = root.absolutePath + // Captured before the bookkeeping call below overwrites it: EditorHandlerActivity.onNewIntent + // (already-live singleTask instance) needs to know what project WAS open to tell a genuine + // switch from a same-project no-op, but recordProjectOpenedBookkeeping's synchronous + // ProjectManagerImpl.projectPath write below makes that global read the NEW path by the time + // onNewIntent runs -- comparing against it there would always see "already open". + val previousProjectPath = IProjectManager.getInstance().projectDirPath - lifecycleScope.launch(Dispatchers.IO) { - val location = root.absolutePath - val recentProject = - project ?: RecentProject( - name = root.name, - location = location, - createdAt = getCreatedTime(location).toString(), - lastModified = getLastModifiedTime(location).toString(), - language = readProjectLanguage(root), - ) - viewModel.saveProjectToRecents(recentProject) - } - - // Track project open in Firebase Analytics - analyticsManager.trackProjectOpened(root.absolutePath) + // Bookkeeping (Recents/analytics/lastOpenedProject) must run regardless of isFinishing -- + // only the startActivity() below is unsafe from a finishing activity. + recordProjectOpenedBookkeeping(recentProjectRepository, root, project, analyticsManager) if (isFinishing) { return @@ -445,10 +582,12 @@ class MainActivity : EdgeToEdgeIDEActivity() { val intent = Intent(this, EditorActivityKt::class.java).apply { - putExtra("PROJECT_PATH", root.absolutePath) + putExtra(EditorIntentExtras.EXTRA_PROJECT_PATH, root.absolutePath) + putExtra(EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH, previousProjectPath) if (hasTemplateIssues) { - putExtra("HAS_TEMPLATE_ISSUES", true) + putExtra(EditorIntentExtras.EXTRA_HAS_TEMPLATE_ISSUES, true) } + pendingFileRequest?.let { putExtra(PendingFileRequest.EXTRA_KEY, it) } addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP) } @@ -479,12 +618,121 @@ class MainActivity : EdgeToEdgeIDEActivity() { override fun onNewIntent(intent: Intent) { super.onNewIntent(intent) + setIntent(intent) + IntentCompat + .getParcelableExtra(intent, DeepLinkRequest.EXTRA_KEY, DeepLinkRequest::class.java) + // The consumed gate applies to a RE-FORWARD only. BaseEditorActivity bounces a request back + // here when a link names a project other than the one it holds, and without the gate an + // already-declined request had its dialog put straight back up. But gating every onNewIntent + // would be worse: DeepLinkRequest carries no nonce, so deliberately tapping the same URL + // again is equal by value to the earlier one, and an unconditional gate silently dropped it + // -- forever, for links naming a project that did not exist when first tapped. + ?.takeIf { + !intent.getBooleanExtra(EditorIntentExtras.EXTRA_REFORWARDED_DEEP_LINK, false) || + it !in consumedDeepLinkRequests || + // A request consumed only because its project could not be resolved never reached + // the editor, so this bounce cannot be the loop the gate guards against -- it is a + // fresh tap that happens to be value-equal to the earlier failure. Let it retry. + it in unresolvedDeepLinkRequests + }?.let { handleDeepLinkRequest(it) } + } + + /** + * Resolves [request]'s project name to an on-disk project directory and opens it -- called when + * [DeepLinkActivity] has already determined no project is currently loaded. + * + * This still goes through [handleOpenProject] rather than calling [openProject] directly, so an + * open arriving by link is gated the same way as one from the project list when the user has + * asked for that gate ([GeneralPreferences.confirmProjectOpen]). + * + * That preference is *not* what makes this extra safe to trust -- it defaults to `false`. The + * boundary is the manifest: [MainActivity] is not exported, so this extra can only have come + * from [DeepLinkActivity], which parsed and validated the URI it came from. It was previously + * exported despite declaring no intent-filter of its own (`SplashActivity` holds the actual + * MAIN/LAUNCHER), which let any co-installed app send this extra directly and force an arbitrary + * project open with no user interaction at all. `DeepLinkTargetsNotExportedTest` pins that. + */ + private fun handleDeepLinkRequest(request: DeepLinkRequest) { + // This attempt supersedes any earlier unresolved one for the same request. If it fails to + // resolve again the failure branch re-records it; if it succeeds the request must stop being + // exempt from the re-forward gate, or the bounce loop that gate exists to stop could resume. + unresolvedDeepLinkRequests.remove(request) + latestDeepLinkRequest = request + lifecycleScope.launch(Dispatchers.IO) { + val lookup = resolveDeepLinkProject(projectsRoot(), request.projectName) + withContext(Dispatchers.Main) { + // The activity may have started finishing while resolveDeepLinkProject was still + // scanning disk -- lifecycleScope only cancels at ON_DESTROY, not the moment isFinishing + // first flips true, so this continuation can otherwise still run and show a dialog on a + // dying window. + if (isFinishing || isDestroyed) return@withContext + if (lookup !is DeepLinkProjectLookup.Found) { + // The two are deliberately distinct events, matching the code's own split: a rise in + // PROJECT_NOT_FOUND means published links naming projects people do not have, while a + // rise in PROJECT_UNVERIFIABLE means storage trouble on the device. + analyticsManager.trackDeepLink( + DeepLinkMetric( + request.depth(), + if (lookup is DeepLinkProjectLookup.NotFound) { + DeepLinkOutcome.PROJECT_NOT_FOUND + } else { + DeepLinkOutcome.PROJECT_UNVERIFIABLE + }, + request.projectName, + ), + ) + // Consumed even though nothing opened: the project does not exist, so retrying on + // every recreate only re-shows "No project named X was found" indefinitely. Recorded + // only when this request is still the current one, so a superseded slow resolve + // cannot consume the newer request's slot. + // + // NotFound only. An Unverifiable result -- an EACCES straight after a + // storage-permission change, an EIO on a flaky SD/FUSE mount -- says nothing about + // whether the project exists, and recording it made a momentary filesystem failure + // silence a valid link on every later delivery (ADFA-5067 review). + if (lookup is DeepLinkProjectLookup.NotFound && latestDeepLinkRequest === request) { + consumedDeepLinkRequests.add(request) + // Tracked apart from the general consumed set so the re-forward gate in + // onNewIntent can let this request through again -- see the field's docs. + unresolvedDeepLinkRequests.add(request) + } + return@withContext + } + val projectDir = lookup.projectDir + // A second, faster-resolving deep link superseded this one while it was still resolving + // -- this stale, slower request must not now bounce the user back to its own (older) + // target after they've already been taken to the newer one. + if (latestDeepLinkRequest !== request) return@withContext + // the request is recorded as consumed once this request is actually opened (or confirmed, if + // GeneralPreferences.confirmProjectOpen is on) -- see handleOpenProject/ + // askProjectOpenPermission and consumedDeepLinkRequests' own docs for why marking it + // here, before the user has necessarily responded to that confirm dialog, would be too + // early. + handleOpenProject(projectDir, pendingFileRequest = request.fileRequest, deepLinkRequest = request) + } + } } override fun onDestroy() { webServer?.stop() ITemplateProvider.getInstance().release() + // Deliberately does NOT record the request consumed, unlike the supersession dismiss above. + // This dismiss is only about not leaking the window; if this is a config-change recreate the + // successor re-reads the launch Intent and should raise the dialog again, which is the + // behaviour a user who has answered nothing expects. + activeOpenPermissionDialog?.dismiss() super.onDestroy() _binding = null } + + override fun onSaveInstanceState(outState: Bundle) { + super.onSaveInstanceState(outState) + outState.putParcelableArrayList(KEY_CONSUMED_DEEP_LINK_REQUESTS, consumedDeepLinkRequests.toSavedList()) + outState.putParcelableArrayList(KEY_UNRESOLVED_DEEP_LINK_REQUESTS, unresolvedDeepLinkRequests.toSavedList()) + } + + companion object { + private const val KEY_CONSUMED_DEEP_LINK_REQUESTS = "consumedDeepLinkRequests" + private const val KEY_UNRESOLVED_DEEP_LINK_REQUESTS = "unresolvedDeepLinkRequests" + } } diff --git a/app/src/main/java/com/itsaky/androidide/activities/OnboardingActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/OnboardingActivity.kt index 385feadbcc..104ff5a066 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/OnboardingActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/OnboardingActivity.kt @@ -47,10 +47,10 @@ import com.itsaky.androidide.preferences.internal.prefManager import com.itsaky.androidide.tasks.doAsyncWithProgress import com.itsaky.androidide.ui.themes.IThemeManager import com.itsaky.androidide.utils.Environment -import com.itsaky.androidide.utils.isTestMode import com.itsaky.androidide.utils.PermissionsHelper import com.itsaky.androidide.utils.isAtLeastV import com.itsaky.androidide.utils.isSystemInDarkMode +import com.itsaky.androidide.utils.isTestMode import com.itsaky.androidide.utils.resolveAttr import com.termux.shared.android.PackageUtils import com.termux.shared.markdown.MarkdownUtils @@ -62,14 +62,13 @@ import kotlinx.coroutines.withContext import org.slf4j.LoggerFactory class OnboardingActivity : AppIntro2() { - private var listJdkInstallationsJob: Job? = null - private lateinit var feedbackButton: FloatingActionButton - private var feedbackButtonManager: FeedbackButtonManager? = null - private lateinit var nextButton: ImageButton - private lateinit var pulseAnimation: Animation + private lateinit var feedbackButton: FloatingActionButton + private var feedbackButtonManager: FeedbackButtonManager? = null + private lateinit var nextButton: ImageButton + private lateinit var pulseAnimation: Animation - companion object { + companion object { private val logger = LoggerFactory.getLogger(OnboardingActivity::class.java) private const val KEY_ARCHCONFIG_WARNING_IS_SHOWN = "ide.archConfig.experimentalWarning.isShown" @@ -103,14 +102,14 @@ class OnboardingActivity : AppIntro2() { setTransformer(AppIntroPageTransformerType.Fade) setProgressIndicator() showStatusBar(true) - setupFeedbackButton() + setupFeedbackButton() isIndicatorEnabled = true isWizardMode = true - nextButton = findViewById(R.id.next) - pulseAnimation = AnimationUtils.loadAnimation(this, R.anim.pulse_animation) + nextButton = findViewById(R.id.next) + pulseAnimation = AnimationUtils.loadAnimation(this, R.anim.pulse_animation) - addSlide(GreetingFragment()) + addSlide(GreetingFragment()) if (!PackageUtils.isCurrentUserThePrimaryUser(this)) { val errorMessage = @@ -161,44 +160,54 @@ class OnboardingActivity : AppIntro2() { } } - private fun setupFeedbackButton() { - val contentRootView = findViewById(android.R.id.content) - contentRootView.viewTreeObserver.addOnGlobalLayoutListener(object : - ViewTreeObserver.OnGlobalLayoutListener { - override fun onGlobalLayout() { - contentRootView.viewTreeObserver.removeOnGlobalLayoutListener(this) - - val appIntroContainer: ConstraintLayout? = findViewById(R.id.background) - if (appIntroContainer != null) { - // Reuse the shared feedback FAB definition (size, icon, elevation) so this - // matches every other screen (ADFA-2686); only positioning is set here. - feedbackButton = (layoutInflater.inflate( - R.layout.feedback_fab, appIntroContainer, false - ) as FloatingActionButton).apply { - layoutParams = ConstraintLayout.LayoutParams( - ConstraintLayout.LayoutParams.WRAP_CONTENT, - ConstraintLayout.LayoutParams.WRAP_CONTENT - ).apply { - startToStart = ConstraintLayout.LayoutParams.PARENT_ID - bottomToBottom = ConstraintLayout.LayoutParams.PARENT_ID - val marginInPx = - resources.getDimensionPixelSize(R.dimen.feedback_fab_margin) - setMargins(marginInPx, marginInPx, marginInPx, marginInPx) - } - } - - appIntroContainer.addView(feedbackButton) - feedbackButtonManager = FeedbackButtonManager( - activity = this@OnboardingActivity, - feedbackFab = feedbackButton - ) - feedbackButtonManager?.setupDraggableFab() - } else { - logger.error("Could not find AppIntro2 container to add FAB.") - } - } - }) - } + private fun setupFeedbackButton() { + val contentRootView = findViewById(android.R.id.content) + contentRootView.viewTreeObserver.addOnGlobalLayoutListener( + object : + ViewTreeObserver.OnGlobalLayoutListener { + override fun onGlobalLayout() { + contentRootView.viewTreeObserver.removeOnGlobalLayoutListener(this) + + val appIntroContainer: ConstraintLayout? = findViewById(R.id.background) + if (appIntroContainer != null) { + // Reuse the shared feedback FAB definition (size, icon, elevation) so this + // matches every other screen (ADFA-2686); only positioning is set here. + feedbackButton = + ( + layoutInflater.inflate( + R.layout.feedback_fab, + appIntroContainer, + false, + ) as FloatingActionButton + ).apply { + layoutParams = + ConstraintLayout + .LayoutParams( + ConstraintLayout.LayoutParams.WRAP_CONTENT, + ConstraintLayout.LayoutParams.WRAP_CONTENT, + ).apply { + startToStart = ConstraintLayout.LayoutParams.PARENT_ID + bottomToBottom = ConstraintLayout.LayoutParams.PARENT_ID + val marginInPx = + resources.getDimensionPixelSize(R.dimen.feedback_fab_margin) + setMargins(marginInPx, marginInPx, marginInPx, marginInPx) + } + } + + appIntroContainer.addView(feedbackButton) + feedbackButtonManager = + FeedbackButtonManager( + activity = this@OnboardingActivity, + feedbackFab = feedbackButton, + ) + feedbackButtonManager?.setupDraggableFab() + } else { + logger.error("Could not find AppIntro2 container to add FAB.") + } + } + }, + ) + } override fun onResume() { super.onResume() @@ -221,27 +230,32 @@ class OnboardingActivity : AppIntro2() { tryNavigateToMainIfSetupIsCompleted() } - fun setOnboardingChromeVisible(visible: Boolean) { - isIndicatorEnabled = visible - isButtonsEnabled = visible - } + fun setOnboardingChromeVisible(visible: Boolean) { + isIndicatorEnabled = visible + isButtonsEnabled = visible + } override fun onPageSelected(position: Int) { super.onPageSelected(position) - when { - !nextButton.isVisible -> nextButton.clearAnimation() - !isTestMode() && nextButton.animation == null -> nextButton.startAnimation(pulseAnimation) - } + when { + !nextButton.isVisible -> nextButton.clearAnimation() + !isTestMode() && nextButton.animation == null -> nextButton.startAnimation(pulseAnimation) + } } private fun checkToolsIsInstalled(): Boolean = IJdkDistributionProvider.getInstance().installedDistributions.isNotEmpty() && Environment.ANDROID_HOME.exists() + // Deliberately the provider's loaded list, not isIdeSetupComplete()'s on-disk check: this screen + // can afford to wait for a validated JDK (it calls loadDistributions() itself when the list is + // empty, a few lines below) and should not hand over to MainActivity until one is really usable. + // The deep-link gate asks the weaker on-disk question because it runs on a cold-start main + // thread and cannot wait -- see SetupState.kt. private fun isSetupCompleted(): Boolean = checkToolsIsInstalled() && - PermissionsHelper.areAllPermissionsGranted(this) + PermissionsHelper.areAllPermissionsGranted(this) internal fun navigateToMain() { startActivity(Intent(this, MainActivity::class.java)) @@ -258,16 +272,16 @@ class OnboardingActivity : AppIntro2() { } private suspend fun reloadJdkDistInfo(distConsumer: (List) -> Unit) { - val distributionProvider = IJdkDistributionProvider.getInstance() - val currentDistributions = distributionProvider.installedDistributions - if (currentDistributions.isNotEmpty()) { - distConsumer(currentDistributions) - return - } + val distributionProvider = IJdkDistributionProvider.getInstance() + val currentDistributions = distributionProvider.installedDistributions + if (currentDistributions.isNotEmpty()) { + distConsumer(currentDistributions) + return + } - if (listJdkInstallationsJob?.isActive == true) { - return - } + if (listJdkInstallationsJob?.isActive == true) { + return + } listJdkInstallationsJob = doAsyncWithProgress( @@ -278,10 +292,10 @@ class OnboardingActivity : AppIntro2() { ) { _, _ -> distributionProvider.loadDistributions() withContext(Dispatchers.Main) { - if (!isFinishing && !isDestroyed) { - distConsumer(distributionProvider.installedDistributions) - } - } + if (!isFinishing && !isDestroyed) { + distConsumer(distributionProvider.installedDistributions) + } + } }.also { it?.invokeOnCompletion { listJdkInstallationsJob = null diff --git a/app/src/main/java/com/itsaky/androidide/activities/SetupState.kt b/app/src/main/java/com/itsaky/androidide/activities/SetupState.kt new file mode 100644 index 0000000000..ae26481dab --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/activities/SetupState.kt @@ -0,0 +1,120 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.activities + +import android.content.Context +import androidx.annotation.VisibleForTesting +import com.itsaky.androidide.utils.Environment +import com.itsaky.androidide.utils.FileUtil +import com.itsaky.androidide.utils.PermissionsHelper +import java.io.File + +/** + * Whether the IDE has everything it needs to open a project: a JDK, an SDK, and the permissions to + * reach them. + * + * Asked by [DeepLinkActivity], because a link arriving before setup finishes would otherwise walk + * straight past onboarding into an editor with no toolchain (ADFA-5067 review). + * + * OnboardingActivity deliberately does *not* share this: it asks the stricter question -- a JDK the + * provider has loaded and validated -- because it can afford to wait for one and must not hand over + * to MainActivity until the toolchain is really usable. This one has to answer on a cold-start main + * thread, where nothing has loaded yet, so it asks what is on disk. Two questions, not two copies of + * one question. + * + * Deliberately *not* the whole of what [SplashActivity] enforces -- free storage and the x86 exit are + * its business, and a caller that finds this false should send the user there rather than re-deciding + * any of it. + * + * The consequence, which is easy to miss: SplashActivity is reached only when this answers FALSE, so + * whenever it answers true a deep link goes straight to the editor and Splash's low-storage dialog + * and x86 `finishAffinity()` never run -- nor OnboardingActivity's primary-user, SD-card-install and + * device-supported checks. A device that finished onboarding and has since filled its storage is + * stopped by Splash on a normal launch and waved through by a link, landing in an editor whose Gradle + * build then fails for want of disk. That is the accepted trade (a link must not re-run onboarding), + * but it means any check added to Splash or Onboarding is silently NOT applied to links: a new one + * that must cover them has to be added here too, or hoisted somewhere both paths share. + */ +internal fun Context.isIdeSetupComplete(): Boolean = + isJdkInstalled() && + androidSdkHome().exists() && + PermissionsHelper.areAllPermissionsGranted(this) + +/** + * Whether a JDK is present *on disk*, which is not the same question as whether one has been loaded + * into memory yet. + * + * The provider's `installedDistributions` is the obvious thing to ask, and it is wrong + * here: it returns an empty list until `loadDistributions()` has run, and that happens inside the + * loader coroutine `IDEApplication` launches on `Dispatchers.Default`. On a cold start an Activity's + * `onCreate` reaches the main thread first, so asking the provider says "no JDK" on a device that + * has one -- which for [DeepLinkActivity] meant discarding the link and telling the user to finish a + * setup they had already finished (ADFA-5067 review). + * + * So this reads the same directory `JdkUtils.findJavaInstallations` scans, and only that: one stat + * and one listing, cheap enough for the main thread, and true as soon as the bootstrap has unpacked + * regardless of what has been loaded. It deliberately does not validate the installations -- that is + * the provider's job once it runs, and a directory that exists but holds nothing usable is a broken + * install, not an unfinished setup. + */ +private fun isJdkInstalled(): Boolean { + val jvmDir = File(jdkInstallPrefix(), "lib/jvm") + return jvmDir.isDirectory && (jvmDir.list()?.isNotEmpty() == true) +} + +/** + * [Environment.PREFIX], or the same path it will hold once `Environment.init()` has run. + * + * `init()` runs inside the same unawaited loader coroutine that loads the JDK distributions (see + * [isJdkInstalled]), so on a cold start this main-thread read routinely happens first and finds the + * field still null -- and the field is not volatile, so even a completed `init()` guarantees nothing + * about visibility here (ADFA-5067 review). `File(null, "lib/jvm")` doesn't throw; it silently + * yields a *relative* path that exists nowhere, answering "not set up" on a fully set-up device and + * discarding the deep link. `init()` derives the field from constants (`new File(DEFAULT_ROOT)` + * then `"usr"`), and [Environment.DEFAULT_PREFIX] is that same path, so falling back to it reads + * the same directory without waiting on the loader. The field is still preferred when visible -- + * it is what the rest of the app uses, and tests redirect it to a temp dir. + */ +@VisibleForTesting +internal fun jdkInstallPrefix(): File = Environment.PREFIX ?: File(Environment.DEFAULT_PREFIX) + +/** + * [Environment.ANDROID_HOME], with the same pre-`Environment.init()` fallback as + * [jdkInstallPrefix]. Needed for the same race: before the fix in [jdkInstallPrefix], the only + * thing keeping [isIdeSetupComplete] from an NPE on this field was [isJdkInstalled] short-circuiting + * to false first. The literal mirrors [Environment]'s private `DEFAULT_ANDROID_HOME` + * (`DEFAULT_HOME + "/android-sdk"`), which `init()` assigns verbatim. + */ +@VisibleForTesting +internal fun androidSdkHome(): File = Environment.ANDROID_HOME ?: File(Environment.DEFAULT_HOME, "android-sdk") + +/** + * [Environment.PROJECTS_DIR], with the same pre-`Environment.init()` fallback as + * [jdkInstallPrefix] and [androidSdkHome]. + * + * The third field assigned by that same unawaited loader coroutine, and the one the sweep missed. + * It is worse than the other two here: they feed [isIdeSetupComplete], which merely answers "not + * set up" when they read null, but this one is handed straight to `resolveDeepLinkProject`'s + * non-null `projectsRoot` parameter -- so a null is not a wrong answer, it is + * `NullPointerException: Parameter specified as non-null is null` thrown inside a + * `lifecycleScope.launch` with no handler, i.e. the process dying when the user taps a link during + * a cold start. `init()` derives the field as `new File(FileUtil.getExternalStorageDir(), + * PROJECTS_FOLDER)`, which is what this reconstructs. + */ +@VisibleForTesting +internal fun projectsRoot(): File = Environment.PROJECTS_DIR ?: File(FileUtil.getExternalStorageDir(), Environment.PROJECTS_FOLDER) diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt index 2d8f88dc70..d1957709da 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt @@ -54,7 +54,9 @@ import androidx.annotation.UiThread import androidx.appcompat.app.ActionBarDrawerToggle import androidx.collection.MutableIntIntMap import androidx.core.content.ContextCompat +import androidx.core.content.IntentCompat import androidx.core.graphics.Insets +import androidx.core.os.BundleCompat import androidx.core.view.GravityCompat import androidx.core.view.ViewCompat import androidx.core.view.WindowInsetsCompat @@ -80,6 +82,7 @@ import com.itsaky.androidide.R import com.itsaky.androidide.R.string import com.itsaky.androidide.actions.build.DebugAction import com.itsaky.androidide.activities.MainActivity +import com.itsaky.androidide.activities.projectsRoot import com.itsaky.androidide.adapters.DiagnosticsAdapter import com.itsaky.androidide.adapters.SearchListAdapter import com.itsaky.androidide.api.BuildOutputProvider @@ -88,6 +91,7 @@ import com.itsaky.androidide.app.IDEApplication import com.itsaky.androidide.databinding.ActivityEditorBinding import com.itsaky.androidide.databinding.ContentEditorBinding import com.itsaky.androidide.databinding.LayoutDiagnosticInfoBinding +import com.itsaky.androidide.deeplink.ConsumedRequests import com.itsaky.androidide.events.InstallationEvent import com.itsaky.androidide.fragments.debug.DebuggerFragment import com.itsaky.androidide.fragments.output.ShareableOutputFragment @@ -102,8 +106,11 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.interfaces.DiagnosticClickListener import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.models.DiagnosticItem +import com.itsaky.androidide.models.DeepLinkRequest import com.itsaky.androidide.models.DiagnosticGroup +import com.itsaky.androidide.models.EditorIntentExtras import com.itsaky.androidide.models.OpenedFile +import com.itsaky.androidide.models.PendingFileRequest import com.itsaky.androidide.models.Range import com.itsaky.androidide.models.SearchResult import com.itsaky.androidide.plugins.extensions.FileTabMenuItem @@ -137,6 +144,7 @@ import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashMessage import com.itsaky.androidide.utils.getOrStoreInitialPadding import com.itsaky.androidide.utils.isAtLeastR +import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject import com.itsaky.androidide.utils.resolveAttr import com.itsaky.androidide.viewmodel.ApkInstallationViewModel import com.itsaky.androidide.viewmodel.AppLogsCoordinator @@ -211,6 +219,57 @@ abstract class BaseEditorActivity : val appLogsViewModel by viewModels() var appLogsCoordinator: AppLogsCoordinator? = null + // Mirrors EditorHandlerActivity's/ProjectHandlerActivity's own same-named, independently-tracked + // flags: set only once onCreate reaches its end without bailing out early (the "no matching + // project" doomed-duplicate-instance branch above returns before this runs). preDestroy() checks + // it before touching the process-wide singletons this onCreate registers this instance with + // (BuildOutputProvider, the plugin snippet-refresh listener) -- a doomed instance never actually + // registered as their owner, so clearing them on its teardown would wipe out whatever a + // genuinely live sibling instance set up instead. + private var didCompleteLiveOnCreate = false + + // The editor-side counterpart of MainActivity.consumedDeepLinkRequests, for the two deep-link + // extras this activity consumes from its intent (DeepLinkRequest in onCreate/onNewIntent, + // PendingFileRequest in postProjectInit): intent.removeExtra only mutates this process's Intent + // object, so after process death the system re-creates this activity from the *parceled* intent + // with the extras still on it, and the drained request would fire again with no user action -- + // yanking the editor back to a file/line the user navigated away from before the kill. Persisted + // via onSaveInstanceState (restored at the top of onCreate) so consumption survives exactly the + // recreate paths removeExtra cannot cover. + protected val consumedDeepLinkRequests = ConsumedRequests() + protected val consumedFileRequests = ConsumedRequests() + + /** + * Arms [request] on [target] for [postProjectInit]'s deferred read, un-marking it as consumed + * first: a deliberately re-armed request (e.g. the same file/line navigation requested a second + * time, parked mid-sync) must not be skipped by the consumed-check just because an equal-by-value + * request was applied earlier. Every putExtra of this key onto a live activity's own intent goes + * through here so arming and the consumed bookkeeping cannot drift apart. + */ + protected fun armPendingFileRequest( + target: Intent, + request: PendingFileRequest, + ) { + consumedFileRequests.remove(request) + target.putExtra(PendingFileRequest.EXTRA_KEY, request) + } + + /** + * Removes the pending file request from this activity's intent and records it as consumed (see + * [consumedFileRequests] for why removal alone is not durable). Returns the request when it was + * still pending, or `null` when there was none or it had already been consumed -- i.e. the intent + * carrying it is a post-process-death redelivery, not a new navigation. + */ + protected fun drainPendingFileRequest(): PendingFileRequest? { + val request = + IntentCompat.getParcelableExtra(intent, PendingFileRequest.EXTRA_KEY, PendingFileRequest::class.java) + ?: return null + val alreadyConsumed = request in consumedFileRequests + consumedFileRequests.add(request) + intent.removeExtra(PendingFileRequest.EXTRA_KEY) + return request.takeUnless { alreadyConsumed } + } + @Suppress("ktlint:standard:backing-property-naming") internal var _binding: ActivityEditorBinding? = null val binding: ActivityEditorBinding @@ -439,6 +498,8 @@ abstract class BaseEditorActivity : const val EDITOR_CONTAINER_SCALE_FACTOR = 0.87f const val KEY_BOTTOM_SHEET_SHOWN = "editor_bottomSheetShown" const val KEY_PROJECT_PATH = "saved_projectPath" + private const val KEY_CONSUMED_DEEP_LINK_REQUESTS = "saved_consumedDeepLinkRequests" + private const val KEY_CONSUMED_FILE_REQUESTS = "saved_consumedFileRequests" } protected abstract fun provideCurrentEditor(): CodeEditorView? @@ -459,9 +520,11 @@ abstract class BaseEditorActivity : internal abstract fun doOpenHelp() protected open fun preDestroy() { - BuildOutputProvider.clearBottomSheet() + if (didCompleteLiveOnCreate) { + BuildOutputProvider.clearBottomSheet() - IDEApplication.getPluginManager()?.setSnippetRefreshListener(null) + IDEApplication.getPluginManager()?.setSnippetRefreshListener(null) + } Shizuku.removeBinderReceivedListener(shizukuBinderReceivedListener) if (isAtLeastR()) wadbConnectionViewModel.stop(this) @@ -508,7 +571,13 @@ abstract class BaseEditorActivity : } protected open fun postDestroy() { - if (isDestroying) { + // didCompleteLiveOnCreate as well as isDestroying, for the same reason preDestroy above and + // both of ProjectHandlerActivity's teardown hooks carry it -- and it matters more here, because + // everything below is process-wide rather than per-instance. An instance whose onCreate took the + // deep-link `deepLinkTargetsAnotherProject` bail (finish() + return) never registered any of + // this, but it *is* finishing, so isDestroying alone let it unregister the Lookup and clear all + // three registries out from under the live sibling that did register them. + if (didCompleteLiveOnCreate && isDestroying) { Lookup.getDefault().unregisterAll() ApiVersionsRegistry.getInstance().clear() ResourceTableRegistry.getInstance().clear() @@ -653,14 +722,50 @@ abstract class BaseEditorActivity : * building the editor UI. */ override fun onCreate(savedInstanceState: Bundle?) { + // Restored before the deep-link reads below: a savedInstanceState means this is a recreate + // (config change or post-process-death), and post-process-death the intent read next still + // carries every extra this task already consumed -- see consumedDeepLinkRequests' docs. + consumedDeepLinkRequests.restore( + savedInstanceState?.let { + BundleCompat.getParcelableArrayList(it, KEY_CONSUMED_DEEP_LINK_REQUESTS, DeepLinkRequest::class.java) + }, + ) + consumedFileRequests.restore( + savedInstanceState?.let { + BundleCompat.getParcelableArrayList(it, KEY_CONSUMED_FILE_REQUESTS, PendingFileRequest::class.java) + }, + ) + + // DeepLinkActivity routes a deep link to this activity's class only when it believes a live + // singleTask instance already exists to handle it via onNewIntent (see + // ActionContextProvider.getLiveActivity()'s docs on how that check can still be stale) -- if + // Android instead spins up a genuinely new instance, this onCreate runs and onNewIntent + // never does, so this is EXTRA_KEY's only other reader on the editor side. An + // already-consumed request is treated as absent: it can only be here again because a + // post-process-death recreate redelivered the parceled intent verbatim, and acting on it + // again would bounce the user back into a project switch they already performed (or + // abandoned) before the kill. + val deepLinkRequest = + IntentCompat + .getParcelableExtra(intent, DeepLinkRequest.EXTRA_KEY, DeepLinkRequest::class.java) + ?.takeUnless { it in consumedDeepLinkRequests } + // The OS can recreate EditorActivity after process death without routing through // MainActivity, leaving the ProjectManagerImpl singleton's lateinit projectPath unset. - // Restore it from the saved state, the launch intent, or the last opened project. - val restoredProjectPath = + // Restore it from the saved state or the launch intent; only fall back to the last opened + // project when there's no pending deep link -- otherwise this would silently open the wrong + // project instead of the one the link actually requested. + val explicitProjectPath = savedInstanceState?.getString(KEY_PROJECT_PATH)?.takeIf { it.isNotBlank() } - ?: intent?.getStringExtra("PROJECT_PATH")?.takeIf { it.isNotBlank() } - ?: GeneralPreferences.lastOpenedProject - .takeIf { it.isNotBlank() && it != GeneralPreferences.NO_OPENED_PROJECT } + ?: intent?.getStringExtra(EditorIntentExtras.EXTRA_PROJECT_PATH)?.takeIf { it.isNotBlank() } + val restoredProjectPath = + explicitProjectPath + ?: if (deepLinkRequest == null) { + GeneralPreferences.lastOpenedProject + .takeIf { it.isNotBlank() && it != GeneralPreferences.NO_OPENED_PROJECT } + } else { + null + } if (restoredProjectPath != null) { ProjectManagerImpl.getInstance().projectPath = restoredProjectPath } @@ -668,14 +773,58 @@ abstract class BaseEditorActivity : // If we still have no project path after every fallback, we cannot safely build the // editor UI (setupToolbar -> getProjectName dereferences the project path). Route the - // user back to MainActivity instead of crashing. - if (ProjectManagerImpl.getInstance().projectDirPath.isBlank()) { - log.warn("No project path available in EditorActivity.onCreate(); returning to MainActivity") - startActivity(Intent(this, MainActivity::class.java)) + // user back to MainActivity instead of crashing -- forwarding a pending deep link along so + // MainActivity can still resolve and open the requested project, instead of silently + // dropping it here. + // + // A deep link also forces this even when a project path IS already loaded: DeepLinkActivity + // routes here only when it believes a live instance already exists to handle the request via + // onNewIntent, but that check can be stale (see ActionContextProvider.getLiveActivity()'s docs) + // -- Android may spin up this genuinely new instance instead, which inherits whatever project + // ProjectManagerImpl's process-wide singleton was last holding, not necessarily the one this + // deep link actually targets. Comparing against the project directory's name (matching how + // projects live directly under Environment.PROJECTS_DIR) catches that mismatch without an + // extra disk scan. + val projectDirPath = ProjectManagerImpl.getInstance().projectDirPath + val deepLinkTargetsAnotherProject = + deepLinkRequest != null && + !isDeepLinkTargetOfOpenProject(projectDirPath, deepLinkRequest.projectName, projectsRoot()) + if (projectDirPath.isBlank() || deepLinkTargetsAnotherProject) { + log.warn("No matching project available in EditorActivity.onCreate(); returning to MainActivity") + startActivity( + Intent(this, MainActivity::class.java).apply { + deepLinkRequest?.let { + putExtra(DeepLinkRequest.EXTRA_KEY, it) + // Marks this as a re-delivery rather than a fresh tap, so MainActivity applies its + // consumed gate here and only here. + putExtra(EditorIntentExtras.EXTRA_REFORWARDED_DEEP_LINK, true) + } + // This branch is reachable far more often now (any deepLinkTargetsAnotherProject + // mismatch, not just a rare cold process-death recreate) -- without CLEAR_TOP, a + // MainActivity instance already lower in the back stack (Main -> Open Project -> + // Editor) would get a stacked duplicate instead of being reused, leaving back-press + // landing on the stale earlier instance instead of exiting. + addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP) + }, + ) finish() return } + // The deep link's project already matches what's loaded (a stale liveness check spun up this + // new instance instead of redelivering via onNewIntent) -- forward its file/line/column + // request through the normal PendingFileRequest pipeline so postProjectInit still applies it + // once the project finishes initializing, instead of silently dropping it here. + deepLinkRequest?.fileRequest?.let { armPendingFileRequest(intent, it) } + // Consumed here -- mirror EditorHandlerActivity.onNewIntent's own drain of this same extra, + // so a launch intent redelivered verbatim after process death doesn't re-navigate to the + // same file/line a second time. Recorded in consumedDeepLinkRequests too: the removeExtra + // alone doesn't survive process death (see that field's docs). + deepLinkRequest?.let { + consumedDeepLinkRequests.add(it) + intent.removeExtra(DeepLinkRequest.EXTRA_KEY) + } + editorViewModel.isBuildInProgress = false editorViewModel.isInitializing = false @@ -755,6 +904,8 @@ abstract class BaseEditorActivity : observeFileOperations() setupGestureDetector() + + didCompleteLiveOnCreate = true } override fun onConfigurationChanged(newConfig: Configuration) { @@ -984,8 +1135,25 @@ abstract class BaseEditorActivity : postDestroy() } + /** + * The project path [onSaveInstanceState] persists, and therefore the project a recreate reopens. + * + * Open because the live [IProjectManager] global is not always the right answer: while a project + * switch is proposed but not yet confirmed, `MainActivity.openProject`'s bookkeeping has already + * moved that global to the *incoming* project, so saving it would hand the successor a project + * the user has not agreed to open -- against a retained ViewModel still holding the previous + * project's tabs and buffers. [EditorHandlerActivity] overrides this to name the project that is + * actually staying open (ADFA-5067 review). + */ + protected open val projectPathForInstanceState: String + get() = IProjectManager.getInstance().projectDirPath + override fun onSaveInstanceState(outState: Bundle) { - outState.putString(KEY_PROJECT_PATH, IProjectManager.getInstance().projectDirPath) + outState.putString(KEY_PROJECT_PATH, projectPathForInstanceState) + // See consumedDeepLinkRequests' docs: a post-process-death recreate is handed the parceled + // intent with already-drained extras still on it, and these are what stop them re-firing. + outState.putParcelableArrayList(KEY_CONSUMED_DEEP_LINK_REQUESTS, consumedDeepLinkRequests.toSavedList()) + outState.putParcelableArrayList(KEY_CONSUMED_FILE_REQUESTS, consumedFileRequests.toSavedList()) super.onSaveInstanceState(outState) } diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt index c3f187e2dd..c8aa83ab93 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt @@ -30,7 +30,9 @@ import android.view.View import android.view.ViewGroup.LayoutParams import android.widget.TextView import androidx.annotation.VisibleForTesting +import androidx.appcompat.app.AlertDialog import androidx.collection.MutableIntObjectMap +import androidx.core.content.IntentCompat import androidx.core.content.res.ResourcesCompat import androidx.core.view.GravityCompat import androidx.core.view.doOnNextLayout @@ -49,6 +51,11 @@ import com.itsaky.androidide.actions.ActionsRegistry.Companion.getInstance import com.itsaky.androidide.actions.build.QuickRunAction import com.itsaky.androidide.actions.internal.DefaultActionsRegistry import com.itsaky.androidide.activities.PluginManagerActivity +import com.itsaky.androidide.activities.projectsRoot +import com.itsaky.androidide.analytics.DeepLinkMetric +import com.itsaky.androidide.analytics.DeepLinkOutcome +import com.itsaky.androidide.analytics.IAnalyticsManager +import com.itsaky.androidide.analytics.depth import com.itsaky.androidide.api.ActionContextProvider import com.itsaky.androidide.app.BaseApplication import com.itsaky.androidide.app.EditorEvents @@ -56,6 +63,8 @@ import com.itsaky.androidide.app.EditorProviderImpl import com.itsaky.androidide.app.IDEApplication import com.itsaky.androidide.databinding.FileActionPopupWindowBinding import com.itsaky.androidide.databinding.FileActionPopupWindowItemBinding +import com.itsaky.androidide.deeplink.PendingDeepLinkOpen +import com.itsaky.androidide.di.APPLICATION_SCOPE import com.itsaky.androidide.editor.language.treesitter.JavaLanguage import com.itsaky.androidide.editor.language.treesitter.JsonLanguage import com.itsaky.androidide.editor.language.treesitter.KotlinLanguage @@ -72,9 +81,14 @@ import com.itsaky.androidide.fragments.sidebar.EditorSidebarFragment import com.itsaky.androidide.idetooltips.TooltipManager import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.interfaces.IEditorHandler +import com.itsaky.androidide.models.DeepLinkOpenRequest +import com.itsaky.androidide.models.DeepLinkRequest +import com.itsaky.androidide.models.EditorIntentExtras import com.itsaky.androidide.models.FileExtension import com.itsaky.androidide.models.OpenedFile import com.itsaky.androidide.models.OpenedFilesCache +import com.itsaky.androidide.models.PendingFileRequest +import com.itsaky.androidide.models.Position import com.itsaky.androidide.models.Range import com.itsaky.androidide.models.SaveResult import com.itsaky.androidide.plugins.manager.build.PluginBuildActionManager @@ -84,25 +98,38 @@ import com.itsaky.androidide.plugins.manager.ui.PluginEditorTabManager import com.itsaky.androidide.plugins.manager.ui.PluginToolbarHost import com.itsaky.androidide.plugins.manager.ui.PluginUiActionManager import com.itsaky.androidide.preferences.internal.EditorPreferences +import com.itsaky.androidide.preferences.internal.GeneralPreferences +import com.itsaky.androidide.projects.IProjectManager import com.itsaky.androidide.projects.ProjectManagerImpl import com.itsaky.androidide.projects.builder.BuildResult +import com.itsaky.androidide.repositories.RecentProjectRepository import com.itsaky.androidide.shortcuts.IdeShortcutActions import com.itsaky.androidide.shortcuts.ShortcutContext import com.itsaky.androidide.shortcuts.ShortcutExecutionContext import com.itsaky.androidide.shortcuts.ShortcutManager import com.itsaky.androidide.tasks.executeAsync +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult +import com.itsaky.androidide.ui.ARCHIVE_EXTENSIONS import com.itsaky.androidide.ui.CodeEditorView +import com.itsaky.androidide.utils.DeepLinkProjectLookup import com.itsaky.androidide.utils.DialogUtils.newMaterialDialogBuilder import com.itsaky.androidide.utils.DialogUtils.showConfirmationDialog import com.itsaky.androidide.utils.EditorActivityActions import com.itsaky.androidide.utils.EditorSidebarActions +import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.ImageUtils import com.itsaky.androidide.utils.IntentUtils.openImage import com.itsaky.androidide.utils.UniqueNameBuilder +import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.utils.forEachViewRecursively import com.itsaky.androidide.utils.hasVisibleDialog +import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject +import com.itsaky.androidide.utils.recordProjectOpenedBookkeeping +import com.itsaky.androidide.utils.resolveDeepLinkProject +import com.itsaky.androidide.utils.resolveWithinDirectory import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.launch @@ -110,6 +137,8 @@ import kotlinx.coroutines.withContext import org.adfa.constants.CONTENT_KEY import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode +import org.koin.android.ext.android.inject +import org.koin.core.qualifier.named import java.io.File import java.util.WeakHashMap import java.util.concurrent.ConcurrentHashMap @@ -159,8 +188,21 @@ open class EditorHandlerActivity : } private val shortcutManager by lazy { ShortcutManager(applicationContext) } + private val analyticsManager: IAnalyticsManager by inject() + private val recentProjectRepository: RecentProjectRepository by inject() + private val pendingDeepLinkOpen: PendingDeepLinkOpen by inject() + + // The process-wide scope from AppModule, used only by saveAllAsync -- see there for why the + // activity's own scope is not enough. + private val appScope: CoroutineScope by inject(named(APPLICATION_SCOPE)) + private var pluginEditorProvider: EditorProviderImpl? = null + // True once onCreate() has completed past its isFinishing check -- see there and preDestroy() + // for why a doomed, finishing-from-birth instance must not run teardown meant only for an + // instance that actually became the live one. + private var didCompleteLiveOnCreate = false + private fun getTabPositionForFileIndex(fileIndex: Int): Int { val safeContent = contentOrNull ?: return -1 val totalTabs = safeContent.tabs.tabCount @@ -212,10 +254,23 @@ open class EditorHandlerActivity : override fun preDestroy() { super.preDestroy() - TSLanguageRegistry.instance.destroy() + // TSLanguageRegistry.instance is a process-wide singleton whose own KDoc says destroy() "must + // be called only when the application is exiting" -- guarded on didCompleteLiveOnCreate (same + // reasoning as pluginEditorProvider below) so a doomed instance, spun up and finishing before + // its onCreate() ever got this far, can't tear down the registry a different, actually-live + // sibling instance still depends on for syntax highlighting. + if (didCompleteLiveOnCreate) { + TSLanguageRegistry.instance.destroy() + } editorViewModel.removeAllFiles() - IDEApplication.getPluginManager()?.setEditorProvider(null) + // Guarded on pluginEditorProvider (rather than unconditional) so an instance whose onCreate() + // returned early because it was already finishing (see onCreate()) -- and which therefore + // never registered a provider of its own -- can't null out a DIFFERENT, actually-live + // instance's provider out from under it during its own teardown. + if (pluginEditorProvider != null) { + IDEApplication.getPluginManager()?.setEditorProvider(null) + } pluginEditorProvider?.dispose() pluginEditorProvider = null } @@ -230,6 +285,29 @@ open class EditorHandlerActivity : mBuildEventListener.setActivity(this) super.onCreate(savedInstanceState) + // BaseEditorActivity.onCreate() (just run via super.onCreate() above) may have already called + // finish() -- e.g. this instance was spun up for a deep link whose project doesn't match what + // it holds, or with no project path at all -- and returned; finish() doesn't stop execution + // from continuing here. Without this check, the registrations below would unconditionally + // clobber process-wide singleton state (ActionContextProvider, the plugin editor provider) + // away from whatever OTHER, actually-live instance currently owns it, with nothing to ever + // restore it once this doomed instance is eventually torn down. + if (isFinishing) { + return + } + didCompleteLiveOnCreate = true + + // Registered here (right after super.onCreate() finishes wiring the toolbar/action registry), + // not just onResume (see there too), so this instance is discoverable via + // ActionContextProvider.getLiveActivity() for almost its whole lifetime -- see that + // function's docs for the redundant-open race a gap between onCreate and onResume otherwise + // leaves open. + // Registering before super.onCreate() returns would instead expose a partially-constructed + // activity (no toolbar/action registry yet) to external callers like a floating + // EditorPanelDockableContent window, which is explicitly documented to outlive this activity + // and can act on it at any time. + ActionContextProvider.setActivity(this) + supportFragmentManager.registerFragmentLifecycleCallbacks(pluginFontScalingListener, true) floatingTabController.start() @@ -327,13 +405,137 @@ open class EditorHandlerActivity : Log.d("EditorHandlerActivity", "Saved open plugin tabs: $openPluginTabIds") } + // Actually performs a pending "close then reopen a different project" hand-off recorded via + // pendingDeepLinkOpen. Shared by onDestroy() (the normal case -- see its docs for why the + // hand-off waits until here) and confirmProjectClose's "Save and close" completion (the race + // case -- see there for why that path can't always rely on onDestroy() running afterward). + private fun performPendingDeepLinkOpen(pending: DeepLinkOpenRequest) { + val root = File(pending.projectRoot) + val ctx = applicationContext + // The value carried on the request, NOT the live global. Reading the global here was wrong on + // the plain-switch path: MainActivity.openProject had already overwritten it with the new path + // before the intent arrived, so previous == new and the receiver saw a same-project no-op. + // Falling back to the global still helps the deep-link path, where nothing pre-mutates it. + val previousProjectPath = pending.previousProjectPath ?: IProjectManager.getInstance().projectDirPath + if (!pending.bookkeepingAlreadyRecorded) { + recordProjectOpenedBookkeeping(recentProjectRepository, root, project = null, analyticsManager = analyticsManager) + } + + // Starting an activity from onDestroy() is a background activity start -- and therefore dropped + // silently by Android 10+ -- only once the finishing activity was the last one in its task. + // That cannot happen here: this activity is never its task's root. It is reachable only + // through MainActivity (openProject) or DeepLinkActivity, which routes through MainActivity + // when no editor is live, and it is not exported, so nothing else can launch it. Verified on + // an Android 13 device: every androidide task has MainActivity at Hist #0 with this activity + // above it, so finishing leaves the task non-empty and the app in the foreground. A + // Beepy -> Aegis1 deep-link switch completed with no dropped start. + ctx.startActivity( + Intent(ctx, EditorActivityKt::class.java).apply { + putExtra(EditorIntentExtras.EXTRA_PROJECT_PATH, pending.projectRoot) + // The same extra MainActivity.openProject sends, and for the same reason: the receiver + // falls back to IProjectManager.projectDirPath, which recordProjectOpenedBookkeeping has + // already overwritten to this very path. Omitting it made a delivery that lands on + // onNewIntent (rather than a fresh onCreate) compute previousProjectPath == newProjectPath, + // so switchToProject took its same-project branch and the confirmed switch silently + // no-opped -- with the process-wide global naming the new project while the editor still + // showed the old one. + putExtra(EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH, previousProjectPath) + pending.fileRequest?.let { putExtra(PendingFileRequest.EXTRA_KEY, it) } + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + }, + ) + } + + // Drains the hand-off THIS instance armed (if any) and performs it -- shared by onDestroy() (the + // normal case) and confirmProjectClose's "Save and close" completion once isDestroyed confirms + // onDestroy() already ran (the race case) -- so the one-shot "check, clear, perform" sequence has + // a single copy instead of being kept in sync by hand across both call sites. + private fun drainPendingDeepLinkOpen() { + pendingDeepLinkOpen.drainArmedBy(handoffOwner)?.let(::performPendingDeepLinkOpen) + } + override fun onDestroy() { super.onDestroy() ActionContextProvider.clearActivity(this) + // Not dismissing this would leak the dialog's window (WindowLeaked) past this activity's + // death -- e.g. a rotation while the confirm-close dialog is showing. + // + // dismiss() alone was not enough: it tears the window down WITHOUT dispatching the negative + // button or the OnCancelListener, so a dialog still on screen at destroy time died with its + // decline handling never run -- leaving the intent and the process-wide bookkeeping pointing + // at a switch the user never confirmed (ADFA-5067 review). cancel() would dispatch it, but + // cancelOrDecline() can recursively show a fresh dialog for a superseding request, which is + // exactly what must not happen from here; declineInFlightProjectClose() is its rollback half. + // + // The Bundle is handled separately: onSaveInstanceState runs BEFORE onDestroy, so by now it + // has already been written -- see projectPathForInstanceState for the half of this fix that + // keeps the un-confirmed project out of it in the first place. + activeProjectCloseDialog?.let { dialog -> + if (dialog.isShowing && !closeDialogAnswered) { + declineInFlightProjectClose() + } + dialog.dismiss() + } + activeProjectCloseDialog = null + + // Both gated on isFinishing: onDestroy() also runs for a non-finishing recreate (a config + // change EditorActivityKt's own configChanges doesn't cover - dark mode, locale, display size + // - or "Don't keep activities"), which can land while the confirm-close dialog above is still + // showing and pendingCloseCallback is already armed for it (set the moment the dialog opens, + // not once the user actually chooses an option - see confirmProjectClose). Without this guard, + // a config change the user never asked for silently confirms that pending close/switch and + // discards the project it was showing. The two legitimate confirm paths (Close without saving, + // Save and close) both route through closeProject(), whose deferred finish() must land before + // onDestroy() can run, so isFinishing is true here by the time onDestroy() drains it. + if (isFinishing) { + // The callback drain is additionally gated on closeCommitted: isFinishing alone is also + // true when the task is swiped out of Recents while the dialog is still showing -- an + // armed-but-uncommitted pendingCloseCallback then means the user bailed, and running it + // here would startActivity into a project switch they never confirmed (see the field's + // docs). When the flag IS set, this drain still matters: a "Close without saving" confirm + // deliberately leaves confirmCloseInProgress stuck true (see there) since this instance + // is finishing either way -- a later request that arrived in the window before + // onDestroy() actually ran got parked here with nothing else left to read it. Run and + // clear it now instead of silently orphaning it. + // + // closeCommitted, NOT closeDialogAnswered: the latter went true the instant "Save and + // close" was tapped, so a finish() landing while the save was still writing made this + // line commit the switch and skip the save-failure abort entirely, abandoning the user's + // edits with no message (ADFA-5067 review). Committing is now the save's own decision. + if (closeCommitted) { + pendingCloseCallback?.invoke() + } + pendingCloseCallback = null + } + + // Drain any deep-link-triggered "close then reopen a different project" request this instance + // recorded. Deliberately waits until onDestroy -- which only runs once the framework has + // committed to tearing this singleTask instance down -- rather than firing startActivity() + // synchronously right after finish(), because the two calls racing could otherwise have the + // new PROJECT_PATH redelivered to this dying instance via onNewIntent (which never reads it) + // instead of a genuinely new instance's onCreate. + // + // Outside the isFinishing guard, and keyed on ownership rather than didCompleteLiveOnCreate. + // isFinishing was the wrong question: closeProject() arms the hand-off and then defers its + // finish() into a lifecycleScope coroutine that ON_DESTROY cancels, so a destroy that beat + // the finish() left a confirmed switch sitting in this Koin `single` -- to fire later against + // an unrelated project close, since nothing else clears it. A config-change recreate landing + // mid-save reached here the same way. drainArmedBy answers "did I arm this?" directly, which + // is what didCompleteLiveOnCreate was standing in for: an instance that took + // BaseEditorActivity.onCreate's deepLinkTargetsAnotherProject bail armed nothing, so it now + // drains nothing rather than stealing a live instance's hand-off. + drainPendingDeepLinkOpen() } override fun onResume() { super.onResume() + // Re-asserted here too (not just onCreate) so this instance reclaims ActionContextProvider's + // registration whenever it becomes the foreground-active one again -- e.g. if a different, + // stale-duplicate instance briefly registered over it (see ActionContextProvider.getLiveActivity()'s + // docs) and was then destroyed, clearing the reference entirely with nothing left to restore it + // otherwise. A doomed instance whose onCreate() returned early (isFinishing) never reaches + // onResume() at all, so this can't re-expose a partially-constructed instance the way doing this + // unconditionally in onCreate() would. ActionContextProvider.setActivity(this) isOpenedFilesSaved.set(false) checkForExternalFileChanges() @@ -369,6 +571,11 @@ open class EditorHandlerActivity : editorView.markAsSaved() fileTimestamps[file.absolutePath] = currentTimestamp updateTabs() + // Without this, areFilesModified() (a cached flag, only ever recomputed as a side + // effect of a successful per-file write - see saveResultInternal) can stay + // stale-true after this reload+markAsSaved: nothing else here reflects that the + // buffer this loop just cleaned is no longer modified. + editorViewModel.areFilesModified = hasUnsavedFiles() } } } @@ -705,7 +912,13 @@ open class EditorHandlerActivity : selection: Range?, ) { lifecycleScope.launch { - val editorView = openFile(file, selection) + // A copy here too, not just in the postInLifecycle block below. openFile hands this straight + // to CodeEditorView's constructor, whose content-load pipeline calls selection.validate() + // and ideEditor.validateRange(selection) -- both of which clamp start/end in place on the + // caller's own object. The caller is often not ours to mutate: IDEEditor.showDocument passes + // the Range out of an LSP ShowDocumentParams via IDELanguageClientImpl.openFileAndSelect, so + // clamping it corrupts the language server's own Location. + val editorView = openFile(file, selection?.let { Range(it) }) editorView?.editor?.also { editor -> editor.postInLifecycle { @@ -713,8 +926,18 @@ open class EditorHandlerActivity : editor.setSelection(0, 0) return@postInLifecycle } - editor.validateRange(selection) - editor.setSelection(selection) + // EditorFeatures.validateRange mutates Position in place. For a file that was + // just opened (new CodeEditorView), that same `selection` instance was also handed + // to the view's constructor, whose own async content-load pipeline calls + // validateRange/setSelection on it again once the file finishes reading. If this + // call runs first -- while the document is still the freshly-constructed empty + // one line -- it clamps the shared Position down to (0,0) *before* the real + // content loads, permanently corrupting the value the constructor's own pipeline + // later relies on. Validate/apply a defensive copy here instead, so this call can + // never corrupt the shared instance regardless of which side runs first. + val safeSelection = Range(selection) + editor.validateRange(safeSelection) + editor.setSelection(safeSelection) } } } @@ -725,7 +948,15 @@ open class EditorHandlerActivity : selection: Range?, ): CodeEditorView? = withContext(Dispatchers.Main) { - val range = selection ?: Range.NONE + // Not the shared Range.NONE/Position.NONE singleton -- openFileAndGetIndex below hands this + // straight to CodeEditorView's constructor, whose async content-load pipeline calls + // validateRange/setSelection on it (the identical hazard openFileAndSelect's own selection + // != null path already guards against with a defensive copy). Position has mutable var + // line/column and overrides equals() structurally, so mutating the actual Range.NONE/ + // Position.NONE instance in place would permanently corrupt every future `== Range.NONE`/ + // `== Position.NONE` "nothing found" sentinel check elsewhere in the app (e.g. + // GoToDefinition, FindUsages, OrganizeImportsAction) for the rest of the process. + val range = selection ?: Range(Range.NONE) val isImage = withContext(Dispatchers.IO) { ImageUtils.isImage(file) } if (isImage) { openImage(this@EditorHandlerActivity, file) @@ -788,7 +1019,14 @@ open class EditorHandlerActivity : log.info("Opening file at file index {} tab position {} file:{}", fileIndex, tabPosition, file) - val editor = CodeEditorView(this, file, selection!!) + // A copy, and a real Range rather than `!!`. CodeEditorView's async content-load pipeline calls + // selection.validate() and ideEditor.validateRange(selection), both of which clamp start/end in + // place -- on whatever object the CALLER owns. openFileAndSelect and openFile each copy before + // reaching here, but this is the third entry point IEditorHandler advertises and a caller can + // use it directly. The interface also declares `selection: Range?`, so `!!` turned a documented + // null into a KotlinNullPointerException for any caller honouring that nullability; no in-app + // caller passes null today, which is the only reason it had not fired. + val editor = CodeEditorView(this, file, selection?.let { Range(it) } ?: Range(Range.NONE)) editor.layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT) if (tabPosition >= totalTabs) { @@ -878,14 +1116,60 @@ open class EditorHandlerActivity : requestSync: Boolean, processResources: Boolean, progressConsumer: ((Int, Int) -> Unit)?, - runAfter: (() -> Unit)?, + runAfter: ((Boolean) -> Unit)?, ) { - lifecycleScope.launch(Dispatchers.IO) { + // Not lifecycleScope: NonCancellable protects the body only once it has started running, and + // a launch on the IO dispatcher can still be queued when onDestroy() cancels the activity's + // scope -- in which case the body never starts and runAfter never runs, losing a confirmed + // deep-link project switch exactly as if the guard that used to skip it were still there + // (found in review). The application scope has no such window. The activity is retained for + // the duration of the save, which is what NonCancellable already implied. + // + // That retention is real and deliberate: this lambda captures the activity, so a save still in + // flight after onDestroy holds the binding, the tabs and every editor buffer alive from an + // app-lifetime root -- tens of MB with many files open, and indefinitely if a write blocks on + // stuck SAF/FUSE storage. The narrower design is to move only what must outlive the activity + // (arming pendingDeepLinkOpen, a ~200-byte request) off the activity, and leave the save on + // lifecycleScope. That is a restructure of this method's contract with its callers, not a + // tweak, so it is left as a known cost rather than half-done here. + appScope.launch(Dispatchers.IO) { + // The whole body -- not just saveAll() -- runs NonCancellable. onDestroy() cancels the + // activity's Job as soon as it runs; leaving NonCancellable partway through (e.g. + // right before invoking runAfter) would let that cancellation surface at the next + // suspension point and drop runAfter entirely instead of running it. Callers rely on it + // always running (e.g. confirmProjectClose's onClosed, which arms a pending deep-link + // project switch and would otherwise vanish with no error if this activity is torn down + // while the save is still in flight). withContext(NonCancellable) { - saveAll(notify, requestSync, processResources, progressConsumer) - } - withContext(Dispatchers.Main) { - runAfter?.invoke() + val saveSucceeded = + try { + saveAll(notify, requestSync, processResources, progressConsumer) + true + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // A write failure here (e.g. CodeEditorView.save()'s IOException) must not skip + // runAfter below -- callers rely on it always running to know the save attempt is + // over, successful or not (e.g. confirmProjectClose's confirmCloseInProgress guard, + // which would otherwise stay stuck true and permanently block closing this activity). + log.error("saveAll failed", e) + false + } + withContext(Dispatchers.Main) { + // NonCancellable above means this whole block, including this Main-dispatcher hop, + // keeps running even after onDestroy() -- unlike before this method wrapped the + // entire body in NonCancellable, when that hop was simply dropped on teardown. + // + // runAfter is invoked unconditionally, teardown included. A liveness check here used + // to skip it wholesale, which silently dropped the *non-UI* half of a callback's + // work: confirmProjectClose's onClosed arms a process-wide pending deep-link switch + // (ADFA-5067) that has to outlive this instance, so losing it means a confirmed + // "Save and close" never opens the project the link asked for, with nothing logged. + // Each callback decides for itself what needs a live window -- see the teardown + // branches at the two call sites in this file, and GitBottomSheetFragment's own + // _binding check. + runAfter?.invoke(saveSucceeded) + } } } } @@ -1155,6 +1439,36 @@ open class EditorHandlerActivity : getEditorForFile(file)?.isModified == true } + /** + * Like [hasUnsavedFiles], but excludes files [CodeEditorView.save] never actually writes (an + * [ARCHIVE_EXTENSIONS] extension, opened read-only) -- those can never leave the "modified" + * state through a save, so counting them as a save failure would block "Save and close" forever. + * + * @param files The files to check -- defaults to every currently open file, appropriate for a + * whole-project close like [confirmProjectClose]. A narrower close (e.g. [closeFile]'s single + * tab, via [notifyFilesUnsaved]) must scope this to just the file(s) actually being closed, or + * an unrelated, still-open file's save failure would block a close it has nothing to do with. + */ + private fun hasFilesThatFailedToSave(files: List = editorViewModel.getOpenedFiles()): Boolean { + // Fail closed once the view binding is gone. [getEditorForFile] resolves through + // contentOrNull and returns null for EVERY file the moment it is null, so the per-file test + // below would report "nothing failed" for a whole set of genuinely modified buffers. Every + // caller uses this to decide whether it is safe to close, discard or commit, so an answer + // that cannot be computed must read as "unsafe" -- the direction the ViewModel-backed + // areFilesModified this replaced happened to fail in. + // + // areFilesModified is the fallback rather than a bare `true` because it is retained across + // activity recreation and is only ever recomputed while the binding is alive (see + // saveResult's contentOrNull-guarded block), so it holds the last state actually observed + // rather than a stale-by-construction guess. It is coarser -- one flag for all open files, + // not per-file -- which can over-report for a narrowed [files] set; over-reporting blocks a + // close, under-reporting loses the buffer. + contentOrNull ?: return editorViewModel.areFilesModified + return files.any { file -> + getEditorForFile(file)?.isModified == true && file.extension.lowercase() !in ARCHIVE_EXTENSIONS + } + } + /** * Runs [action] with the "files are saving" flag raised. * @@ -1204,6 +1518,8 @@ open class EditorHandlerActivity : override fun areFilesModified(): Boolean = editorViewModel.areFilesModified + override fun hasUnsavedWritableFiles(): Boolean = hasFilesThatFailedToSave() + override fun areFilesSaving(): Boolean = editorViewModel.areFilesSaving override fun closeFile( @@ -1305,8 +1621,9 @@ open class EditorHandlerActivity : return } - // If there are NO unsaved files, just perform the close action directly. - // The 'manualFinish' is false because this action doesn't exit the activity by itself. + // If there are NO unsaved files, just perform the close action directly. This action + // doesn't exit the activity by itself (performCloseAllFiles never finishes; only + // closeProject does). performCloseAllFiles() runAfter() } @@ -1386,7 +1703,30 @@ open class EditorHandlerActivity : message = getString(string.msg_files_unsaved, TextUtils.join("\n", mapped)), positiveClickListener = { dialog, _ -> dialog.dismiss() - saveAllAsync(notify = true, runAfter = { runOnUiThread(invokeAfter) }) + saveAllAsync( + notify = true, + runAfter = { succeeded -> + runOnUiThread { + // Nothing in this tail survives teardown usefully: flashError needs a live + // window, and invokeAfter closes tabs on a binding that is going away. The + // write itself already completed in saveAllAsync. + if (isFinishing || isDestroyed) return@runOnUiThread + // Matches confirmProjectClose's identical check: saveAllAsync's succeeded + // only means saveAll() didn't throw, not that every file's write actually + // landed (a silent per-file failure, e.g. disk full, leaves isModified + // true without succeeded going false) -- proceeding to invokeAfter (which + // closes/discards these files) on that alone risks silent data loss. + // Scoped to unsavedEditors (not every open file, unlike confirmProjectClose's + // whole-project close) -- this call can be for a single tab (closeFile), and + // an unrelated, still-open file's save failure must not block it. + if (!succeeded || hasFilesThatFailedToSave(unsavedEditors.mapNotNull { it?.file })) { + flashError(getString(string.save_failed)) + return@runOnUiThread + } + invokeAfter.run() + } + }, + ) }, ) { dialog, _ -> dialog.dismiss() @@ -1931,48 +2271,793 @@ open class EditorHandlerActivity : } } - private fun closeProject(saveFloatingFiles: Boolean) { + // [onClosed] (e.g. arming a pending deep-link project switch -- see confirmProjectClose) runs + // synchronously here, before the deferred finish() above can land: onDestroy()'s drain reads + // what it arms, and onDestroy() cannot run before finish() does. + private fun closeProject( + saveFloatingFiles: Boolean, + onClosed: (() -> Unit)? = null, + ) { performCloseAllFiles() lifecycleScope.launch { floatingTabController.closeAll(save = saveFloatingFiles) finish() } - } - - private fun confirmProjectClose() { + onClosed?.invoke() + } + + // Tracked so onDestroy() can dismiss it (avoiding a leaked window) and so a confirm-close flow + // already in progress -- dialog showing, or its "Save and close" still writing files -- can + // reject a second, overlapping confirmProjectClose call rather than either stacking a second + // dialog or silently swapping out the one the user is already looking at. The two flows this + // guards between are the plain manual close (back button, sidebar action, onClosed == null) and + // the deep-link close-then-reopen (onClosed sets pendingDeepLinkOpen) -- letting one hijack the + // other's dialog would mean a user tapping "Close without saving" on what looks like an ordinary + // close ends up with an unrelated deep-linked project opened instead, or vice versa. + private var activeProjectCloseDialog: AlertDialog? = null + private var confirmCloseInProgress = false + + // The onClosed to actually run once the in-flight confirm-close flow resolves. Read at + // resolution time rather than captured per-call, so a THIRD overlapping request (e.g. a deep + // link C arriving while confirmCloseInProgress is already true for an earlier B) can supersede + // B by overwriting this field, instead of being silently dropped by the confirmCloseInProgress + // guard below with no way to ever apply it. + private var pendingCloseCallback: (() -> Unit)? = null + + // True only once the user actually answered the confirm-close dialog with one of its two + // confirm options ("Close without saving" / "Save and close"). onDestroy() gates its + // pendingCloseCallback drain on this rather than on isFinishing alone: isFinishing is true for + // ANY finish -- including the task being swiped out of Recents while the dialog is still up -- + // and pendingCloseCallback is armed the moment the dialog opens, so without this flag that + // swipe ran the callback and performed a project switch the user never confirmed, out of + // onDestroy(), with the previous project's buffers never saved or closed. Reset whenever a + // fresh dialog is shown; a cancel/decline never sets it. + private var closeDialogAnswered = false + + // True once the close is actually being PERFORMED, which is a later moment than + // closeDialogAnswered for "Save and close": that option only commits after its save comes back + // clean, so between the tap and the save resolving the close is answered but not yet committed. + // onDestroy() gates its pendingCloseCallback drain on this, because a finish() arriving inside + // that window used to commit the switch on the answered flag alone and skip the + // `!saveSucceeded || hasFilesThatFailedToSave()` abort entirely -- discarding the user's unsaved + // edits with nothing shown (ADFA-5067 review). "Close without saving" sets it immediately: there + // is no write to wait on and closeProject() runs in the same breath. Reset with the dialog. + private var closeCommitted = false + + // Identity token for this instance's entry in the process-wide PendingDeepLinkOpen, so an + // instance only ever drains the hand-off it armed itself. A plain Any() rather than `this`: the + // token is stored in a Koin `single` that outlives every activity, and parking an Activity + // reference there -- even one only ever compared by identity -- is the kind of thing that turns + // into a leak the first time someone dereferences it. + private val handoffOwner = Any() + + // Captured in onNewIntent, right before setIntent() replaces the intent, whenever the incoming + // intent targets a genuinely different project -- restored by cancelOrDecline()/the "Save and + // close" failure branch below if that switch attempt doesn't end up completing, so the staying + // project's own still-pending file request (if any) isn't silently lost. + private var pendingFileRequestBeforeSwitch: PendingFileRequest? = null + + // True once pendingFileRequestBeforeSwitch has captured the ORIGINAL staying project's request. + // Without this, a second overlapping project-switch intent arriving before the first is + // resolved/declined would re-capture from getIntent() -- which by then holds the FIRST switch + // attempt's intent, not the original -- clobbering the real value with whatever (usually + // nothing) that intermediate intent happened to carry. + private var capturedPendingFileRequestBeforeSwitch = false + + // The project that is actually staying open if the in-flight switch is cancelled or declined, + // snapshotted when the switch is detected. Not re-derived at restore time: on the plain-switch + // path MainActivity.openProject's bookkeeping has already overwritten + // IProjectManager.projectDirPath to the NEW project before the intent is even delivered here, so + // reading that global on decline restored PROJECT_PATH to the project the user just refused -- + // and the next config-change recreate then loaded it, with the open buffers still belonging to + // the project that stayed. onNewIntent computes this same value for isProjectSwitchIntent; this + // keeps it for the decline path, which runs long after that local has gone. + private var stayingProjectPathBeforeSwitch: String? = null + + // Tracked so a slower, older deep-link resolve (still in flight when a second, faster-resolving + // deep link arrives via onNewIntent) can tell it's been superseded -- mirrors + // MainActivity.latestDeepLinkRequest's identical race on the cold-open path. + private var latestDeepLinkRequest: DeepLinkRequest? = null + + // While a switch is proposed but unconfirmed, IProjectManager's global already names the INCOMING + // project (MainActivity.openProject's bookkeeping runs before the intent is even delivered), so + // the base implementation would persist a project the user has not agreed to open -- and + // onSaveInstanceState runs before onDestroy, so onDestroy's decline rollback below cannot undo it. + // The snapshot is null except in exactly that window, so this is the global everywhere else. + override val projectPathForInstanceState: String + get() = stayingProjectPathBeforeSwitch ?: super.projectPathForInstanceState + + // The state-rollback half of confirmProjectClose's cancelOrDecline, without its other half -- + // re-confirming a superseding request, which shows a NEW dialog and must never run from + // onDestroy(), where the window it would attach to is already going away. + private fun declineInFlightProjectClose() { + confirmCloseInProgress = false + val abandoned = pendingCloseCallback + pendingCloseCallback = null + // Only a switch (onClosed != null) put a foreign PROJECT_PATH on the intent and moved the + // process-wide bookkeeping; a plain manual close has nothing to roll back, and touching the + // intent for one would corrupt whatever legitimate pending state it holds. + if (abandoned != null) { + restoreIntentToStayingProject() + } + } + + private fun restoreIntentToStayingProject() { + // Guarded here, not at the call sites. Only one of the three had this check, and the other two + // (cancelOrDecline and the save-failure branch) are reachable with no capture: onNewIntent and + // switchToProject answer "is this a switch?" with different predicates -- the former via + // isDeepLinkTargetOfOpenProject (normalised name plus canonicalised parent), the latter via raw + // string equality -- so they disagree whenever the open project's stored path reaches the same + // directory by another string (/sdcard vs /storage/emulated/0, a symlinked alias). With no + // capture taken, the `restore == null` arm below drains the staying project's own + // carried-forward file request, destroying a navigation it was never asked to touch. + if (!capturedPendingFileRequestBeforeSwitch) { + return + } + // Reset unconditionally, before the blank-path bail below: a blank projectDirPath (e.g. a + // post-process-death recreate with no PROJECT_PATH) must not leave these permanently set -- + // every later switch's capture guard would otherwise stay false forever, silently losing the + // staying project's pending file request on every subsequent decline for the rest of this + // instance's life. + val restore = pendingFileRequestBeforeSwitch + val staying = stayingProjectPathBeforeSwitch + pendingFileRequestBeforeSwitch = null + capturedPendingFileRequestBeforeSwitch = false + stayingProjectPathBeforeSwitch = null + + // The snapshot taken when the switch was detected, not the live global: on the plain-switch + // path the global already holds the NEW project by the time this runs. + val stayingProjectPath = staying ?: IProjectManager.getInstance().projectDirPath + if (stayingProjectPath.isBlank()) return + + // Roll the process-wide bookkeeping back too, not just the intent. MainActivity.openProject + // writes both of these before this dialog can be answered, so a decline otherwise leaves the + // app pointing at the project the user just refused: applyDeepLinkFileRequest resolves file + // paths against projectDirPath, so a later deep-link file navigation would look inside the + // wrong project, and lastOpenedProject would reopen it on the next cold start. Recents and + // the analytics event are deliberately left alone -- the user did ask to open it, and an + // insert that already happened is not wrong, merely early. + if (IProjectManager.getInstance().projectDirPath != stayingProjectPath) { + ProjectManagerImpl.getInstance().projectPath = stayingProjectPath + GeneralPreferences.lastOpenedProject = stayingProjectPath + } + intent.putExtra(EditorIntentExtras.EXTRA_PROJECT_PATH, stayingProjectPath) + // The abandoned switch's own file request (if any) is drained first -- durably, so a + // post-process-death redelivery can't resurrect the navigation the user just declined -- + // then the staying project's still-pending request, if any, is re-armed over it. + drainPendingFileRequest() + restore?.let { armPendingFileRequest(intent, it) } + } + + private fun confirmProjectClose(onClosed: (() -> Unit)? = null) { val content = contentOrNull ?: return + if (confirmCloseInProgress) { + // A plain close (onClosed == null, e.g. back button/sidebar) must not erase an + // already-armed deep-link switch -- only a request that carries its own callback + // supersedes the pending one. + if (onClosed != null) { + pendingCloseCallback = onClosed + } + flashError(getString(string.msg_project_close_in_progress)) + return + } + confirmCloseInProgress = true + pendingCloseCallback = onClosed + closeDialogAnswered = false + closeCommitted = false + val builder = newMaterialDialogBuilder(this) builder.setTitle(string.title_confirm_project_close) builder.setMessage(string.msg_confirm_project_close) - builder.setNegativeButton(string.cancel_project_text, null) + // If a later, superseding request (e.g. a second deep link arriving while this dialog was + // already showing) overwrote pendingCloseCallback, cancelling *this* dialog must not + // silently drop that superseding request too -- give it its own confirmation instead. + // confirmCloseInProgress is reset first so the recursive call starts a fresh dialog rather + // than hitting the "already in progress" guard above. + fun cancelOrDecline() { + confirmCloseInProgress = false + val superseding = pendingCloseCallback + pendingCloseCallback = null + if (superseding !== onClosed) { + confirmProjectClose(superseding) + } else if (onClosed != null) { + // onNewIntent/handlePlainProjectSwitch already called setIntent() with the abandoned + // switch's target (PROJECT_PATH/PendingFileRequest) before this dialog could even show + // -- a genuine decline of that switch (onClosed != null, nothing superseding it) must + // restore the intent to reflect the project that's actually staying open, or a later + // process-death recreate would read the abandoned target from getIntent() and silently + // reopen it instead of resuming this one (see BaseEditorActivity.onCreate's PROJECT_PATH + // fallback). A plain manual close (onClosed == null, e.g. the sidebar's "Close Project") + // never went through onNewIntent's setIntent() in the first place -- there's nothing to + // restore, and touching the intent here would instead corrupt whatever legitimate + // pending state it already holds (e.g. an original cold-open's still-unconsumed file + // request, mid-sync). + restoreIntentToStayingProject() + } + } + + builder.setOnCancelListener { cancelOrDecline() } + + builder.setNegativeButton(string.cancel_project_text) { dialog, _ -> + dialog.dismiss() + cancelOrDecline() + } // OPTION 1: Close without saving builder.setNeutralButton(string.close_without_saving) { dialog, _ -> dialog.dismiss() + closeDialogAnswered = true + // Committed in the same breath: there is no write to wait on, and closeProject() runs + // below unconditionally. Only "Save and close" has a window between answered and + // committed. + closeCommitted = true for (i in 0 until editorViewModel.getOpenedFileCount()) { (content.editorContainer.getChildAt(i) as? CodeEditorView)?.editor?.markUnmodified() } - closeProject(saveFloatingFiles = false) + // Activity is finishing either way (closeProject defers the finish() until the floating + // tabs are closed, but nothing can cancel it); no need to reset confirmCloseInProgress. + // Null out pendingCloseCallback now so a later request arriving before onDestroy() + // actually runs (confirmCloseInProgress stays stuck true) parks its own callback instead + // of this already-consumed one being read and invoked again by onDestroy()'s drain below. + val onClosedNow = pendingCloseCallback + pendingCloseCallback = null + closeProject(saveFloatingFiles = false, onClosed = onClosedNow) } // OPTION 2: Save and close builder.setPositiveButton(string.save_and_close) { dialog, _ -> dialog.dismiss() + closeDialogAnswered = true - saveAllAsync(notify = false) { + saveAllAsync(notify = false) { saveSucceeded -> runOnUiThread { - if (contentOrNull == null) return@runOnUiThread - closeProject(saveFloatingFiles = true) + confirmCloseInProgress = false + + // The save outcome is decided FIRST, before any teardown branching. It used to be + // checked only after the two returns below, so a finish() or a config-change + // recreate landing while the write was still in flight committed the close without + // ever consulting it -- abandoning the user's unsaved edits with nothing shown + // (ADFA-5067 review). A failed write must abort the close on every path. + // + // Answerable during teardown now that hasFilesThatFailedToSave() falls back to the + // retained ViewModel's areFilesModified once the binding is gone; it previously + // resolved every file through the binding and so reported "nothing failed" for a + // whole set of unwritten buffers. + if (!saveSucceeded || hasFilesThatFailedToSave()) { + // closeCommitted stays false, so onDestroy's drain leaves the callback alone. + val superseding = pendingCloseCallback + pendingCloseCallback = null + if (isFinishing || isDestroyed) { + // No window for a Flashbar and no instance left to re-confirm on. The + // buffers are still on disk unchanged and the switch simply does not + // happen. Logged because the branch is otherwise invisible from the UI. + log.warn( + "Save failed during teardown (isFinishing={}, isDestroyed={}); abandoning the " + + "confirmed close rather than switching projects over unwritten changes.", + isFinishing, + isDestroyed, + ) + return@runOnUiThread + } + // Routed through the String overload (indefinite duration, must-dismiss) rather + // than flashError(Int) (a ~1s auto-dismissing toast) -- a user who looks away + // right after tapping "Save and close" must not miss that the close was aborted + // and the activity is still open with unsaved changes. + flashError(getString(string.save_failed)) + // A later, superseding request (e.g. a third deep link arriving while this save + // was in flight) must not be silently dropped just because THIS attempt's save + // failed -- give it its own confirmation, mirroring cancelOrDecline()'s handling + // of the identical race on the cancel path. + if (superseding !== onClosed) { + confirmProjectClose(superseding) + } else if (onClosed != null) { + // Mirrors cancelOrDecline()'s identical restoration -- this failed "Save and + // close" is itself a decline of the switch, and nothing superseded it. + restoreIntentToStayingProject() + } + return@runOnUiThread + } + + // The save landed, so the close is now genuinely committed and onDestroy may act + // on the callback if this instance is torn down before the branches below finish. + closeCommitted = true + + // Teardown: no window for a message, and no point re-confirming a superseded request + // on an instance that is going away -- but the handoff below is process-wide state + // that gets drained by owner, so it must still happen. Mirrors the contentOrNull == + // null branch further down, which exists for the same reason. + // isDestroyed WITHOUT isFinishing is not teardown at all: it is a config-change + // recreate (dark mode, locale, density -- none in EditorActivityKt's configChanges), + // where a successor instance for this same project is already coming up. This + // continuation survives it only because saveAllAsync now runs on the process-wide + // appScope rather than lifecycleScope. + // + // This used to return here and "leave the close callback for the successor + // instance" -- but nothing handed it over: pendingCloseCallback is a per-instance + // field on the dying instance, the hand-off was never armed, and onNewIntent had + // already recorded the deep-link request consumed and stripped it from the intent, + // with the consumed mark persisted through onSaveInstanceState. The successor + // therefore had no way to learn a switch had been confirmed, and re-tapping the + // same URL was gated out as value-equal, so the switch was lost permanently and + // silently (ADFA-5067 review). Arm and drain it here instead. The successor may + // flash up on the old project for a moment before the new one replaces it; that is + // a strictly better outcome than dropping a switch the user explicitly confirmed. + if (!isFinishing && isDestroyed) { + log.info( + "Save completed during a config-change recreate; performing the confirmed close " + + "here rather than leaving it for a successor that cannot recover it.", + ) + val onClosedDuringRecreate = pendingCloseCallback + pendingCloseCallback = null + onClosedDuringRecreate?.invoke() + // onDestroy has already run for this instance (isDestroyed), so its own + // ownership-keyed drain is behind us and nothing else will fire this. + drainPendingDeepLinkOpen() + return@runOnUiThread + } + if (isFinishing) { + val onClosedDuringTeardown = pendingCloseCallback + pendingCloseCallback = null + // Logged, not silent: this branch is the one that used to lose the switch, and it + // is invisible from the UI -- the only symptom was a project that never opened. + log.info( + "Save completed during teardown (isFinishing={}, isDestroyed={}); running the close callback anyway: {}.", + isFinishing, + isDestroyed, + onClosedDuringTeardown != null, + ) + onClosedDuringTeardown?.invoke() + // isDestroyed too: onDestroy has already run and drained, so nothing else will. + // While it has not, onDestroy's own drain is still ahead of us and doing it here + // would fire the handoff twice. + if (isDestroyed) drainPendingDeepLinkOpen() + return@runOnUiThread + } + recentProjectsViewModel.updateProjectModifiedDate( + editorViewModel.getProjectName(), + ) + // Captured then nulled before use, mirroring the neutral-button handler above -- + // otherwise onDestroy()'s own unconditional pendingCloseCallback?.invoke() would fire + // this same callback a second time. + val onClosedNow = pendingCloseCallback + pendingCloseCallback = null + // contentOrNull can already be null here if the binding was torn down while the + // save was in flight -- closeProject's performCloseAllFiles would NPE on the view + // manipulation it does, but onClosedNow (e.g. arming a pending deep-link project + // switch) has no such dependency and must still run, or a confirmed close silently + // drops it. + if (contentOrNull != null) { + closeProject(saveFloatingFiles = true, onClosed = onClosedNow) + } else { + onClosedNow?.invoke() + // contentOrNull also goes null via isDestroying, which onPause() sets from + // isFinishing -- well before onDestroy() actually runs -- so it is NOT reliable + // proof onDestroy()'s one-shot drain already happened. Only isDestroyed (the real + // Activity flag, true only once onDestroy() has actually been called) means that. + // If onDestroy() hasn't run yet, it still will (isFinishing guarantees it + // eventually does) and will drain whatever pendingCloseCallback just armed itself + // -- draining it here instead would risk redelivering the new PROJECT_PATH to + // this still-alive singleTask instance via onNewIntent rather than a genuinely new + // instance, the exact race onDestroy()'s deferred design exists to avoid. + if (isDestroyed) { + drainPendingDeepLinkOpen() + } + } + } + } + } + + activeProjectCloseDialog = builder.show() + } + + override fun onNewIntent(intent: Intent) { + super.onNewIntent(intent) + + val deepLinkRequest = + IntentCompat.getParcelableExtra(intent, DeepLinkRequest.EXTRA_KEY, DeepLinkRequest::class.java) + + // Only true for an intent that ISN'T itself requesting a switch to a genuinely *different* + // project -- e.g. some other explicit re-launch of this activity, or a deep link/PROJECT_PATH + // intent that re-targets the project already loading. Gating the carry-forward below on this + // prevents a still-loading project's own stale file request from getting attached to an + // unrelated switch to a different project, while still preserving it when the incoming intent + // turns out to be for the SAME project: a same-project deep link with no file target of its + // own (or a bare Recents re-tap) would otherwise silently lose the original cold-open's still- + // pending request, since neither switchToProject's nor handlePlainProjectSwitch's same-project + // branch reads the carried-forward extra itself -- they only apply whatever fileRequest THIS + // intent carries, which is often none. Comparing the deep link's project name against the + // currently-loading project's directory name (mirroring BaseEditorActivity.onCreate's own + // deepLinkTargetsAnotherProject check) is a synchronous, disk-free way to tell same from + // different without waiting on the deep-link path's own async resolve. + // EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH, when present, is what IProjectManager.projectDirPath held before + // MainActivity.openProject's bookkeeping call overwrote it to the NEW path -- by the time this + // intent arrives here, the global itself already reads as the new path regardless of whether + // this is actually a switch, so re-reading it for the comparison below would never detect one. + val previousProjectPath = + intent.getStringExtra(EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH) ?: IProjectManager.getInstance().projectDirPath + val isProjectSwitchIntent = + ( + deepLinkRequest != null && + !isDeepLinkTargetOfOpenProject( + IProjectManager.getInstance().projectDirPath, + deepLinkRequest.projectName, + projectsRoot(), + ) + ) || + intent.getStringExtra(EditorIntentExtras.EXTRA_PROJECT_PATH)?.let { it != previousProjectPath } == true + + // Preserve a not-yet-applied file-navigation request from the previous intent -- postProjectInit + // reads it lazily once a sync completes, and setIntent() below would otherwise silently drop it + // if this onNewIntent call is for something unrelated to that pending request. + if (!isProjectSwitchIntent && !intent.hasExtra(PendingFileRequest.EXTRA_KEY)) { + IntentCompat + .getParcelableExtra(getIntent(), PendingFileRequest.EXTRA_KEY, PendingFileRequest::class.java) + ?.let { armPendingFileRequest(intent, it) } + } + // The reverse case: this IS a switch to a genuinely different project, so the carry-forward + // above is skipped and the old intent's own still-pending file request (for the project + // that's actually staying open if this switch gets cancelled/declined) would otherwise be + // lost the moment setIntent() below replaces it. restoreIntentToStayingProject() puts it back + // if that turns out to be what happens. + // Guarded on capturedPendingFileRequestBeforeSwitch so a SECOND overlapping switch intent, + // arriving before the first is resolved/declined, doesn't re-capture from getIntent() -- by + // then holding the first switch's own intent, not the original staying project's -- and + // clobber the real value with whatever (usually nothing) that intermediate intent carries. + if (isProjectSwitchIntent && !capturedPendingFileRequestBeforeSwitch) { + pendingFileRequestBeforeSwitch = + IntentCompat.getParcelableExtra(getIntent(), PendingFileRequest.EXTRA_KEY, PendingFileRequest::class.java) + capturedPendingFileRequestBeforeSwitch = true + stayingProjectPathBeforeSwitch = previousProjectPath + } + setIntent(intent) + + val request = deepLinkRequest + if (request == null) { + // Not a deep link -- a plain project-switch intent from MainActivity.openProject (Recents, + // Clone, Template creation) redelivered here via onNewIntent because this singleTask + // instance is already alive for a different project. Without this, the user taps a + // different project elsewhere in the app and nothing visibly happens. + handlePlainProjectSwitch(intent) + return + } + + // This is the request's only chance to be consumed: whether it's applied immediately, + // deferred via pendingDeepLinkOpen, or dropped because the user cancels the close-project + // dialog below, it must not linger on the intent setIntent() just stored. Android redelivers + // that same intent verbatim to onCreate() if this process dies and gets recreated later, and + // BaseEditorActivity.onCreate() would then wrongly compare a live, unrelated project against + // this stale request's projectName and bounce the user out of it. The removeExtra only + // scrubs this process's Intent object, not the parceled copy that post-process-death + // redelivery is made from -- consumedDeepLinkRequests (persisted via onSaveInstanceState) is + // what makes the consumption stick there, gating onCreate's read. + consumedDeepLinkRequests.add(request) + intent.removeExtra(DeepLinkRequest.EXTRA_KEY) + + // Tracked so a second deep link delivered moments later doesn't have its resolve complete + // out of order with this one -- mirrors MainActivity.latestDeepLinkRequest's identical race. + latestDeepLinkRequest = request + + lifecycleScope.launch(Dispatchers.IO) { + val lookup = resolveDeepLinkProject(projectsRoot(), request.projectName) + if (lookup !is DeepLinkProjectLookup.Found) { + // Mirrors MainActivity's identical branch. Without it this path emitted RECEIVED and + // then nothing, which reads exactly like the silent drop-off the paired events exist + // to expose -- a missing instrument masquerading as the bug it was added to find. + analyticsManager.trackDeepLink( + DeepLinkMetric( + request.depth(), + if (lookup is DeepLinkProjectLookup.NotFound) { + DeepLinkOutcome.PROJECT_NOT_FOUND + } else { + DeepLinkOutcome.PROJECT_UNVERIFIABLE + }, + request.projectName, + ), + ) + // No such project, so the switch this intent announced is never going to happen. Without + // this the capture above is stranded: setIntent() has already dropped the staying + // project's pending file request, nothing puts it back, and + // capturedPendingFileRequestBeforeSwitch stays true for the life of the instance -- so + // the next genuine switch skips its own capture and a later decline restores this stale + // one instead. The two lifecycle early-returns below deliberately don't do this: a + // finishing instance has nothing to restore into, and a superseded request must leave + // the capture alone for the newer switch that is still relying on it. + withContext(Dispatchers.Main) { + // Two further conditions, both matching the sibling call sites. + // + // capturedPendingFileRequestBeforeSwitch: only restore what this path actually + // captured. With nothing captured, restoreIntentToStayingProject's `restore == null` + // arm runs intent.removeExtra(PendingFileRequest.EXTRA_KEY) and deletes a + // carried-forward request belonging to the staying project -- the corruption the + // other two call sites' `else if (onClosed != null)` guard exists to avoid. + // + // latestDeepLinkRequest === request: a slow, failing link must not clear a capture + // that a newer link's own decline path is still relying on. + // The capture check now lives inside restoreIntentToStayingProject; what stays here is + // the supersession check, which is specific to this async path: a slow, failing link + // must not clear a capture a newer link's decline still depends on. + if (!isFinishing && !isDestroyed && latestDeepLinkRequest === request) { + restoreIntentToStayingProject() + } } - recentProjectsViewModel.updateProjectModifiedDate( - editorViewModel.getProjectName(), + return@launch + } + withContext(Dispatchers.Main) { + // The activity may have started finishing while resolveDeepLinkProject was still + // scanning disk -- lifecycleScope only cancels at ON_DESTROY, not the moment isFinishing + // first flips true, so this continuation can otherwise still run and try to show the + // confirm-close dialog on a dying window. + if (isFinishing || isDestroyed) return@withContext + // A newer deep link's onNewIntent call already superseded this one -- switching to + // this stale target now would undo the newer request the user actually tapped. + if (latestDeepLinkRequest !== request) return@withContext + switchToProject(lookup.projectDir.absolutePath, request.fileRequest) + } + } + } + + override fun postProjectInit( + isSuccessful: Boolean, + failure: TaskExecutionResult.Failure?, + ) { + super.postProjectInit(isSuccessful, failure) + + // Covers requirement #1 (cold open + file) and the tail of requirement #3 (a fresh + // EditorActivityKt instance always runs the normal init pipeline, whether started by + // MainActivity.openProject or by this activity's own onDestroy() hand-off). + // + // Drained regardless of outcome, not just on success -- otherwise a failed sync leaves the + // extra armed, and it fires later on the next unrelated *successful* sync/variant switch, + // silently yanking the editor back to this stale request instead of never reapplying. (Two + // init failures return before this is ever called and drain at their own early returns + // instead -- see ProjectHandlerActivity.initializeProject.) The drain also records the + // request in consumedFileRequests, which is what makes it stick across process death: a + // recreate is handed the parceled intent with the extra still on it, and without the durable + // marker the restored editor would be yanked back to this file/line with no user action. + // drainPendingFileRequest returns null for exactly that redelivered-but-already-consumed case. + val request = drainPendingFileRequest() ?: return + if (!isSuccessful) return + applyDeepLinkFileRequest(request) + } + + // Handles a plain project-switch intent from MainActivity.openProject (Recents, Clone, or + // Template creation) redelivered here via onNewIntent because this singleTask instance is + // already alive -- mirrors the deep-link "different project" handling in onNewIntent above + // (same no-op-if-already-open check, same confirm-close-then-reopen handoff), just without a + // project name to resolve first since the caller already supplies an absolute path directly. + private fun handlePlainProjectSwitch(intent: Intent) { + // Deliberately no isFinishing/isDestroyed early-return here (unlike the deep-link path): this + // instance may already be finishing because it just armed pendingDeepLinkOpen for an earlier + // request and called finish() (switchToProject's isBlank() branch), awaiting its own + // onDestroy(). Dropping this request outright would be strictly worse than letting it + // supersede the earlier one -- MainActivity.openProject already synchronously recorded THIS + // project as opened everywhere (ProjectManagerImpl, lastOpenedProject, Recents, analytics) + // before redelivering this intent, so silently ignoring it here would leave every persisted + // "last opened project" record pointing at a project the app never actually opens. Letting the + // later request win (matching pendingCloseCallback's/askProjectOpenPermission's same + // last-request-wins pattern elsewhere in this file) keeps behavior consistent with bookkeeping. + val newProjectPath = intent.getStringExtra(EditorIntentExtras.EXTRA_PROJECT_PATH)?.takeIf { it.isNotBlank() } ?: return + val fileRequest = + IntentCompat.getParcelableExtra(intent, PendingFileRequest.EXTRA_KEY, PendingFileRequest::class.java) + // No unconditional drain here (unlike this method's earlier version): switchToProject's own + // same-project branch now drains this only once the request is actually applied or + // intentionally dropped, so a request arriving mid-sync stays armed for postProjectInit's + // deferred retry instead of being silently lost. The other branches (isFinishing, blank path, + // different project) don't read the intent's own copy at all -- they thread fileRequest + // through DeepLinkOpenRequest to a brand-new intent instead. + + // See onNewIntent's identical read: MainActivity.openProject's bookkeeping call already + // overwrote the live global to newProjectPath before this intent arrived. + val previousProjectPath = + intent.getStringExtra(EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH) ?: IProjectManager.getInstance().projectDirPath + // bookkeepingAlreadyRecorded: this intent came from MainActivity.openProject, which recorded + // the open before sending it. + switchToProject(newProjectPath, fileRequest, previousProjectPath, bookkeepingAlreadyRecorded = true) + } + + /** + * Shared three-way dispatch for switching this singleTask instance to [newProjectPath]: no + * project loaded yet, the same project already open, or a different project requiring the + * confirm-close-then-reopen handoff. Used by both the deep-link path (onNewIntent, once the + * project name is resolved to a path) and the plain project-switch path ([handlePlainProjectSwitch], + * which already has an absolute path from its caller) -- previously duplicated in both places. + * + * [previousProjectPath] defaults to the live [IProjectManager] global, which is accurate for the + * deep-link caller (nothing pre-mutates it before onNewIntent runs there); the plain-switch caller + * passes its own pre-mutation snapshot instead, since by the time its intent arrives, + * MainActivity.openProject's bookkeeping has already overwritten that global to [newProjectPath]. + */ + private fun switchToProject( + newProjectPath: String, + fileRequest: PendingFileRequest?, + previousProjectPath: String = IProjectManager.getInstance().projectDirPath, + bookkeepingAlreadyRecorded: Boolean = false, + ) { + val currentProjectPath = previousProjectPath + when { + // This instance is already finishing (e.g. it just armed pendingDeepLinkOpen for an + // earlier switch and called finish() below, awaiting its own onDestroy()) -- comparing + // newProjectPath against currentProjectPath below would be comparing against + // ProjectManagerImpl's process-wide path, which a *different*, unrelated instance's + // MainActivity.openProject() can overwrite in the meantime, making this look like a + // same-project no-op when it isn't. Superseding the earlier pending open (last request + // wins, matching handlePlainProjectSwitch's own reasoning) is unconditionally correct + // here since this instance can't do anything else with a new request anyway. + isFinishing -> { + pendingDeepLinkOpen.arm( + handoffOwner, + DeepLinkOpenRequest( + newProjectPath, + fileRequest, + bookkeepingAlreadyRecorded, + previousProjectPath, + ), ) } + + // Either no project has actually finished initializing in this instance yet (e.g. it was + // recreated after process death without a PROJECT_PATH extra), or contentOrNull is + // already null (binding torn down) -- either way, confirmProjectClose below would + // silently no-op, dropping the request with no error shown. Route through the same + // onDestroy()-deferred handoff used for a confirmed project switch instead of showing (or + // trying to show) a close dialog that can't work either way. + currentProjectPath.isBlank() || contentOrNull == null -> { + pendingDeepLinkOpen.arm( + handoffOwner, + DeepLinkOpenRequest( + newProjectPath, + fileRequest, + bookkeepingAlreadyRecorded, + previousProjectPath, + ), + ) + finish() + } + + // projectDirPath is set as soon as a project starts opening -- unlike workspace, which + // stays null for the whole duration of a Gradle sync -- so this correctly matches the + // "already in this project" case even mid-sync, instead of falling through to the + // disruptive close-and-reopen confirmation below for a no-op. + newProjectPath == currentProjectPath -> { + // Gates both applying now and draining the intent's own copy below: a request + // arriving while the project is still syncing (workspace == null) must stay armed for + // postProjectInit's deferred retry once that sync completes, or it's lost for good -- + // applyDeepLinkFileRequest resolves against files a still-in-progress sync may not + // have settled yet. + val projectReady = IProjectManager.getInstance().workspace != null + if (confirmCloseInProgress) { + // A close-confirmation dialog for a *different* project switch is already + // showing -- navigating underneath it now would just get silently discarded if + // the user goes on to confirm that close. + flashError(getString(string.msg_project_close_in_progress)) + // Drop only THIS request's own copy of the extra (the plain-switch path arms it + // on the intent before this runs). The extra can instead be an older, + // still-unapplied request for this staying project -- armed by the mid-sync + // branch below on an earlier call -- which has nothing to do with the in-flight + // close and which postProjectInit must still apply if the user cancels it; + // draining unconditionally here destroyed that older request along with this one. + val armedRequest = + IntentCompat.getParcelableExtra(intent, PendingFileRequest.EXTRA_KEY, PendingFileRequest::class.java) + if (fileRequest != null && fileRequest == armedRequest) { + drainPendingFileRequest() + } + } else if (projectReady) { + fileRequest?.let { + applyDeepLinkFileRequest(it) + // This request supersedes whatever onNewIntent's carry-forward guard just + // re-armed onto the intent from the PREVIOUS, still-unconsumed request -- + // leaving it in place would have postProjectInit silently jump back to that + // stale target once the current sync completes, discarding this newer + // navigation. + drainPendingFileRequest() + } + } else { + // Mid-sync: arm the request on the intent so postProjectInit's deferred retry + // finds it once the sync completes -- this branch is the "must stay armed" + // case the projectReady comment above describes, and without the put the + // request dies with this call while onNewIntent's carry-forward has already + // re-armed the PREVIOUS, still-unconsumed request, sending the editor to that + // stale target instead (ADFA-5067 review). The arm also supersedes that + // carried-forward value, so this branch needs no drain. + fileRequest?.let { armPendingFileRequest(intent, it) } + } + } + + else -> { + // A different project is open. Reuse the existing, unmodified confirm-close dialog; + // only record the pending open if the user actually confirms -- see onDestroy() for + // why the reopen itself waits until this instance is torn down. + confirmProjectClose { + pendingDeepLinkOpen.arm( + handoffOwner, + DeepLinkOpenRequest( + newProjectPath, + fileRequest, + bookkeepingAlreadyRecorded, + previousProjectPath, + ), + ) + } + } } + } - builder.show() + /** + * Applies a deep-link file/line/column request to the *currently open* project. [request]'s + * file path is attacker-controllable URL input, so it's resolved through + * [resolveWithinDirectory] rather than a bare [File] constructor -- see that function's docs for + * why a lexical `..` check alone isn't enough. + * + * [resolveWithinDirectory]'s ancestor-symlink walk and the [File.isFile] check both hit disk, so + * -- like [openFile]'s own image check -- this runs off [Dispatchers.IO] rather than blocking the + * main thread the two call sites (`onNewIntent`, [postProjectInit]) invoke this from. + */ + private fun applyDeepLinkFileRequest(request: PendingFileRequest) { + lifecycleScope.launch(Dispatchers.IO) { + val projectDir = File(IProjectManager.getInstance().projectDirPath) + val file = + try { + resolveWithinDirectory(projectDir, request.filePath)?.takeIf { it.isFile } + } catch (e: CancellationException) { + throw e + } catch (e: SecurityException) { + // resolveWithinDirectory's toRealPath()/Files.exists() walk and the chained + // File.isFile() check both hit disk -- resolveDeepLinkProject already treats this + // as a real risk for the same kind of I/O one call away. + log.error("Failed to resolve deep-link file request for {}", request.filePath, e) + withContext(Dispatchers.Main) { + if (!isFinishing && !isDestroyed) { + flashError(getString(string.msg_deeplink_scan_failed)) + } + } + return@launch + } + + withContext(Dispatchers.Main) { + // The activity may have started finishing while resolveWithinDirectory was still + // hitting disk -- lifecycleScope only cancels at ON_DESTROY, not the moment isFinishing + // first flips true, so this continuation can otherwise still run and touch a dying + // window. Same race onNewIntent already guards against. + if (isFinishing || isDestroyed) return@withContext + if (file == null) { + flashError(getString(string.msg_deeplink_file_not_found, request.filePath)) + return@withContext + } + + // URL line/column are 1-based; internal Position is 0-based. + val (line, lineInvalidRaw) = zeroBasedOrInvalid(request.lineRaw) + val (column, columnInvalidRaw) = zeroBasedOrInvalid(request.columnRaw) + + // A dangling keyword (a trailing line/column segment with no value after it) is + // reported as raw = "" -- show a readable placeholder instead of literal empty quotes. + fun shown(raw: String) = raw.ifEmpty { getString(string.msg_deeplink_no_value) } + // At most one Flashbar here -- a malformed URL can have both line and column invalid + // at once, and showing both would stack two indefinite-duration bars instead of one. + when { + lineInvalidRaw != null -> flashError(getString(string.msg_deeplink_invalid_line, shown(lineInvalidRaw))) + columnInvalidRaw != null -> flashError(getString(string.msg_deeplink_invalid_column, shown(columnInvalidRaw))) + } + + // Range.pointRange, not Range(pos, pos): the factory already exists for exactly this, and + // it hands back two distinct Positions -- passing one instance as both ends gives + // EditorFeatures.validateRange a Range whose clamps move each other. + openFileAndSelect(file, Range.pointRange(Position(line, column))) + } + } + } + + /** + * Converts a 1-based deep-link line/column value to 0-based, paired with the raw value if it + * was present but invalid (fails [String.toIntOrNull] or non-positive) -- a `null` [raw] + * (segment absent from the URL) is never reported, only a present-but-invalid one. See + * [PendingFileRequest]'s docs for why those two cases are distinguished upstream. + */ + private fun zeroBasedOrInvalid(raw: String?): Pair { + raw ?: return 0 to null + val parsed = raw.toIntOrNull() + return if (parsed == null || parsed <= 0) 0 to raw else (parsed - 1) to null } } diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt index b63a3e6540..26ed2966e3 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt @@ -58,6 +58,7 @@ import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.IDELanguageClientImpl import com.itsaky.androidide.lsp.debug.DebugClientConnectionResult import com.itsaky.androidide.lsp.java.utils.CancelChecker +import com.itsaky.androidide.models.EditorIntentExtras import com.itsaky.androidide.models.Position import com.itsaky.androidide.models.Range import com.itsaky.androidide.models.SearchResult @@ -188,6 +189,14 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { private val buildServiceConnection = GradleBuildServiceConnnection() + // True once onCreate() has completed past its isFinishing check -- mirrors + // EditorHandlerActivity.didCompleteLiveOnCreate. super.onCreate() (BaseEditorActivity) may + // already have called finish() for a doomed instance spun up by a stale deep-link liveness + // check; finish() doesn't stop execution, so without this flag preDestroy() would unregister + // the process-wide build-service Lookup entry and shut down the LSP singleton that an + // actually-live sibling instance still depends on. + private var didCompleteLiveOnCreate = false + companion object { private val logger = LoggerFactory.getLogger(ProjectHandlerActivity::class.java) @@ -214,6 +223,16 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) + // super.onCreate() may have already called finish() for a doomed instance (see + // EditorHandlerActivity.onCreate's own isFinishing guard for the fuller explanation); + // finish() doesn't stop execution here, so without this check startServices() below would + // unconditionally bind a build service and register a listener that preDestroy() will + // later tear down, corrupting the actually-live sibling instance's state. + if (isFinishing) { + return + } + didCompleteLiveOnCreate = true + editorViewModel._isSyncNeeded.observe(this) { isSyncNeeded -> if (!isSyncNeeded) { // dismiss if already showing @@ -232,7 +251,7 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { observeStates() startServices() - if (intent.getBooleanExtra("HAS_TEMPLATE_ISSUES", false)) { + if (intent.getBooleanExtra(EditorIntentExtras.EXTRA_HAS_TEMPLATE_ISSUES, false)) { flashError(getString(string.msg_template_warnings)) } } @@ -373,7 +392,7 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { syncNotificationFlashbar?.dismiss() syncNotificationFlashbar = null - if (isDestroying) { + if (didCompleteLiveOnCreate && isDestroying) { releaseServerListener() this.initializingFuture?.cancel(true) this.initializingFuture = null @@ -381,13 +400,13 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { doCloseAll() } - if (IDELanguageClientImpl.isInitialized()) { + if (didCompleteLiveOnCreate && IDELanguageClientImpl.isInitialized()) { IDELanguageClientImpl.shutdown() } super.preDestroy() - if (isDestroying) { + if (didCompleteLiveOnCreate && isDestroying) { try { stopLanguageServers() } catch (_: Exception) { @@ -589,11 +608,19 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE) as? GradleBuildService if (buildService == null) { log.error("No build service found. Cannot initialize project.") + // This init failed before postProjectInit could ever run, so its regardless-of-outcome + // drain never happens here -- without this, a deep link's pending file navigation stays + // armed on the intent and fires on the next unrelated successful sync or variant + // switch, the stale jump that drain exists to prevent. Same for the tooling-server + // check below. (The handleMissingProjectDirectory returns above don't need it: they + // finish() this instance, and the request dies with it.) + withContext(Dispatchers.Main.immediate) { drainPendingFileRequest() } return@launch } if (!buildService.isToolingServerStarted()) { flashError(string.msg_tooling_server_unavailable) + withContext(Dispatchers.Main.immediate) { drainPendingFileRequest() } return@launch } @@ -674,6 +701,13 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { if (service.isToolingServerStarted()) { if (service.isBuildInProgress) { log.info("Skipping project initialization while build is in progress") + // The third early return that never reaches postProjectInit, and so never reaches its + // drain -- the same reason initializeProject's two failure returns drain. Cold-opening a + // project by deep link while a Gradle build is already running otherwise left the + // PendingFileRequest armed on the intent: the editor never navigated (only a log line), + // and the request then fired on the first unrelated later sync, yanking the editor to + // that stale file and line. + lifecycleScope.launch(Dispatchers.Main.immediate) { drainPendingFileRequest() } return } initializeProject() diff --git a/app/src/main/java/com/itsaky/androidide/analytics/AnalyticsManager.kt b/app/src/main/java/com/itsaky/androidide/analytics/AnalyticsManager.kt index 3a252cab94..4f7eae9306 100644 --- a/app/src/main/java/com/itsaky/androidide/analytics/AnalyticsManager.kt +++ b/app/src/main/java/com/itsaky/androidide/analytics/AnalyticsManager.kt @@ -7,6 +7,7 @@ import com.google.firebase.ktx.Firebase import com.itsaky.androidide.analytics.gradle.BuildCompletedMetric import com.itsaky.androidide.analytics.gradle.BuildStartedMetric import com.itsaky.androidide.analytics.gradle.StrategySelectedMetric +import org.slf4j.LoggerFactory import java.util.concurrent.TimeUnit interface IAnalyticsManager { @@ -40,6 +41,8 @@ interface IAnalyticsManager { fun trackBuildCompleted(metric: BuildCompletedMetric) = trackMetric(metric) + fun trackDeepLink(metric: DeepLinkMetric) = trackMetric(metric) + fun trackMetric(metric: Metric) } @@ -159,6 +162,19 @@ class AnalyticsManager : IAnalyticsManager { val bundle = metric.asBundle() bundle.putLong("timestamp", System.currentTimeMillis()) - analytics.logEvent(metric.eventName, bundle) + // Reaching `analytics` initializes FirebaseAnalytics, which throws outright when the default + // FirebaseApp was never initialized in this process. Measuring a feature must never be able + // to break it: DeepLinkActivity is exported and logs before it does anything else, so an + // uninitialized Firebase would turn every incoming link into a crash rather than a lost + // event. Swallowed at this one choke point so no metric call site has to guard for itself. + try { + analytics.logEvent(metric.eventName, bundle) + } catch (e: IllegalStateException) { + log.warn("Dropping metric {}: analytics unavailable.", metric.eventName, e) + } + } + + private companion object { + private val log = LoggerFactory.getLogger(AnalyticsManager::class.java) } } diff --git a/app/src/main/java/com/itsaky/androidide/analytics/DeepLinkMetric.kt b/app/src/main/java/com/itsaky/androidide/analytics/DeepLinkMetric.kt new file mode 100644 index 0000000000..16c8bed7e0 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/analytics/DeepLinkMetric.kt @@ -0,0 +1,95 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.analytics + +import android.os.Bundle +import com.itsaky.androidide.models.DeepLinkRequest + +/** + * How specific an incoming deep link was -- how far down + * `project/{name}/file/{f}/line/{n}/column/{n}` it actually reached. + */ +enum class DeepLinkDepth { + /** Not yet parsed, or not a link this app understands. */ + UNKNOWN, + PROJECT, + FILE, + LINE, + COLUMN, +} + +/** What an incoming deep link ended up doing. */ +enum class DeepLinkOutcome { + /** Parsed and accepted, before anything was resolved. Pairs with a later terminal outcome. */ + RECEIVED, + + /** Dropped because the IDE has not finished onboarding -- see [DeepLinkOutcome] call site. */ + SETUP_INCOMPLETE, + + /** The URI did not parse as a link this app understands. */ + INVALID_LINK, + + /** No project of that name exists. */ + PROJECT_NOT_FOUND, + + /** + * Whether the project exists could not be determined (a filesystem failure unrelated to its + * existence). Kept apart from [PROJECT_NOT_FOUND] for the same reason the code does: a spike + * here means storage trouble, not people mistyping project names. + */ + PROJECT_UNVERIFIABLE, +} + +/** + * A deep link arrived (ADFA-5067). Emitted once with [DeepLinkOutcome.RECEIVED] when a link is + * accepted, and again with whatever terminal outcome it reached, so the drop-off between the two is + * visible -- every bug this feature shipped with looked identical from the outside (a link that + * silently did nothing), and nothing on the device recorded that it had happened at all. + */ +class DeepLinkMetric( + private val depth: DeepLinkDepth, + private val outcome: DeepLinkOutcome, + private val projectName: String? = null, +) : Metric { + override val eventName: String = EVENT_NAME + + override fun asBundle(): Bundle = + Bundle().apply { + putString("depth", depth.name.lowercase()) + putString("outcome", outcome.name.lowercase()) + // Hashed, never the raw name -- a project name is the user's content, and a link can + // carry a file path too. Matches trackProjectOpened's project_hash, which also makes the + // two joinable without either of them carrying the name off the device. + projectName?.let { putLong("project_hash", it.hashCode().toLong()) } + putLong("timestamp", System.currentTimeMillis()) + } + + companion object { + const val EVENT_NAME = "deep_link" + } +} + +/** How far down the optional segments this request actually reached. */ +fun DeepLinkRequest.depth(): DeepLinkDepth { + val file = fileRequest ?: return DeepLinkDepth.PROJECT + return when { + file.columnRaw != null -> DeepLinkDepth.COLUMN + file.lineRaw != null -> DeepLinkDepth.LINE + else -> DeepLinkDepth.FILE + } +} diff --git a/app/src/main/java/com/itsaky/androidide/api/ActionContextProvider.kt b/app/src/main/java/com/itsaky/androidide/api/ActionContextProvider.kt index e5972bbeb1..9e836e22ae 100644 --- a/app/src/main/java/com/itsaky/androidide/api/ActionContextProvider.kt +++ b/app/src/main/java/com/itsaky/androidide/api/ActionContextProvider.kt @@ -8,24 +8,53 @@ import java.lang.ref.WeakReference * to allow decoupled services to trigger UI actions. */ object ActionContextProvider { - private var activityRef: WeakReference? = null + // IDEApiFacade.runApp() (a suspend fun with no explicit Dispatchers.Main) reads getActivity() with + // no guarantee its caller is already on the main thread that writes this -- @Volatile establishes + // the same happens-before guarantee this PR's sibling PendingDeepLinkOpen.value already relies on + // for the identical cross-thread read/write pattern. + @Volatile + private var activityRef: WeakReference? = null - fun setActivity(activity: EditorHandlerActivity) { - this.activityRef = WeakReference(activity) - } + fun setActivity(activity: EditorHandlerActivity) { + this.activityRef = WeakReference(activity) + } - fun clearActivity() { - this.activityRef?.clear() - this.activityRef = null - } + fun clearActivity() { + this.activityRef?.clear() + this.activityRef = null + } - fun clearActivity(activity: EditorHandlerActivity) { - if (this.activityRef?.get() === activity) { - clearActivity() - } - } + fun clearActivity(activity: EditorHandlerActivity) { + if (this.activityRef?.get() === activity) { + clearActivity() + } + } - fun getActivity(): EditorHandlerActivity? { - return activityRef?.get() - } -} \ No newline at end of file + /** + * The current registered [EditorHandlerActivity], or `null` if there is none. Deliberately NOT + * filtered on `isFinishing`/`isDestroyed` (unlike [getLiveActivity]): pre-existing consumers + * depend on getting the instance even during a finishing window -- e.g. a floating + * `EditorPanelDockableContent`, documented to outlive the activity, and `IDEApiFacade.runApp`, + * which should still launch the built app rather than report "no active IDE window". + */ + fun getActivity(): EditorHandlerActivity? = activityRef?.get() + + /** + * Like [getActivity], but `null` also for an instance that is already tearing down -- one that + * called `finish()` but hasn't run `onDestroy()` (and cleared itself via [clearActivity]) yet. + * Android delivers `singleTask` intents to a finishing instance's [android.app.Activity.onNewIntent] + * inconsistently (a genuinely new instance can be created instead), so callers that route based + * on "is there a live editor to hand this off to" -- the deep-link routing in + * [com.itsaky.androidide.activities.DeepLinkActivity] -- need this distinction, not just non-null. + * + * [setActivity] is called from both `onCreate` and `onResume`: `onCreate` closes the blind window + * between `onCreate` and `onResume` where a caller like + * [com.itsaky.androidide.activities.DeepLinkActivity] would otherwise see `null` for a live + * instance and start a second, redundant open flow via `MainActivity`; `onResume` lets an instance + * reclaim this registration whenever it becomes foreground-active again, in case a different, + * stale-duplicate instance briefly registered over it and was destroyed without anything else + * restoring it. The `isFinishing`/`isDestroyed` filter here still excludes an instance that + * registered but is already tearing down. + */ + fun getLiveActivity(): EditorHandlerActivity? = activityRef?.get()?.takeIf { !it.isFinishing && !it.isDestroyed } +} diff --git a/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt b/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt index 189eb6f287..a2906f6c11 100644 --- a/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt +++ b/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt @@ -241,8 +241,9 @@ class EditorProviderImpl( column: Int, ): Boolean { val activity = activity() ?: return false - val pos = Position(line.coerceAtLeast(0), column.coerceAtLeast(0)) - activity.openFileAndSelect(file, Range(pos, pos)) + // pointRange rather than an open-coded Range(pos, pos): same factory, and it keeps this + // Position out of the Range that openFileAndSelect's pipeline goes on to clamp in place. + activity.openFileAndSelect(file, Range.pointRange(Position(line.coerceAtLeast(0), column.coerceAtLeast(0)))) return true } diff --git a/app/src/main/java/com/itsaky/androidide/deeplink/ConsumedRequests.kt b/app/src/main/java/com/itsaky/androidide/deeplink/ConsumedRequests.kt new file mode 100644 index 0000000000..7c8e114c65 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/deeplink/ConsumedRequests.kt @@ -0,0 +1,132 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.deeplink + +import android.os.Parcelable + +/** + * The deep-link-style requests (e.g. [com.itsaky.androidide.models.DeepLinkRequest], + * [com.itsaky.androidide.models.PendingFileRequest]) this task has already acted on, so a + * redelivered Intent carrying one of them does not force its navigation a second time (ADFA-5067). + * `Intent.removeExtra` alone cannot provide this: it mutates only this process's Intent object, + * while a recreate after process death is handed the system's *parceled* copy, extras intact -- + * which is why consumers persist this set through `onSaveInstanceState`. + * + * Every consumed request is remembered, not just the latest. One slot was not enough: after link A + * is consumed and link B arrives through `onNewIntent`, `setIntent` makes B the live Intent while + * the task still holds A as its launch Intent -- and that is the Intent a recreate after process + * death is given. A then failed a "same as the last consumed request" test and reopened its project, + * which is the very loss the single field existed to prevent. + * + * Kept out of the activity so the bookkeeping can be tested without one: this is the third distinct + * lifecycle path (config change, process death, second link) whose correctness rests entirely on it. + */ +class ConsumedRequests { + private val requests = LinkedHashSet() + + /** + * The launch-Intent entry: the first request this set ever recorded, and the one Android replays + * verbatim after process death. Held by value rather than by position because [add] reorders the + * set to track recency -- a re-add used to slide this entry off slot 0, leaving the positional + * pin in [add]'s eviction loop protecting whichever arbitrary request happened to land there. + */ + private var pinned: T? = null + + /** For `onSaveInstanceState`; pairs with [restore]. [pinned] leads, so [restore] can re-establish it. */ + fun toSavedList(): ArrayList { + val saved = ArrayList(requests.size) + pinned?.let(saved::add) + requests.filterTo(saved) { it != pinned } + return saved + } + + /** + * Replaces the contents with [saved], which is null when there is no instance state to restore. + * + * [MAX_REMEMBERED] is re-applied here: the cap exists to bound the saved Bundle against a sender + * firing links in a loop, and restoring an over-long list unchecked would carry an oversized set + * straight back into the next `onSaveInstanceState`. + */ + fun restore(saved: List?) { + requests.clear() + pinned = saved?.firstOrNull() + saved?.let(requests::addAll) + evictExcess() + } + + operator fun contains(request: T): Boolean = request in requests + + /** + * Forgets [request], so an equal-by-value request deliberately re-armed by the caller (e.g. the + * same file/line navigation requested a second time, parked on the Intent for a deferred apply) + * is not mistaken for the already-consumed earlier one and silently skipped. + */ + fun remove(request: T) { + requests.remove(request) + // Dropping the pinned entry releases the pin too, so the next add() re-establishes it rather + // than leaving a pin on a request no longer in the set -- which would exempt nothing and let + // eviction reach the real launch entry again. + if (pinned == request) { + pinned = null + } + } + + /** + * Records [request] as acted on. Null is accepted and ignored: the caller's "latest request" can + * legitimately be unset by the time a confirmation dialog is answered. + * + * Eviction past [MAX_REMEMBERED] keeps the saved Bundle bounded against a sender that fires links + * in a loop -- but it deliberately does NOT evict [pinned], the request on the task's launch + * Intent. That is the one entry this class exists to remember, since it is the Intent Android + * replays verbatim after process death; evicting it force-reopened its project over whatever the + * user was doing, the regression this class was written to prevent. + * + * A re-add refreshes an entry's position so "oldest" tracks use rather than first sighting -- + * except for [pinned], which is left where it is. Reordering it was the bug: the pin used to be + * positional (slot 0), so re-tapping the same URL, or [remove] followed by [add] on a re-armed + * request, slid the launch entry down and handed its protection to an unrelated request. + */ + fun add(request: T?) { + request ?: return + if (pinned == null) { + pinned = request + } + // Re-insert so position tracks recency: `requests += request` leaves an existing element where + // it was, which made a repeatedly-seen request look like the least recent. Skipped for the + // pinned entry, whose position no longer carries meaning and must stay stable for [toSavedList]. + if (request != pinned) { + requests.remove(request) + } + requests += request + evictExcess() + } + + /** Drops the oldest non-[pinned] entries until the set is back within [MAX_REMEMBERED]. */ + private fun evictExcess() { + while (requests.size > MAX_REMEMBERED) { + // firstOrNull, not first(): a set of nothing but the pinned entry has no eligible victim, + // and looping forever on it would hang whichever lifecycle callback got here. + val victim = requests.firstOrNull { it != pinned } ?: return + requests.remove(victim) + } + } + + private companion object { + const val MAX_REMEMBERED = 32 + } +} diff --git a/app/src/main/java/com/itsaky/androidide/deeplink/PendingDeepLinkOpen.kt b/app/src/main/java/com/itsaky/androidide/deeplink/PendingDeepLinkOpen.kt new file mode 100644 index 0000000000..ef82fa390b --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/deeplink/PendingDeepLinkOpen.kt @@ -0,0 +1,75 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.deeplink + +import com.itsaky.androidide.models.DeepLinkOpenRequest + +/** + * In-memory, process-lifetime handoff for "the user confirmed closing the current project via a + * deep link; once this activity instance is actually destroyed, open the requested project." + * + * Deliberately not acted on synchronously inside the close-confirmation dialog's button callback -- + * see [com.itsaky.androidide.activities.editor.EditorHandlerActivity.onDestroy] for why the hand-off + * must wait until the old, `singleTask` activity instance is guaranteed torn down. + * + * Koin-provided (`single` in `di/AppModule.kt`) rather than a Kotlin `object`, per ADR 0006 -- + * still one process-wide instance either way, but this keeps it substitutable in tests and out of + * the "hand-rolled singleton" pattern the ADR asks new code to avoid. + */ +internal class PendingDeepLinkOpen { + @Volatile + private var request: DeepLinkOpenRequest? = null + + /** + * The activity instance that armed [request], compared by identity only and never dereferenced, + * so holding it here cannot keep a destroyed activity alive in any way that matters -- the + * reference is dropped the moment the hand-off is drained or superseded. + */ + @Volatile + private var owner: Any? = null + + /** Records the hand-off [owner] confirmed, replacing any earlier one. */ + fun arm( + owner: Any, + request: DeepLinkOpenRequest, + ) { + this.owner = owner + this.request = request + } + + /** + * Hands back and clears the request [owner] armed, or null if it armed none -- so an instance + * only ever performs its own hand-off. + * + * The ownership test replaces the `isFinishing && didCompleteLiveOnCreate` pair this used to be + * gated on, which asked two different questions and got both wrong at the edges. isFinishing was + * false whenever the deferred `finish()` never ran (its `lifecycleScope` coroutine cancelled at + * ON_DESTROY, or a config-change recreate landing mid-save), so a confirmed switch was stranded + * in this process-wide `single` and fired later against an unrelated project close. + * didCompleteLiveOnCreate was standing in for "did I arm this?" -- which is now asked directly. + */ + fun drainArmedBy(owner: Any): DeepLinkOpenRequest? { + if (this.owner !== owner) { + return null + } + val drained = request + request = null + this.owner = null + return drained + } +} diff --git a/app/src/main/java/com/itsaky/androidide/di/AppModule.kt b/app/src/main/java/com/itsaky/androidide/di/AppModule.kt index 0e3b3f65f4..b1aece6ba5 100644 --- a/app/src/main/java/com/itsaky/androidide/di/AppModule.kt +++ b/app/src/main/java/com/itsaky/androidide/di/AppModule.kt @@ -1,10 +1,12 @@ package com.itsaky.androidide.di - import com.itsaky.androidide.actions.FileActionManager import com.itsaky.androidide.analytics.AnalyticsManager import com.itsaky.androidide.analytics.IAnalyticsManager +import com.itsaky.androidide.deeplink.PendingDeepLinkOpen import com.itsaky.androidide.git.core.GitCredentialsManager +import com.itsaky.androidide.repositories.RecentProjectRepository +import com.itsaky.androidide.repositories.RecentProjectRepositoryImpl import com.itsaky.androidide.roomData.recentproject.RecentProjectRoomDatabase import com.itsaky.androidide.viewmodel.CloneRepositoryViewModel import com.itsaky.androidide.viewmodel.GitBottomSheetViewModel @@ -14,8 +16,12 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob import org.koin.android.ext.koin.androidApplication import org.koin.android.ext.koin.androidContext -import org.koin.dsl.module import org.koin.core.module.dsl.viewModel +import org.koin.core.qualifier.named +import org.koin.dsl.module + +/** Qualifier for the process-lifetime [CoroutineScope]; see the binding for why it is named. */ +const val APPLICATION_SCOPE = "applicationScope" val coreModule = module { @@ -25,24 +31,32 @@ val coreModule = single { AnalyticsManager() } viewModel { - GitBottomSheetViewModel(get()) + GitBottomSheetViewModel(get()) + } + viewModel { MainViewModel() } + viewModel { CloneRepositoryViewModel(get(), get()) } + + // Named, because an unqualified single is claimed by type alone: this one + // instance was serving both the Room database below and EditorHandlerActivity's saveAllAsync, + // and a second unqualified CoroutineScope added anywhere would silently retarget the save with + // no compile error and no failing test. Consumers now ask for it by name. + single(named(APPLICATION_SCOPE)) { + CoroutineScope(SupervisorJob() + Dispatchers.IO) } - viewModel { MainViewModel(get()) } - viewModel { CloneRepositoryViewModel(get(), get()) } - - single { - CoroutineScope(SupervisorJob() + Dispatchers.IO) - } + single { + RecentProjectRoomDatabase.getDatabase(androidApplication(), get(named(APPLICATION_SCOPE))) + } - single { - RecentProjectRoomDatabase.getDatabase(androidApplication(), get()) - } + single { + get().recentProjectDao() + } - single { - get().recentProjectDao() - } + single { + RecentProjectRepositoryImpl(get()) + } - single { GitCredentialsManager(get()) } + single { GitCredentialsManager(get()) } + single { PendingDeepLinkOpen() } } diff --git a/app/src/main/java/com/itsaky/androidide/fragments/git/GitBottomSheetFragment.kt b/app/src/main/java/com/itsaky/androidide/fragments/git/GitBottomSheetFragment.kt index 689eab81e1..01d43226b5 100644 --- a/app/src/main/java/com/itsaky/androidide/fragments/git/GitBottomSheetFragment.kt +++ b/app/src/main/java/com/itsaky/androidide/fragments/git/GitBottomSheetFragment.kt @@ -33,6 +33,7 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.idetooltips.attachTooltip import com.itsaky.androidide.interfaces.IEditorHandler import com.itsaky.androidide.preferences.internal.GitPreferences +import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.utils.onLongPress import com.itsaky.androidide.viewmodel.BottomSheetViewModel @@ -672,12 +673,42 @@ class GitBottomSheetFragment : Fragment(R.layout.fragment_git_bottom_sheet) { private fun checkUnsavedChangesAndProceed(action: () -> Unit) { val handler = requireActivity() as? IEditorHandler - if (handler?.areFilesModified() == true) { + // hasUnsavedWritableFiles(), matching the post-save check below. areFilesModified() is a cached + // flag that counts read-only archive tabs no save can ever write, so with a .zip or .apk open it + // raised this "save before the git action?" prompt on every commit, pull and push even with + // nothing actually dirty -- and the save it offered could not clear it. + if (handler?.hasUnsavedWritableFiles() == true) { MaterialAlertDialogBuilder(requireContext()) .setTitle(R.string.title_files_unsaved) .setMessage(R.string.msg_save_before_git_action) .setPositiveButton(R.string.save_before_git_action) { _, _ -> - handler.saveAllAsync { action() } + handler.saveAllAsync { succeeded -> + // saveAllAsync is owned by the activity's lifecycle and can still invoke this + // callback after onDestroyView() clears _binding -- the user navigating away while + // the save is in flight -- and action() dereferences binding, so bail out first. + if (_binding == null) { + return@saveAllAsync + } + // succeeded means saveAll() did not throw, not that every write landed: a silent + // per-file failure (disk full, say) leaves a file modified with succeeded still + // true, and running a commit or pull then operates on a tree whose edits were + // never written. + // + // hasUnsavedWritableFiles(), not areFilesModified(): the latter is a cached flag + // that counts read-only archive tabs, which no save ever writes, so with a .zip or + // .apk tab open it stays true forever and every git action here would be refused + // with no way for the user to clear it. Same question EditorHandlerActivity asks + // itself after its own saves. + if (succeeded && !handler.hasUnsavedWritableFiles()) { + action() + } else { + // The String overload, not flashError(Int): the Int one shows a ~1s + // auto-dismissing bar, and a user who looked away would never learn the git + // action was abandoned and their edits are still unwritten. Matches the + // reasoning EditorHandlerActivity spells out on its own save-failure path. + flashError(getString(R.string.save_failed)) + } + } }.setNegativeButton(R.string.no_save_before_git_action) { _, _ -> action() }.setNeutralButton(android.R.string.cancel, null) diff --git a/app/src/main/java/com/itsaky/androidide/interfaces/IEditorHandler.kt b/app/src/main/java/com/itsaky/androidide/interfaces/IEditorHandler.kt index 3a25c882b7..2a02ba5b6f 100644 --- a/app/src/main/java/com/itsaky/androidide/interfaces/IEditorHandler.kt +++ b/app/src/main/java/com/itsaky/androidide/interfaces/IEditorHandler.kt @@ -26,64 +26,104 @@ import java.io.File * @author Akash Yadav */ interface IEditorHandler { - - fun findIndexOfEditorByFile(file: File?) : Int - - fun getCurrentEditor(): CodeEditorView? - fun getEditorAtIndex(index: Int) : CodeEditorView? - fun getEditorForFile(file: File) : CodeEditorView? - - suspend fun openFile(file: File) : CodeEditorView? = openFile(file, null) - suspend fun openFile(file: File, selection: Range?) : CodeEditorView? - fun openFileAndSelect(file: File, selection: Range?) - fun openFileAndGetIndex(file: File, selection: Range?) : Int - - fun areFilesModified(): Boolean - fun areFilesSaving(): Boolean - - /** - * Save all files. - * - * @param notify Whether to notify the user about the save event. - * @param processResources Whether the resources must be generated after the save operation. - * @param progressConsumer A function which consumes the progress of the save operation. - * See [saveAllResult] for more details. - */ - suspend fun saveAll( - notify: Boolean = true, - requestSync: Boolean = true, - processResources: Boolean = false, - progressConsumer: ((progress: Int, total: Int) -> Unit)? = null - ) : Boolean - - /** - * Save all files asynchronously. - * - * @param runAfter A callback function which will be run after the files are saved. - * @see saveAll - */ - fun saveAllAsync( - notify: Boolean = true, - requestSync: Boolean = true, - processResources: Boolean = false, - progressConsumer: ((progress: Int, total: Int) -> Unit)? = null, - runAfter: (() -> Unit)? = null - ) - - /** - * Save all files and get the [SaveResult]. - * - * @param progressConsumer A function which consumes the progress of the save operation. The first - * parameter of the function is the current save progress (saved file count) and the second parameter - * is the total file count. - */ - suspend fun saveAllResult(progressConsumer: ((progress: Int, total: Int) -> Unit)? = null) : SaveResult - suspend fun saveResult(index: Int, result: SaveResult) - - fun closeFile(index: Int) = closeFile(index) {} - fun closeFile(index: Int, runAfter: () -> Unit) - fun closeAll() = closeAll {} - fun closeAll(runAfter: () -> Unit) - fun closeOthers() - fun openFAQActivity(htmlData: String) -} \ No newline at end of file + fun findIndexOfEditorByFile(file: File?): Int + + fun getCurrentEditor(): CodeEditorView? + + fun getEditorAtIndex(index: Int): CodeEditorView? + + fun getEditorForFile(file: File): CodeEditorView? + + suspend fun openFile(file: File): CodeEditorView? = openFile(file, null) + + suspend fun openFile( + file: File, + selection: Range?, + ): CodeEditorView? + + fun openFileAndSelect( + file: File, + selection: Range?, + ) + + fun openFileAndGetIndex( + file: File, + selection: Range?, + ): Int + + fun areFilesModified(): Boolean + + /** + * Whether any open file still holds unsaved edits *that a save could actually have written*. + * + * Not the same question as [areFilesModified], which is a cached flag recomputed only as a side + * effect of a successful per-file write, and which counts read-only archive tabs (.zip/.apk and + * friends) that `CodeEditorView.save()` never writes. Asking [areFilesModified] after a save to + * decide whether the save worked therefore answers "still modified" forever whenever such a tab + * is open, which is why this exists. + */ + fun hasUnsavedWritableFiles(): Boolean + + fun areFilesSaving(): Boolean + + /** + * Save all files. + * + * @param notify Whether to notify the user about the save event. + * @param processResources Whether the resources must be generated after the save operation. + * @param progressConsumer A function which consumes the progress of the save operation. + * See [saveAllResult] for more details. + */ + suspend fun saveAll( + notify: Boolean = true, + requestSync: Boolean = true, + processResources: Boolean = false, + progressConsumer: ((progress: Int, total: Int) -> Unit)? = null, + ): Boolean + + /** + * Save all files asynchronously. + * + * @param runAfter A callback function which will be run after the save attempt is over, whether + * it succeeded or not, receiving `true` iff every file saved without throwing. Callers that act + * on the saved state (e.g. proceeding with a git operation) must check this rather than assuming + * the callback firing means the save succeeded. + * @see saveAll + */ + fun saveAllAsync( + notify: Boolean = true, + requestSync: Boolean = true, + processResources: Boolean = false, + progressConsumer: ((progress: Int, total: Int) -> Unit)? = null, + runAfter: ((succeeded: Boolean) -> Unit)? = null, + ) + + /** + * Save all files and get the [SaveResult]. + * + * @param progressConsumer A function which consumes the progress of the save operation. The first + * parameter of the function is the current save progress (saved file count) and the second parameter + * is the total file count. + */ + suspend fun saveAllResult(progressConsumer: ((progress: Int, total: Int) -> Unit)? = null): SaveResult + + suspend fun saveResult( + index: Int, + result: SaveResult, + ) + + fun closeFile(index: Int) = closeFile(index) {} + + fun closeFile( + index: Int, + runAfter: () -> Unit, + ) + + fun closeAll() = closeAll {} + + fun closeAll(runAfter: () -> Unit) + + fun closeOthers() + + fun openFAQActivity(htmlData: String) +} diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt new file mode 100644 index 0000000000..926afbe085 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -0,0 +1,260 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +import android.net.Uri +import android.os.Parcelable +import kotlinx.parcelize.Parcelize + +/** + * A request to open a file at an optional line/column, carried as part of a [DeepLinkRequest] or a + * [DeepLinkOpenRequest]. + * + * [lineRaw]/[columnRaw] are kept as raw strings rather than parsed [Int]s so that callers can + * distinguish "segment absent from the URL" (`null`) from "segment present but not a valid positive + * integer" (non-null, fails [String.toIntOrNull] or non-positive) -- the latter must be reported to the + * user, the former must not. + */ +@Parcelize +data class PendingFileRequest( + val filePath: String, + val lineRaw: String?, + val columnRaw: String?, +) : Parcelable { + companion object { + const val EXTRA_KEY = "com.itsaky.androidide.PENDING_FILE_REQUEST" + } +} + +/** + * A parsed (but not yet resolved-to-a-path) request for + * `https://appdevforall.org/device/open/project/{projectName}[/file/{filename}[/line/{n}[/column/{n}]]]` + * (the `www` subdomain works identically -- see [HOSTS]). + */ +@Parcelize +data class DeepLinkRequest( + val projectName: String, + val fileRequest: PendingFileRequest? = null, +) : Parcelable { + companion object { + const val EXTRA_KEY = "com.itsaky.androidide.DEEP_LINK_REQUEST" + + private const val SCHEME = "https" + + // Both hosts serve an identical, verified assetlinks.json (see AndroidManifest.xml's matching + // pair of elements on DeepLinkActivity's intent-filter) -- kept in sync with that list. + private val HOSTS = setOf("www.appdevforall.org", "appdevforall.org") + private const val PATH_PREFIX = "/device/open/project/" + + /** + * See [parse]: an upper bound on the whole path, since the parsed pieces get parcelled. + * + * Sized against the Binder budget rather than the filesystem. Up to + * `ConsumedRequests.MAX_REMEMBERED` (32) of these are held per set, Parcel writes UTF-16 + * (2 bytes per char plus a length), and BaseEditorActivity saves TWO such sets while + * MainActivity saves a third -- so the ceiling that matters is 32 x 3 x 2 bytes x this value. + * At 4096 that was ~768 KB against a ~1 MB transaction limit, which is not a bound at all. At + * 512 it is ~96 KB. Still far above any real project path: Linux caps a single name at 255 + * bytes, and a link longer than this cannot name a project that exists. + */ + private const val MAX_LINK_PATH_LENGTH = 512 + + private const val SEGMENT_PROJECT = "project" + private const val SEGMENT_FILE = "file" + private const val SEGMENT_LINE = "line" + private const val SEGMENT_COLUMN = "column" + + /** First index at or after [from] holding [segment], or -1. Unlike [List.indexOf], never + * matches an already-consumed segment earlier in the path -- e.g. a project name that + * happens to equal `"line"` can't be mistaken for the `line` keyword that follows it. */ + private fun List.indexOfFrom( + from: Int, + segment: String, + ): Int { + for (i in from until size) { + if (this[i] == segment) return i + } + return -1 + } + + /** + * Peels a trailing `keyword`/value pair off the end of `this[startIdx until endIdx]`, or a + * bare, valueless `keyword` at the very last position (e.g. a URL ending in `.../column` with + * nothing after it). Returns the raw value paired with the new `endIdx` (that segment, and its + * value if any, excluded) -- `null` raw if `keyword` wasn't found at all (endIdx unchanged), + * `""` raw if found dangling with no value, e.g. -- see [parse]'s inline docs for why there's + * no numeric check on the paired value itself. + */ + private fun List.peelTrailingKeyword( + startIdx: Int, + endIdx: Int, + keyword: String, + ): Pair { + val pairIdx = (endIdx - 2).takeIf { it >= startIdx && this[it] == keyword } + if (pairIdx != null) { + return this[pairIdx + 1] to pairIdx + } + val danglingIdx = (endIdx - 1).takeIf { it >= startIdx && this[it] == keyword } + if (danglingIdx != null) { + return "" to danglingIdx + } + return null to endIdx + } + + /** + * Parses a deep-link [Uri] of the form described in [DeepLinkRequest]'s docs. Returns `null` if + * the URI does not match this scheme/host/path at all, or does not contain a `project` segment + * followed by a name -- i.e. it isn't a deep link this app understands, not merely a deep link + * with missing optional parts. + * + * [DeepLinkActivity][com.itsaky.androidide.activities.DeepLinkActivity] is `exported="true"` (a + * requirement for App Links), which means its `` data scoping only constrains + * *implicit* intent matching -- any co-installed app can still target it directly with an + * explicit intent carrying an arbitrary [Uri]. Re-checking scheme/host/path prefix here, rather + * than trusting the manifest declaration alone, closes that gap regardless of how the intent + * arrived. + */ + fun parse(uri: Uri?): DeepLinkRequest? { + // Scheme and host are case-insensitive per RFC 3986 -- an explicit intent from another app + // (see this function's own doc on why that's re-validated at all) could carry either in + // non-canonical case, and a semantically valid link must not be rejected over that alone. + if (uri == null || + !uri.scheme.equals(SCHEME, ignoreCase = true) || + HOSTS.none { it.equals(uri.host, ignoreCase = true) } || + uri.path?.startsWith(PATH_PREFIX) != true + ) { + return null + } + + // Uri.pathSegments silently drops empty segments, shifting everything after them one slot + // left -- ".../project//file/Main.kt" would pass the prefix check above and then parse as + // a project literally named "file" (and open it, should one exist). Reject the malformed + // link outright instead of resolving a name the user never wrote. + if (uri.path?.contains("//") == true) { + return null + } + + // A length ceiling, because everything downstream of here is parcelled. DeepLinkActivity is + // exported, so any co-installed app can send explicit ACTION_VIEW intents in a loop; the + // parsed name and path are kept in ConsumedRequests (up to 32 of them) and written verbatim + // to MainActivity's saved-instance Bundle, so unbounded names cross the ~1 MB Binder budget + // and crash the activity with TransactionTooLargeException on every rotation or + // backgrounding until the task is cleared. Rejecting is safe: this bounds a legitimate + // path far above anything the filesystem accepts (Linux caps a single name at 255 bytes), + // and a link this long cannot name a real project anyway. + if ((uri.path?.length ?: 0) > MAX_LINK_PATH_LENGTH) { + return null + } + + val segments = uri.pathSegments + + val projectIdx = segments.indexOfFrom(0, SEGMENT_PROJECT) + if (projectIdx < 0 || projectIdx + 1 >= segments.size) { + return null + } + val projectName = segments[projectIdx + 1] + + val fileIdx = segments.indexOfFrom(projectIdx + 2, SEGMENT_FILE) + val fileRequest = + fileIdx.takeIf { it >= 0 }?.let { fIdx -> + val startIdx = fIdx + 1 + if (startIdx >= segments.size) { + return@let null + } + + // line/column are trailing modifiers, so -- unlike the project/file lookup above -- + // they're matched from the END of the path backward (column peeled off first, then + // line against whatever remains), never by searching for the keyword's first + // occurrence. That makes a literal "line"/"column" segment earlier in the file path + // (e.g. a directory named "line") part of the filename rather than misread as + // metadata, as long as a real trailing pair follows it. Peeling column off before + // checking for line (rather than computing both against the original, un-trimmed end) + // matters for a case like ".../Main.kt/line/5/column": a bare trailing "column" with + // no value consumed first re-exposes "line/5" as a real pair for the line check that + // follows, instead of two independent checks both missing it against the original end. + // The shape this can't resolve: any path whose last two segments happen to be + // [directory-literally-named "line"/"column", some other segment] -- not just the + // degenerate two-segment case (`file/line/Main.kt` alone), but equally a longer one + // (`file/foo/line/Notes.txt`, where "foo" is a real preceding directory). Neither is + // distinguishable from an actual line/column suffix by position alone, and this URL + // scheme has no delimiter to tell them apart -- there's no numeric-lookahead check on + // the value segment because that would instead break the *intentional* "malformed but + // present" case this class's docs call out (e.g. `.../line/abc`, which must surface as + // an invalid line number, not silently become part of the file path). Both read as the + // keyword (existing behavior, unchanged); a user who genuinely has a directory named + // "line"/"column" must avoid placing the target file's segment where it would be + // misread as the value. + var endIdx = segments.size + val (columnRaw, endIdxAfterColumn) = segments.peelTrailingKeyword(startIdx, endIdx, SEGMENT_COLUMN) + endIdx = endIdxAfterColumn + val (lineRaw, endIdxAfterLine) = segments.peelTrailingKeyword(startIdx, endIdx, SEGMENT_LINE) + endIdx = endIdxAfterLine + + val filePath = segments.subList(startIdx, endIdx).joinToString("/") + + // `.../file/line/5` peels "line/5" off and leaves nothing between startIdx and + // endIdx, so the file request would carry an empty path -- which resolves against + // nothing and surfaces to the user as the nonsense `File "" was not found in the + // project.` The link named no file, so it is a plain project-open link; the line and + // column, having nothing to apply to, go with it. + if (filePath.isEmpty()) { + null + } else { + PendingFileRequest( + filePath = filePath, + lineRaw = lineRaw, + columnRaw = columnRaw, + ) + } + } + + return DeepLinkRequest(projectName = projectName, fileRequest = fileRequest) + } + } +} + +/** + * The resolved-path counterpart to [DeepLinkRequest], used once the project name has been resolved to + * an absolute directory -- e.g. when handing a pending "close current project, then open this one" off + * across activities via [com.itsaky.androidide.deeplink.PendingDeepLinkOpen]. + */ +@Parcelize +data class DeepLinkOpenRequest( + val projectRoot: String, + val fileRequest: PendingFileRequest?, + /** + * Whether `recordProjectOpenedBookkeeping` has already run for [projectRoot]. + * + * True for the plain project-switch path, where MainActivity.openProject records the open before + * it even sends the intent; false for a deep link arriving at an already-live editor, which + * never goes through MainActivity at all. Without the distinction the hand-off recorded every + * plain switch twice -- two Recents inserts and, more visibly, two `trackProjectOpened` events + * for one user action. + */ + val bookkeepingAlreadyRecorded: Boolean = false, + /** + * The project that was open when this switch was requested. + * + * Captured at request time, because by the time the handoff is performed the live + * `IProjectManager` global no longer holds it: on the plain-switch path + * `MainActivity.openProject` overwrote it with the NEW path before the intent was even delivered. + * Reading the global there produced previous == new, which made the receiver treat a confirmed + * switch as a same-project no-op. + */ + val previousProjectPath: String? = null, +) : Parcelable diff --git a/app/src/main/java/com/itsaky/androidide/models/EditorIntentExtras.kt b/app/src/main/java/com/itsaky/androidide/models/EditorIntentExtras.kt new file mode 100644 index 0000000000..4030b60648 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/models/EditorIntentExtras.kt @@ -0,0 +1,56 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +/** + * The Intent extras MainActivity and the editor activities pass between themselves. + * + * Spelled out here rather than repeated as string literals at each of their ~13 call sites across + * three files. A typo in one of those reads back as a missing extra -- silently, at runtime -- and + * [EXTRA_PREVIOUS_PROJECT_PATH] in particular is what the same-project-vs-genuine-switch decision + * turns on, so getting it wrong makes every switch look like a no-op. The values keep their original + * unqualified spelling: they are read from Intents that a previous install may have persisted, and + * renaming them now would silently drop those. + */ +object EditorIntentExtras { + /** Absolute path of the project the editor should open. */ + const val EXTRA_PROJECT_PATH = "PROJECT_PATH" + + /** + * Absolute path of the project that was open *before* this Intent was built. + * + * Sent because `recordProjectOpenedBookkeeping` overwrites the live `IProjectManager` global to + * the new path before the Intent is even delivered, so by the time the editor reads it there is + * no longer any way to tell what it was. + */ + const val EXTRA_PREVIOUS_PROJECT_PATH = "PREVIOUS_PROJECT_PATH" + + /** + * Set only on the Intent [com.itsaky.androidide.activities.editor.BaseEditorActivity] sends back to + * MainActivity when a deep link names a project other than the one it holds. + * + * That is a programmatic re-delivery of a request the user tapped once, so MainActivity must apply + * its consumed-requests gate to it. A link arriving without this flag is a fresh tap -- possibly of + * the very same URL, which carries no nonce and is therefore equal by value to an earlier one -- + * and must be acted on, not silently swallowed as a duplicate. + */ + const val EXTRA_REFORWARDED_DEEP_LINK = "REFORWARDED_DEEP_LINK" + + /** Set when the project was created from a template that reported issues. */ + const val EXTRA_HAS_TEMPLATE_ISSUES = "HAS_TEMPLATE_ISSUES" +} diff --git a/app/src/main/java/com/itsaky/androidide/repositories/RecentProjectRepository.kt b/app/src/main/java/com/itsaky/androidide/repositories/RecentProjectRepository.kt new file mode 100644 index 0000000000..2c863b552b --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/repositories/RecentProjectRepository.kt @@ -0,0 +1,36 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.repositories + +import com.itsaky.androidide.roomData.recentproject.RecentProject + +/** + * Repository for recording a project's presence in Recents -- keeps [RecentProjectDao] (a Room + * data source) out of the UI layer, per ARCHITECTURE.md's UI -> ViewModel -> Repository -> data + * source layering. + */ +interface RecentProjectRepository { + /** Inserts [recentProject] into Recents; a no-op if a row for its location already exists. */ + suspend fun insert(recentProject: RecentProject) + + /** Updates the detected language for the Recents row at [location]. */ + suspend fun updateLanguage( + location: String, + language: String, + ) +} diff --git a/app/src/main/java/com/itsaky/androidide/repositories/RecentProjectRepositoryImpl.kt b/app/src/main/java/com/itsaky/androidide/repositories/RecentProjectRepositoryImpl.kt new file mode 100644 index 0000000000..617cbec725 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/repositories/RecentProjectRepositoryImpl.kt @@ -0,0 +1,32 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.repositories + +import com.itsaky.androidide.roomData.recentproject.RecentProject +import com.itsaky.androidide.roomData.recentproject.RecentProjectDao + +class RecentProjectRepositoryImpl( + private val recentProjectDao: RecentProjectDao, +) : RecentProjectRepository { + override suspend fun insert(recentProject: RecentProject) = recentProjectDao.insert(recentProject) + + override suspend fun updateLanguage( + location: String, + language: String, + ) = recentProjectDao.updateLanguage(location, language) +} diff --git a/app/src/main/java/com/itsaky/androidide/ui/CodeEditorView.kt b/app/src/main/java/com/itsaky/androidide/ui/CodeEditorView.kt index 4324eec039..e50f376c59 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/CodeEditorView.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/CodeEditorView.kt @@ -90,7 +90,9 @@ import kotlin.math.abs private const val MIN_FONT_SIZE = EditorPreferences.FONT_SIZE_MIN private const val DEFAULT_FONT_SIZE = EditorPreferences.FONT_SIZE_DEFAULT private const val MAX_FONT_SIZE = EditorPreferences.FONT_SIZE_MAX -private val ARCHIVE_EXTENSIONS = setOf("apk", PLUGIN_ARCHIVE_EXTENSION, TEMPLATE_ARCHIVE_EXTENSION, "zip") + +/** File extensions [CodeEditorView.save] never writes -- these are opened read-only. */ +internal val ARCHIVE_EXTENSIONS = setOf("apk", PLUGIN_ARCHIVE_EXTENSION, TEMPLATE_ARCHIVE_EXTENSION, "zip") /** * A view that handles opened code editor. diff --git a/app/src/main/java/com/itsaky/androidide/utils/DeepLinkProjectResolution.kt b/app/src/main/java/com/itsaky/androidide/utils/DeepLinkProjectResolution.kt new file mode 100644 index 0000000000..c4cae1661f --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/DeepLinkProjectResolution.kt @@ -0,0 +1,100 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import android.app.Activity +import com.itsaky.androidide.resources.R.string +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext +import org.slf4j.LoggerFactory +import java.io.File + +private val log = LoggerFactory.getLogger("DeepLinkProjectResolution") + +/** + * The outcome of [resolveDeepLinkProject]. Keeps "no such project" apart from "could not tell", + * which a bare `File?` collapsed: callers record a definitive absence so the link stops re-reporting + * itself on every recreate, and recording an [Unverifiable] the same way made a momentary + * filesystem failure kill a perfectly valid link permanently (ADFA-5067 review). + */ +sealed interface DeepLinkProjectLookup { + data class Found( + val projectDir: File, + ) : DeepLinkProjectLookup + + /** No project of that name exists. Definitive, so callers may remember it. */ + data object NotFound : DeepLinkProjectLookup + + /** The lookup failed for a reason unrelated to the project's existence. Remember nothing. */ + data object Unverifiable : DeepLinkProjectLookup +} + +/** + * Resolves [projectName] to a validated project directory under [projectsRoot] for a deep link, + * reporting every failure to the user via `flashError` on the main thread. The caller can return on + * anything but [DeepLinkProjectLookup.Found] -- each failure case has already shown its own message + * -- but must consult *which* failure it was before recording the request as dealt with. + * + * Call from a background dispatcher (e.g. `Dispatchers.IO`); this only switches to + * [Dispatchers.Main] itself for the user-facing error messages. + */ +suspend fun Activity.resolveDeepLinkProject( + projectsRoot: File, + projectName: String, +): DeepLinkProjectLookup { + val lookup = + try { + lookupValidProjectByName(projectsRoot, projectName) + } catch (e: CancellationException) { + throw e + } catch (e: SecurityException) { + log.error("Failed to scan {} for deep link", projectsRoot, e) + flashOnMain(getString(string.msg_deeplink_scan_failed)) + // A denied scan says nothing about whether the project is there. + return DeepLinkProjectLookup.Unverifiable + } + + return when (lookup) { + is ProjectNameLookup.Found -> { + DeepLinkProjectLookup.Found(lookup.dir) + } + + is ProjectNameLookup.Unverifiable -> { + log.error("Could not determine whether project {} exists under {}", projectName, projectsRoot, lookup.cause) + // Deliberately the scan-failed message, not "no project named X": telling the user that a + // project they can see in the projects list does not exist is worse than saying the + // lookup failed. + flashOnMain(getString(string.msg_deeplink_scan_failed)) + DeepLinkProjectLookup.Unverifiable + } + + ProjectNameLookup.NotFound -> { + flashOnMain(getString(string.msg_deeplink_project_not_found, projectName)) + DeepLinkProjectLookup.NotFound + } + } +} + +private suspend fun Activity.flashOnMain(message: String) { + withContext(Dispatchers.Main) { + // Re-checked here, not before the hop -- the activity can start finishing during the hop + // itself, and a check taken only beforehand would miss that window. + if (!isFinishing && !isDestroyed) flashError(message) + } +} diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.kt new file mode 100644 index 0000000000..83c7f4aba3 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.kt @@ -0,0 +1,96 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import androidx.lifecycle.ProcessLifecycleOwner +import androidx.lifecycle.lifecycleScope +import com.itsaky.androidide.analytics.IAnalyticsManager +import com.itsaky.androidide.preferences.internal.GeneralPreferences +import com.itsaky.androidide.projects.ProjectManagerImpl +import com.itsaky.androidide.repositories.RecentProjectRepository +import com.itsaky.androidide.roomData.recentproject.RecentProject +import com.itsaky.androidide.templates.Language +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch +import org.slf4j.LoggerFactory +import java.io.File + +private val log = LoggerFactory.getLogger("ProjectOpenBookkeeping") + +/** + * Marks [root] as the currently open project (singleton state + last-opened pref), records it in + * Recents, and tracks the open in analytics -- the same bookkeeping + * [com.itsaky.androidide.activities.MainActivity.openProject] does for a normal manual open, + * extracted so a deep-link-triggered project switch gets it too even though that path bypasses + * `openProject` entirely (see + * [com.itsaky.androidide.activities.editor.EditorHandlerActivity.onDestroy]). + * + * [recentProjectRepository] is the caller's Koin-provided instance (`by inject()`) -- per ADR + * 0001/0006, persistence is always acquired through Koin, never by re-deriving the database + * directly, and kept behind this repository interface (not the raw DAO) so callers in the UI layer + * (both [com.itsaky.androidide.activities.MainActivity] and + * [com.itsaky.androidide.activities.editor.EditorHandlerActivity]) don't depend on a Room data + * source directly, per ARCHITECTURE.md's UI -> ViewModel -> Repository -> data source layering. + * + * Uses [ProcessLifecycleOwner]'s scope rather than a per-activity one, since this can run from an + * activity's `onDestroy()` after its own `lifecycleScope` has already been cancelled. + */ +fun recordProjectOpenedBookkeeping( + recentProjectRepository: RecentProjectRepository, + root: File, + project: RecentProject?, + analyticsManager: IAnalyticsManager, +) { + ProjectManagerImpl.getInstance().projectPath = root.absolutePath + GeneralPreferences.lastOpenedProject = root.absolutePath + + ProcessLifecycleOwner.get().lifecycleScope.launch(Dispatchers.IO) { + val location = root.absolutePath + val recentProject = + project ?: RecentProject( + name = root.name, + location = location, + createdAt = getCreatedTime(location).toString(), + lastModified = getLastModifiedTime(location).toString(), + language = readProjectLanguage(root), + ) + try { + // Insert is IGNOREd for a project already in Recents, so refresh the detected language + // separately -- but never clobber a stored value with a failed ("Unknown") detection. + recentProjectRepository.insert(recentProject) + if (!recentProject.language.equals(Language.Unknown.lang, ignoreCase = true)) { + recentProjectRepository.updateLanguage(recentProject.location, recentProject.language) + } + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // This runs on ProcessLifecycleOwner's permanent, app-wide scope, which has no + // CoroutineExceptionHandler -- unlike the ViewModel-scoped version this replaced, ANY + // escaping Exception here (not just SQLException; Room's generated insert can also throw + // e.g. IllegalStateException from an already-closed database) crashes the whole process, + // not just fails to record one Recents entry. The project-open state above is already set + // synchronously, so a Recents-write failure doesn't affect it. Deliberately narrower than + // Throwable: a genuine JVM Error (OutOfMemoryError, StackOverflowError) should still crash + // and get reported rather than being silently downgraded to this warning. + log.warn("Failed to record opened project '{}' in Recents", recentProject.name, e) + } + } + + analyticsManager.trackProjectOpened(root.absolutePath) +} diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 4859e048c8..10db33199a 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -1,6 +1,8 @@ package com.itsaky.androidide.utils import java.io.File +import java.io.IOException +import java.text.Normalizer import kotlin.collections.filter import kotlin.collections.orEmpty @@ -11,14 +13,148 @@ internal fun File.isProjectCandidateDir(): Boolean = isDirectory && canRead() && internal fun findValidProjects(projectsRoot: File): List { if (!projectsRoot.isProjectCandidateDir()) return emptyList() - val subdirs = projectsRoot.listFiles() - ?.filter { it.isProjectCandidateDir() } - .orEmpty() + val subdirs = + projectsRoot + .listFiles() + ?.filter { it.isProjectCandidateDir() } + .orEmpty() if (subdirs.isEmpty()) return emptyList() return subdirs.filter { dir -> isValidProjectDirectory(dir) } } +/** + * The outcome of [lookupValidProjectByName], which unlike a bare `File?` keeps "no project by that + * name" apart from "whether one exists could not be determined". + * + * Callers act on the two very differently: a definitive absence is worth remembering (so a link + * naming a project that does not exist stops re-reporting itself on every recreate), while an + * unverifiable result says nothing at all about the project and must leave every such decision + * untouched. + */ +internal sealed interface ProjectNameLookup { + data class Found( + val dir: File, + ) : ProjectNameLookup + + /** No project of that name exists under the projects root. */ + data object NotFound : ProjectNameLookup + + /** + * A filesystem failure other than absence (EACCES right after a storage-permission change, EIO + * on a flaky SD/FUSE mount) stopped the lookup from reaching an answer. + */ + data class Unverifiable( + val cause: IOException, + ) : ProjectNameLookup +} + +/** + * Resolves [name] directly to `[projectsRoot]/[name]` and validates just that one directory -- + * the O(1) counterpart to [findValidProjects] for callers (e.g. deep links) that already know the + * exact project name and don't need every project under [projectsRoot] scanned to find it. + * + * [name] is attacker-controllable (a deep-link URL segment), so it's resolved through + * [resolveWithinDirectory] rather than a bare `File(projectsRoot, name)` -- [findValidProjects] + * only ever matches against names of directories it already enumerated under [projectsRoot], so it + * can't be pointed outside it, but a direct `File(root, name)` join can (e.g. `name = "../../etc"`). + * + * Reports *why* it found nothing -- see [ProjectNameLookup]; [findValidProjectByName] is this + * reduced to a nullable directory for callers that cannot act on the difference. + */ +internal fun lookupValidProjectByName( + projectsRoot: File, + name: String, +): ProjectNameLookup { + // A project name is always a single path segment. resolveWithinDirectory's lexical check only + // rejects ".."/a leading separator, so without this, name = "." would resolve to projectsRoot + // itself (opening the whole projects directory as "a project" if it happens to satisfy + // isValidProjectDirectory), and an embedded separator like "foo/bar" would resolve two levels + // deep instead of naming a direct child. + if (name.isEmpty() || name == "." || name.contains("/") || name.contains("\\")) { + return ProjectNameLookup.NotFound + } + if (!projectsRoot.isProjectCandidateDir()) return ProjectNameLookup.NotFound + + // A deep-link name is typically authored/normalized as NFC by web tooling, but an on-disk + // project directory imported from elsewhere (e.g. a git clone authored on macOS, which + // decomposes accented filenames to NFD) may not codepoint-match it even though the two look + // identical. Try both normal forms -- still O(1) filesystem lookups, not a directory scan -- + // rather than reporting a visually-identical project as "not found". + val candidateNames = linkedSetOf(name, Normalizer.normalize(name, Normalizer.Form.NFC), Normalizer.normalize(name, Normalizer.Form.NFD)) + // Remembered rather than returned on the spot: a later candidate form may still resolve cleanly, + // and a definite Found has to win over an earlier form's transient IO failure. + var unverifiable: IOException? = null + val resolver = ContainedPathResolver(projectsRoot) + for (candidateName in candidateNames) { + when (val resolution = resolver.resolve(candidateName)) { + is ContainedPathResolver.Resolution.Contained -> { + val candidate = resolution.file + if (candidate.isProjectCandidateDir() && isValidProjectDirectory(candidate)) { + return ProjectNameLookup.Found(candidate) + } + } + + is ContainedPathResolver.Resolution.Unverifiable -> { + unverifiable = resolution.cause + } + + // A traversal attempt is a definitive "not this project", not an unknown. + is ContainedPathResolver.Resolution.Rejected -> { + Unit + } + } + } + return unverifiable?.let(ProjectNameLookup::Unverifiable) ?: ProjectNameLookup.NotFound +} + +/** [lookupValidProjectByName] reduced to the project directory, or null for any other outcome. */ +internal fun findValidProjectByName( + projectsRoot: File, + name: String, +): File? = (lookupValidProjectByName(projectsRoot, name) as? ProjectNameLookup.Found)?.dir + +/** + * True if [a] and [b] name the same project, tolerating an NFC/NFD codepoint difference (e.g. an + * accented project name authored as NFD on macOS vs. the NFC form a deep-link URL typically + * carries) - the same normalization [findValidProjectByName] applies for its filesystem lookup, + * but as a direct string comparison here rather than multiple candidate paths. + */ +internal fun projectNamesMatch( + a: String, + b: String, +): Boolean { + if (a == b) return true + return Normalizer.normalize(a, Normalizer.Form.NFC) == Normalizer.normalize(b, Normalizer.Form.NFC) +} + +/** + * Whether [openProjectPath] is the project a deep link naming [projectName] would resolve to. + * + * The name alone is not enough. A deep link can only ever resolve to `/`, but a + * project can be opened from anywhere -- the file picker (`BaseFragment`'s ACTION_OPEN_DOCUMENT_TREE + * uses the projects dir as a starting hint, not a constraint), Recents, or a clone destination. With + * `/storage/emulated/0/Download/work/MyApp` open and an unrelated `/MyApp` on disk, a + * leaf-name comparison says "same project" and the link's file path is then resolved against the + * OPEN one -- for two clones of a repo the path exists in both, so the wrong file opens silently. + * So the parent has to match as well. + * + * Canonicalised, since either side can reach the same directory through a symlink; a failure to + * canonicalise (an unreadable parent) falls back to the absolute path rather than throwing. + */ +internal fun isDeepLinkTargetOfOpenProject( + openProjectPath: String, + projectName: String, + projectsRoot: File, +): Boolean { + if (openProjectPath.isBlank()) return false + val open = File(openProjectPath) + if (!projectNamesMatch(open.name, projectName)) return false + return canonicalOrAbsolute(open.parentFile ?: return false) == canonicalOrAbsolute(projectsRoot) +} + +private fun canonicalOrAbsolute(file: File): String = runCatching { file.canonicalPath }.getOrElse { file.absolutePath } + /** Determines if the directory contains a valid Android project structure. */ fun isValidProjectDirectory(selectedDir: File): Boolean { if (isPluginProject(selectedDir)) { @@ -56,4 +192,4 @@ internal fun isPluginProject(dir: File): Boolean { val pluginApiJar = File(dir, "libs/plugin-api.jar") val buildGradle = File(dir, "build.gradle.kts") return pluginApiJar.exists() && buildGradle.exists() -} \ No newline at end of file +} diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt index 4d59706af4..6736fd9511 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt @@ -17,34 +17,42 @@ package com.itsaky.androidide.viewmodel -import android.database.SQLException import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.Observer import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope -import com.itsaky.androidide.roomData.recentproject.RecentProject -import com.itsaky.androidide.roomData.recentproject.RecentProjectDao -import com.itsaky.androidide.templates.Language import com.itsaky.androidide.templates.Template -import kotlinx.coroutines.CancellationException -import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch -import org.slf4j.Logger -import org.slf4j.LoggerFactory import java.util.concurrent.atomic.AtomicInteger /** - * [ViewModel] for main activity. + * [ViewModel] for [com.itsaky.androidide.activities.MainActivity] -- holds the single-Activity, + * multi-"screen" navigation state (see the `SCREEN_*` constants) plus one-shot events unrelated to + * persisted UI state. + * + * **Threading:** all mutable state here ([currentScreen], [isTransitionInProgress]) is backed by + * [MutableLiveData] set via direct `.value =` assignment, never `postValue` -- every mutator + * ([setScreen], the [isTransitionInProgress] setter) must run on the main thread. + * + * **Screen state:** [currentScreen]/[previousScreen] are mutually exclusive, identified by one of + * the `SCREEN_*` constants; `-1` is the sentinel for "no screen yet" rather than `null`, since both + * are non-nullable `Int`. [setScreen] records the outgoing screen as [previousScreen] before + * advancing [currentScreen] -- there's no history beyond that one step back. [postTransition] runs + * its `action` immediately unless [isTransitionInProgress] is true, in which case it defers `action` + * until the next transition-complete signal, then detaches its observer (fires at most once). + * + * **Clone-request event:** [requestCloneRepository] is a one-shot, single-consumer event, not + * persisted state -- delivered through a buffered [Channel] exposed as [cloneRepositoryEvent] via + * [kotlinx.coroutines.flow.receiveAsFlow]. A URL sent before any collector attaches is buffered, not + * dropped, but if more than one collector attaches, only one of them receives a given element. * * @author Akash Yadav */ -class MainViewModel( - private val recentProjectDao: RecentProjectDao, -) : ViewModel() { +class MainViewModel : ViewModel() { companion object { // The values assigned to these variables reflect the order in which the screens are presented // to the user. A screen with a lower value is displayed before a screen with a higher value. @@ -60,8 +68,6 @@ class MainViewModel( const val SCREEN_SAVED_PROJECTS = 4 const val SCREEN_DELETE_PROJECTS = 5 const val SCREEN_CLONE_REPO = 6 - - val logger: Logger = LoggerFactory.getLogger(MainViewModel::class.java) } private val _currentScreen = MutableLiveData(-1) @@ -116,22 +122,4 @@ class MainViewModel( action.run() } } - - fun saveProjectToRecents(project: RecentProject) { - viewModelScope.launch(Dispatchers.IO) { - try { - // Insert is IGNOREd for projects already in recents, so refresh the - // detected language separately - but never clobber a stored value - // with a failed detection. - recentProjectDao.insert(project) - if (!project.language.equals(Language.Unknown.lang, ignoreCase = true)) { - recentProjectDao.updateLanguage(project.location, project.language) - } - } catch (e: CancellationException) { - throw e - } catch (e: SQLException) { - logger.warn("Failed to save project to recents", e) - } - } - } } diff --git a/app/src/test/java/com/itsaky/androidide/activities/DeepLinkSetupGateTest.kt b/app/src/test/java/com/itsaky/androidide/activities/DeepLinkSetupGateTest.kt new file mode 100644 index 0000000000..dbfedb2fa2 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/DeepLinkSetupGateTest.kt @@ -0,0 +1,176 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.activities + +import android.content.Context +import android.content.Intent +import android.net.Uri +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.app.configuration.IJdkDistributionProvider +import com.itsaky.androidide.utils.Environment +import com.itsaky.androidide.utils.PermissionsHelper +import io.mockk.every +import io.mockk.mockkObject +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import java.io.File + +/** + * A deep link must not walk past setup. Both of DeepLinkActivity's targets sit beyond + * SplashActivity and OnboardingActivity, which are the only things enforcing terms, permissions, + * the JDK and SDK install, the low-storage check and the x86 exit -- so a link arriving on a fresh + * install used to land the user in an editor that could not build (ADFA-5067 review). + * + * Robolectric's environment has no installed JDK distribution and no ANDROID_HOME, which is exactly + * the not-set-up state under test. + */ +@RunWith(RobolectricTestRunner::class) +class DeepLinkSetupGateTest { + @get:Rule + val tempFolder = TemporaryFolder() + + @Test + fun `a link arriving before setup is finished goes to the launcher chain, not the editor`() { + val intent = + Intent(Intent.ACTION_VIEW, Uri.parse("https://appdevforall.org/device/open/project/MyApp")) + val activity = Robolectric.buildActivity(DeepLinkActivity::class.java, intent).create().get() + + val next = shadowOf(activity).nextStartedActivity + assertThat(next).isNotNull() + assertThat(next.component?.className).isEqualTo(SplashActivity::class.java.name) + assertThat(activity.isFinishing).isTrue() + } + + @Test + fun `the setup predicate is false when no toolchain is installed`() { + val context = ApplicationProvider.getApplicationContext() + assertThat(context.isIdeSetupComplete()).isFalse() + } + + /** + * The cold-start case, and the reason this predicate reads the filesystem. + * + * `IJdkDistributionProvider.installedDistributions` is empty until the loader coroutine + * `IDEApplication` starts on `Dispatchers.Default` has run, and an Activity's `onCreate` beats it + * to the main thread. Asking the provider therefore answered "not set up" on a device that was, + * and the link was discarded with a message telling the user to finish a finished setup. Nothing + * loads the provider in this test either -- which is precisely the state under test. + */ + @Test + fun `the setup predicate is true from disk alone, with no distributions loaded`() { + val context = ApplicationProvider.getApplicationContext() + val prefix = tempFolder.newFolder("prefix") + File(prefix, "lib/jvm/jdk-17").mkdirs() + val previousPrefix = Environment.PREFIX + val previousHome = Environment.ANDROID_HOME + Environment.PREFIX = prefix + Environment.ANDROID_HOME = tempFolder.newFolder("android-sdk") + mockkObject(PermissionsHelper) + every { PermissionsHelper.areAllPermissionsGranted(any()) } returns true + try { + assertThat(IJdkDistributionProvider.getInstance().installedDistributions).isEmpty() + assertThat(context.isIdeSetupComplete()).isTrue() + } finally { + unmockkAll() + Environment.PREFIX = previousPrefix + Environment.ANDROID_HOME = previousHome + } + } + + // The second cold-start race, same shape as the provider one (ADFA-5067 review): + // Environment.init() runs on the same unawaited loader coroutine, so PREFIX and ANDROID_HOME + // are still null when a cold-start main thread asks. The gate must fall back to the constant + // defaults init() itself would assign, not wait -- and not NPE on ANDROID_HOME. + @Test + fun `the toolchain paths do not wait for Environment-init`() { + val previousPrefix = Environment.PREFIX + val previousHome = Environment.ANDROID_HOME + Environment.PREFIX = null + Environment.ANDROID_HOME = null + try { + assertThat(jdkInstallPrefix().path).isEqualTo(Environment.DEFAULT_PREFIX) + assertThat(androidSdkHome().path).isEqualTo(Environment.DEFAULT_HOME + "/android-sdk") + } finally { + Environment.PREFIX = previousPrefix + Environment.ANDROID_HOME = previousHome + } + } + + /** + * The regression from the ADFA-5067 review: toolchain fully on disk, `Environment.PREFIX` never + * assigned -- a cold start that beat the loader coroutine to `Environment.init()`. The old + * `File(Environment.PREFIX, "lib/jvm")` read silently became the relative path `lib/jvm` and + * answered false, so the link was discarded on a fully set-up device. + * + * [jdkInstallPrefix] is stubbed to a temp dir because its real null-fallback + * (`Environment.DEFAULT_PREFIX`, i.e. `/data/data/...`) is not creatable on a test host; the + * fallback's own value is pinned by the test above. What this test pins is that the predicate + * consults [jdkInstallPrefix] rather than reading the unassigned field directly. + */ + @Test + fun `the setup predicate is true with the JDK on disk and PREFIX never assigned`() { + val context = ApplicationProvider.getApplicationContext() + val prefix = tempFolder.newFolder("prefix-cold-start") + File(prefix, "lib/jvm/jdk-17").mkdirs() + val previousPrefix = Environment.PREFIX + val previousHome = Environment.ANDROID_HOME + Environment.PREFIX = null + Environment.ANDROID_HOME = tempFolder.newFolder("android-sdk-cold-start") + mockkStatic("com.itsaky.androidide.activities.SetupStateKt") + every { jdkInstallPrefix() } returns prefix + mockkObject(PermissionsHelper) + every { PermissionsHelper.areAllPermissionsGranted(any()) } returns true + try { + assertThat(context.isIdeSetupComplete()).isTrue() + } finally { + unmockkAll() + Environment.PREFIX = previousPrefix + Environment.ANDROID_HOME = previousHome + } + } + + // ...and an empty lib/jvm is not a JDK: a bootstrap that unpacked the directory but no + // distribution is an unfinished install, which the gate should still refuse. + @Test + fun `an empty lib-jvm directory does not count as installed`() { + val context = ApplicationProvider.getApplicationContext() + val prefix = tempFolder.newFolder("prefix-empty") + File(prefix, "lib/jvm").mkdirs() + val previousPrefix = Environment.PREFIX + val previousHome = Environment.ANDROID_HOME + Environment.PREFIX = prefix + Environment.ANDROID_HOME = tempFolder.newFolder("android-sdk-empty") + mockkObject(PermissionsHelper) + every { PermissionsHelper.areAllPermissionsGranted(any()) } returns true + try { + assertThat(context.isIdeSetupComplete()).isFalse() + } finally { + unmockkAll() + Environment.PREFIX = previousPrefix + Environment.ANDROID_HOME = previousHome + } + } +} diff --git a/app/src/test/java/com/itsaky/androidide/activities/DeepLinkTargetsNotExportedTest.kt b/app/src/test/java/com/itsaky/androidide/activities/DeepLinkTargetsNotExportedTest.kt new file mode 100644 index 0000000000..52f35a070b --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/DeepLinkTargetsNotExportedTest.kt @@ -0,0 +1,63 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.activities + +import android.content.ComponentName +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.activities.editor.EditorActivityKt +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * DeepLinkActivity validates a deep-link URI and then hands the parsed request on to one of these + * two activities in an Intent extra. Neither may be reachable from outside the app: an exported + * target can be sent that same extra directly, skipping the validation, to force an arbitrary + * project open and navigate to an arbitrary file inside it. + * + * MainActivity was exported with no intent-filter of its own, so nothing legitimate needed it + * (SplashActivity holds MAIN/LAUNCHER) and the extra was forgeable (ADFA-5067 review). + */ +@RunWith(RobolectricTestRunner::class) +class DeepLinkTargetsNotExportedTest { + @Test + fun `the activities DeepLinkActivity hands a parsed request to are not exported`() { + val context = ApplicationProvider.getApplicationContext() + + for (target in listOf(MainActivity::class.java, EditorActivityKt::class.java)) { + val info = + context.packageManager.getActivityInfo(ComponentName(context, target), 0) + assertThat(info.exported).isFalse() + } + } + + // The activity that does the validating must stay exported -- it is the App Link entry point, and + // a non-exported one would make every deep link a no-op rather than a security improvement. + @Test + fun `DeepLinkActivity itself is exported`() { + val context = ApplicationProvider.getApplicationContext() + val info = + context.packageManager.getActivityInfo( + ComponentName(context, DeepLinkActivity::class.java), + 0, + ) + assertThat(info.exported).isTrue() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/SameProjectDeepLinkMidSyncTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/SameProjectDeepLinkMidSyncTest.kt new file mode 100644 index 0000000000..2c56402c27 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/SameProjectDeepLinkMidSyncTest.kt @@ -0,0 +1,159 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.activities.editor + +import android.content.Intent +import androidx.core.content.IntentCompat +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.app.BaseApplication +import com.itsaky.androidide.databinding.ActivityEditorBinding +import com.itsaky.androidide.databinding.ContentEditorBinding +import com.itsaky.androidide.deeplink.PendingDeepLinkOpen +import com.itsaky.androidide.models.PendingFileRequest +import com.itsaky.androidide.projects.IProjectManager +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkObject +import io.mockk.unmockkAll +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.koin.core.context.GlobalContext +import org.koin.core.context.GlobalContext.startKoin +import org.koin.core.context.GlobalContext.stopKoin +import org.koin.dsl.module +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * A deep link into the project that is already open, arriving while that project is still syncing + * (`workspace == null`), must not be dropped: `switchToProject`'s same-project branch has to arm + * the request on the intent so `postProjectInit`'s deferred retry finds it once the sync completes + * (ADFA-5067 review). + * + * The failure mode being pinned is double: the new request used to die in a local variable, and + * because `onNewIntent`'s carry-forward guard had already re-armed the *previous*, still-unconsumed + * request onto the intent, `postProjectInit` then navigated to that stale target -- the link + * appeared to work, at the wrong file. + * + * Mirrors [RestorePluginTabsThreadTest]'s approach of exercising a real, private production method + * on an activity that has been built but not created -- creating the full editor activity is far + * beyond what a JVM test can do, and everything this path touches (the intent, the project + * manager, the binding null-check) can be provided directly. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = SameProjectDeepLinkMidSyncTest.TestApp::class) +class SameProjectDeepLinkMidSyncTest { + open class TestApp : BaseApplication() + + // switchToProject reads the Koin-provided PendingDeepLinkOpen on three of its four branches, and + // this activity is built but never created, so `isDestroyed` makes contentOrNull null and the + // binding-torn-down branch is the one taken. Without a Koin context that branch threw + // "KoinApplication has not been started" before the test could assert anything -- this test has + // been failing on the branch for exactly that reason, independently of what it is meant to check. + private var startedKoin = false + + // switchToProject reads the Koin-provided PendingDeepLinkOpen on three of its four branches, so a + // Koin context has to exist. It may already: run in the full :app suite rather than alone, another + // test's application has started one, and startKoin would throw + // KoinApplicationAlreadyStartedException. Join the existing context in that case and leave it + // running for whoever owns it; only tear down a context this test started itself. + @Before + fun setUp() { + val binding = module { single { PendingDeepLinkOpen() } } + val existing = GlobalContext.getOrNull() + if (existing == null) { + startedKoin = true + startKoin { modules(binding) } + } else { + existing.loadModules(listOf(binding)) + } + } + + @After + fun tearDown() { + if (startedKoin) { + stopKoin() + startedKoin = false + } + unmockkAll() + } + + @Test + fun `a mid-sync same-project request stays armed and supersedes the carried-forward one`() { + val projectPath = "/projects/MyApp" + mockkObject(IProjectManager.Companion) + val projectManager = mockk(relaxed = true) + every { projectManager.projectDirPath } returns projectPath + // The state under test: the project has started opening but the Gradle sync has not + // completed, so the workspace is not available yet. + every { projectManager.workspace } returns null + every { IProjectManager.getInstance() } returns projectManager + + val activity = + Robolectric + .buildActivity(EditorHandlerActivity::class.java, Intent()) + .get() + // Non-null binding AND a non-null `content` on it, so switchToProject takes its same-project + // branch instead of the binding-torn-down handoff; nothing on the branch under test touches the + // views themselves. + // + // `content` is set by reflection because view binding generates it as a public Java FIELD, and + // mockk stubs methods, not fields -- a relaxed mock therefore leaves it null, `contentOrNull` + // (which returns `_binding!!.content`) reads null, and the test silently exercised the + // binding-torn-down branch instead of the one it names. That is why it has been failing. + val activityBinding = mockk(relaxed = true) + ActivityEditorBinding::class.java + .getDeclaredField("content") + .apply { isAccessible = true } + .set(activityBinding, mockk(relaxed = true)) + activity._binding = activityBinding + + // What onNewIntent's carry-forward guard re-arms from the previous intent: the earlier, + // still-unconsumed request. Without the fix, postProjectInit would find (and navigate to) + // this one. + val staleRequest = PendingFileRequest("file/A.kt", null, null) + activity.intent.putExtra(PendingFileRequest.EXTRA_KEY, staleRequest) + + val newRequest = PendingFileRequest("file/B.kt", "10", "2") + val switchToProject = + EditorHandlerActivity::class.java.getDeclaredMethod( + "switchToProject", + String::class.java, + PendingFileRequest::class.java, + String::class.java, + // bookkeepingAlreadyRecorded: false here, since this exercises the deep-link path, where + // MainActivity.openProject never ran and the open has not been recorded yet. + Boolean::class.javaPrimitiveType, + ) + switchToProject.isAccessible = true + switchToProject.invoke(activity, projectPath, newRequest, projectPath, false) + + // postProjectInit's deferred retry reads exactly this extra once the sync completes: it + // must find the new request -- not nothing, and not the stale carried-forward one. + val armed = + IntentCompat.getParcelableExtra( + activity.intent, + PendingFileRequest.EXTRA_KEY, + PendingFileRequest::class.java, + ) + assertThat(armed).isEqualTo(newRequest) + } +} diff --git a/app/src/test/java/com/itsaky/androidide/analytics/DeepLinkMetricTest.kt b/app/src/test/java/com/itsaky/androidide/analytics/DeepLinkMetricTest.kt new file mode 100644 index 0000000000..25bab1adcc --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/analytics/DeepLinkMetricTest.kt @@ -0,0 +1,90 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.analytics + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.models.DeepLinkRequest +import com.itsaky.androidide.models.PendingFileRequest +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class DeepLinkMetricTest { + private fun request( + line: String? = null, + column: String? = null, + file: String? = null, + ) = DeepLinkRequest( + projectName = "MyApp", + fileRequest = file?.let { PendingFileRequest(filePath = it, lineRaw = line, columnRaw = column) }, + ) + + @Test + fun `depth reflects how far down the optional segments a link reached`() { + assertThat(request().depth()).isEqualTo(DeepLinkDepth.PROJECT) + assertThat(request(file = "a.kt").depth()).isEqualTo(DeepLinkDepth.FILE) + assertThat(request(file = "a.kt", line = "42").depth()).isEqualTo(DeepLinkDepth.LINE) + assertThat(request(file = "a.kt", line = "42", column = "7").depth()).isEqualTo(DeepLinkDepth.COLUMN) + } + + // A malformed line/column is still *present* in the URL, and the code reports it to the user + // rather than ignoring it -- so the metric has to count it as having reached that depth, or the + // "links people send are more specific than they resolve" signal quietly loses its worst cases. + @Test + fun `a non-numeric line still counts as line depth`() { + assertThat(request(file = "a.kt", line = "notanumber").depth()).isEqualTo(DeepLinkDepth.LINE) + } + + // The whole point of hashing: a project name is the user's content and must not leave the device. + @Test + fun `the bundle carries a project hash, never the project name`() { + val bundle = DeepLinkMetric(DeepLinkDepth.PROJECT, DeepLinkOutcome.RECEIVED, "MySecretProject").asBundle() + + assertThat(bundle.getLong("project_hash")).isEqualTo("MySecretProject".hashCode().toLong()) + for (key in bundle.keySet()) { + assertThat(bundle.get(key).toString()).doesNotContain("MySecretProject") + } + } + + @Test + fun `depth and outcome are recorded as lowercase names`() { + val bundle = DeepLinkMetric(DeepLinkDepth.COLUMN, DeepLinkOutcome.PROJECT_UNVERIFIABLE).asBundle() + + assertThat(bundle.getString("depth")).isEqualTo("column") + assertThat(bundle.getString("outcome")).isEqualTo("project_unverifiable") + } + + // Omitted rather than logged as 0/"": a metric with no project (an unparseable link, or the + // pre-parse setup gate) must not look like one for a project that hashes to zero. + @Test + fun `no project means no project_hash key at all`() { + val bundle = DeepLinkMetric(DeepLinkDepth.UNKNOWN, DeepLinkOutcome.INVALID_LINK).asBundle() + + assertThat(bundle.containsKey("project_hash")).isFalse() + } + + // One event name across every outcome, so the funnel is a single event filtered by `outcome` + // rather than a set of separate events that have to be summed to spot a drop-off. + @Test + fun `every outcome uses the one event name`() { + for (outcome in DeepLinkOutcome.entries) { + assertThat(DeepLinkMetric(DeepLinkDepth.PROJECT, outcome).eventName).isEqualTo(DeepLinkMetric.EVENT_NAME) + } + } +} diff --git a/app/src/test/java/com/itsaky/androidide/deeplink/ConsumedRequestsTest.kt b/app/src/test/java/com/itsaky/androidide/deeplink/ConsumedRequestsTest.kt new file mode 100644 index 0000000000..295e124f87 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/deeplink/ConsumedRequestsTest.kt @@ -0,0 +1,195 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.deeplink + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.models.DeepLinkRequest +import org.junit.Test + +/** + * ADFA-5067: a redelivered Intent must not force its project open twice. + * + * The scenario that matters is the one a single stored request got wrong -- two links, then process + * death, where the Intent the system hands back is the task's *launch* Intent rather than the last + * one `setIntent` saw. + */ +class ConsumedRequestsTest { + private fun request(name: String) = DeepLinkRequest(projectName = name) + + @Test + fun `a consumed request is recognised`() { + val consumed = ConsumedRequests() + consumed.add(request("alpha")) + + assertThat(request("alpha") in consumed).isTrue() + assertThat(request("beta") in consumed).isFalse() + } + + // The single-slot bug: consuming B made A look unconsumed again, and A is what a post-process-death + // recreate is handed, so A's project reopened over whatever the user was doing. + @Test + fun `consuming a second request does not un-consume the first`() { + val consumed = ConsumedRequests() + consumed.add(request("alpha")) + consumed.add(request("beta")) + + assertThat(request("alpha") in consumed).isTrue() + assertThat(request("beta") in consumed).isTrue() + } + + @Test + fun `the set survives a save and restore`() { + val consumed = ConsumedRequests() + consumed.add(request("alpha")) + consumed.add(request("beta")) + + val restored = ConsumedRequests() + restored.restore(consumed.toSavedList()) + + assertThat(request("alpha") in restored).isTrue() + assertThat(request("beta") in restored).isTrue() + } + + @Test + fun `restoring nothing leaves an empty set, not a stale one`() { + val consumed = ConsumedRequests() + consumed.add(request("alpha")) + consumed.restore(null) + + assertThat(request("alpha") in consumed).isFalse() + } + + @Test + fun `a repeated request is remembered once`() { + val consumed = ConsumedRequests() + consumed.add(request("alpha")) + consumed.add(request("alpha")) + + assertThat(consumed.toSavedList()).containsExactly(request("alpha")) + } + + // A deliberate re-arm (the same navigation requested a second time) must not be mistaken for + // the already-consumed earlier request and silently skipped. + @Test + fun `removing a consumed request lets an equal one be acted on again`() { + val consumed = ConsumedRequests() + consumed.add(request("alpha")) + consumed.remove(request("alpha")) + + assertThat(request("alpha") in consumed).isFalse() + } + + @Test + fun `a null request is ignored`() { + val consumed = ConsumedRequests() + consumed.add(null) + + assertThat(consumed.toSavedList()).isEmpty() + } + + // The cap bounds the saved Bundle; what it must not do is forget the most recent requests. + @Test + fun `past the cap the launch entry is pinned and the second-oldest evicted`() { + val consumed = ConsumedRequests() + repeat(40) { consumed.add(request("project$it")) } + + assertThat(consumed.toSavedList()).hasSize(32) + // project0 is the first ever added, which is by construction the request on the task's launch + // Intent -- the one Android replays verbatim after process death, and the only one whose loss + // force-reopens a project over whatever the user was doing. It survives; eviction takes the + // second-oldest instead. This used to assert the opposite. + assertThat(request("project0") in consumed).isTrue() + assertThat(request("project8") in consumed).isFalse() + assertThat(request("project9") in consumed).isTrue() + assertThat(request("project39") in consumed).isTrue() + } + + @Test + fun `re-adding a request refreshes its position rather than leaving it oldest`() { + val consumed = ConsumedRequests() + repeat(32) { consumed.add(request("project$it")) } + // Touch the second-oldest; it must no longer be the eviction candidate. + consumed.add(request("project1")) + consumed.add(request("fresh")) + + assertThat(request("project1") in consumed).isTrue() + assertThat(request("project2") in consumed).isFalse() + } + + // The pin used to be positional -- the eviction loop simply skipped slot 0 -- while add() + // reordered every entry it saw. Re-tapping the same URL therefore slid the launch entry off + // slot 0 and handed its protection to an unrelated request, so a sender firing links in a loop + // (DeepLinkActivity is exported) could evict it and force its project open after process death. + // Each of these needs a SECOND entry present before the launch entry is re-added. With the set + // holding nothing else, remove-then-append puts the launch entry straight back on slot 0 and the + // old positional pin still covered it -- so a version of these tests without "other" passed + // against the unfixed code and pinned nothing. + @Test + fun `re-adding the launch entry does not surrender its pin`() { + val consumed = ConsumedRequests() + consumed.add(request("project0")) + consumed.add(request("other")) + // The user taps the launch link a second time -- nothing gates a non-reforwarded repeat. + // The positional pin moved project0 off slot 0 here, handing its protection to "other". + consumed.add(request("project0")) + repeat(40) { consumed.add(request("flood$it")) } + + assertThat(consumed.toSavedList()).hasSize(32) + assertThat(request("project0") in consumed).isTrue() + } + + @Test + fun `an interleaved remove and re-add still leaves the launch entry pinned`() { + val consumed = ConsumedRequests() + consumed.add(request("project0")) + consumed.add(request("other")) + // armPendingFileRequest's deliberate re-arm: forget it, then record it again. + consumed.remove(request("project0")) + consumed.add(request("project0")) + repeat(40) { consumed.add(request("flood$it")) } + + assertThat(request("project0") in consumed).isTrue() + } + + @Test + fun `the pin survives a save and restore`() { + val consumed = ConsumedRequests() + consumed.add(request("project0")) + consumed.add(request("other")) + consumed.add(request("project0")) + + val restored = ConsumedRequests() + restored.restore(consumed.toSavedList()) + repeat(40) { restored.add(request("flood$it")) } + + assertThat(request("project0") in restored).isTrue() + } + + // restore() used to addAll() unchecked, so an over-long list came back oversized and went + // straight into the next onSaveInstanceState -- the Bundle bound the cap exists to enforce. + @Test + fun `restore re-applies the cap`() { + val oversized = (0 until 40).map { request("project$it") } + + val restored = ConsumedRequests() + restored.restore(oversized) + + assertThat(restored.toSavedList()).hasSize(32) + assertThat(request("project0") in restored).isTrue() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt new file mode 100644 index 0000000000..af7ec4db8f --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt @@ -0,0 +1,323 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +import android.net.Uri +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class DeepLinkRequestTest { + private fun parse(url: String) = DeepLinkRequest.parse(Uri.parse(url)) + + @Test + fun `project only`() { + val request = parse("https://www.appdevforall.org/device/open/project/MyApp") + assertThat(request).isEqualTo(DeepLinkRequest(projectName = "MyApp")) + } + + @Test + fun `project and file`() { + val request = parse("https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = null, columnRaw = null), + ), + ) + } + + @Test + fun `project, file, and line`() { + val request = parse("https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/line/42") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = "42", columnRaw = null), + ), + ) + } + + @Test + fun `project, file, line, and column`() { + val request = + parse( + "https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/line/42/column/7", + ) + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = "42", columnRaw = "7"), + ), + ) + } + + @Test + fun `multi-segment file path is rejoined with slashes`() { + val request = + parse( + "https://www.appdevforall.org/device/open/project/MyApp/file/app/src/main/Main.kt/line/1", + ) + assertThat(request?.fileRequest?.filePath).isEqualTo("app/src/main/Main.kt") + assertThat(request?.fileRequest?.lineRaw).isEqualTo("1") + } + + @Test + fun `project name equal to a reserved keyword does not corrupt line parsing`() { + // Regression test: a project literally named "line" used to make the parser latch onto the + // project-name segment itself as the `line` keyword (the first occurrence in the whole path), + // discarding the real line/42 suffix that follows `file`. + val request = parse("https://www.appdevforall.org/device/open/project/line/file/Main.kt/line/42") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "line", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = "42", columnRaw = null), + ), + ) + } + + @Test + fun `project name equal to a reserved keyword with no line suffix yields no line`() { + val request = parse("https://www.appdevforall.org/device/open/project/line/file/Main.kt") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "line", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = null, columnRaw = null), + ), + ) + } + + @Test + fun `project name equal to the file keyword does not corrupt the file lookup`() { + val request = parse("https://www.appdevforall.org/device/open/project/file/file/Main.kt") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "file", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = null, columnRaw = null), + ), + ) + } + + @Test + fun `a file path segment literally named 'line' is preserved when a real line suffix follows`() { + // Regression test: line/column are now matched from the end of the path backward, not by the + // keyword's first occurrence -- so a directory genuinely named "line" earlier in the file path + // is kept as part of the filename as long as a real trailing line/{n} pair follows it. + val request = + parse("https://www.appdevforall.org/device/open/project/MyApp/file/line/Main.kt/line/42") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "line/Main.kt", lineRaw = "42", columnRaw = null), + ), + ) + } + + @Test + fun `a file path segment literally named 'column' is preserved when a real trailing pair follows`() { + val request = + parse( + "https://www.appdevforall.org/device/open/project/MyApp/file/column/Main.kt/line/1/column/7", + ) + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "column/Main.kt", lineRaw = "1", columnRaw = "7"), + ), + ) + } + + @Test + fun `a file path that is only 'line' plus one segment names no file, so there is no file request`() { + // The keyword ambiguity itself is still unresolved and still a known limitation: with nothing + // else in the path, `file/line/Main.kt` is structurally identical to a real line suffix, and + // this URL scheme has no delimiter to tell "a directory named line" from "the line keyword". + // What changed is what the parser does once it has peeled the pair off and found nothing left + // to open. It used to hand back a PendingFileRequest whose filePath was empty, which resolves + // against nothing and reached the user as `File "" was not found in the project.` A link that + // names no file is a plain project-open link, so the file request is dropped -- and the line + // value goes with it, having nothing left to apply to. + val request = parse("https://www.appdevforall.org/device/open/project/MyApp/file/line/Main.kt") + assertThat(request).isEqualTo(DeepLinkRequest(projectName = "MyApp", fileRequest = null)) + } + + @Test + fun `a numeric line with no file path is a plain project-open link`() { + // The shape the review actually reported: `file/line/5` peels cleanly, leaves an empty path, + // and must not become a file request for the empty string. + assertThat(parse("https://www.appdevforall.org/device/open/project/MyApp/file/line/5")) + .isEqualTo(DeepLinkRequest(projectName = "MyApp", fileRequest = null)) + } + + @Test + fun `an empty file path with both line and column is a plain project-open link`() { + assertThat(parse("https://www.appdevforall.org/device/open/project/MyApp/file/line/5/column/7")) + .isEqualTo(DeepLinkRequest(projectName = "MyApp", fileRequest = null)) + } + + @Test + fun `malformed line and column are carried through unparsed, not rejected`() { + val request = + parse( + "https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/line/abc/column/xyz", + ) + assertThat(request?.fileRequest?.lineRaw).isEqualTo("abc") + assertThat(request?.fileRequest?.columnRaw).isEqualTo("xyz") + } + + @Test + fun `missing project segment yields null`() { + assertThat(parse("https://www.appdevforall.org/device/open/MyApp")).isNull() + } + + @Test + fun `project segment with no name yields null`() { + assertThat(parse("https://www.appdevforall.org/device/open/project")).isNull() + assertThat(parse("https://www.appdevforall.org/device/open/project/")).isNull() + } + + @Test + fun `file keyword with no name yields no file request`() { + val request = parse("https://www.appdevforall.org/device/open/project/MyApp/file") + assertThat(request).isEqualTo(DeepLinkRequest(projectName = "MyApp", fileRequest = null)) + } + + @Test + fun `an empty path segment yields null`() { + // Uri.pathSegments drops empty segments, so ".../project//file/Main.kt" used to parse as a + // project literally named "file" -- surfacing as `No project named "file" was found`, or + // silently opening a real project of that name, instead of an honest invalid-link error. + assertThat(parse("https://www.appdevforall.org/device/open/project//file/Main.kt")).isNull() + assertThat(parse("https://www.appdevforall.org/device/open/project/MyApp//file/Main.kt")).isNull() + } + + @Test + fun `null uri yields null`() { + assertThat(DeepLinkRequest.parse(null)).isNull() + } + + @Test + fun `wrong scheme yields null`() { + // DeepLinkActivity is exported (required for App Links), so its intent-filter's data scoping + // only constrains implicit intent matching -- an explicit intent from another app can carry + // any Uri. This must be rejected here regardless of how the intent arrived. + assertThat(parse("http://www.appdevforall.org/device/open/project/MyApp")).isNull() + } + + @Test + fun `wrong host yields null`() { + assertThat(parse("https://evil.example/device/open/project/MyApp")).isNull() + } + + @Test + fun `apex host without the www subdomain also matches`() { + // Both hosts serve an identical, verified assetlinks.json - see AndroidManifest.xml's matching + // pair of elements on DeepLinkActivity's intent-filter. + val request = parse("https://appdevforall.org/device/open/project/MyApp") + assertThat(request).isEqualTo(DeepLinkRequest(projectName = "MyApp")) + } + + @Test + fun `wrong path prefix yields null`() { + assertThat(parse("https://www.appdevforall.org/some/other/path/project/MyApp")).isNull() + } + + @Test + fun `non-canonical scheme and host case still matches`() { + // Scheme and host are case-insensitive per RFC 3986 -- an explicit intent from another app + // (see the "wrong scheme" test's rationale) could carry either in non-canonical case, and a + // semantically valid link must not be rejected over that alone. + val request = parse("HTTPS://WWW.APPDEVFORALL.ORG/device/open/project/MyApp") + assertThat(request).isEqualTo(DeepLinkRequest(projectName = "MyApp")) + } + + @Test + fun `a bare trailing 'column' keyword with no value is reported as invalid, not swallowed into the path`() { + // Regression test: `.../column` with nothing after it can never match the keyword-at- + // (size-2) pair check (there's no slot left for a value), so it used to silently fold into + // the file path with no error at all -- unlike the equivalent dangling-line-before-column + // case below, which was already reported. + val request = parse("https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/column") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = null, columnRaw = ""), + ), + ) + } + + @Test + fun `a bare 'line' keyword immediately before a 'column' pair is reported as invalid, not swallowed into the path`() { + // Regression test: `.../line/column/7` has no numeric value for "line" -- unlike the + // swallowed-into-filename ambiguity documented above, "line" here sits directly in front of a + // recognized "column" pair, so it must surface as an invalid line rather than silently + // becoming part of the file path with no line requested and no error. + val request = + parse("https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/line/column/7") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = "", columnRaw = "7"), + ), + ) + } + + @Test + fun `a real line pair followed by a bare trailing 'column' is still parsed, not swallowed whole`() { + // Regression test: a bare trailing "column" used to be checked independently against the + // original, un-trimmed end -- missing it, then leaving the real "line/5" pair unexamined and + // swallowed whole into the file path ("Main.kt/line/5") instead of peeling "column" off first + // and re-checking what's left for the line pair it exposes. + val request = + parse("https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/line/5/column") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = "5", columnRaw = ""), + ), + ) + } + + @Test + fun `a bare trailing 'line' keyword with no value is reported as invalid, not swallowed into the path`() { + // Regression test: symmetric to the bare-trailing-"column" case above, which was already + // caught -- a bare trailing "line" used to silently fold into the file path with no line + // number and no error at all. + val request = parse("https://www.appdevforall.org/device/open/project/MyApp/file/Main.kt/line") + assertThat(request) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "Main.kt", lineRaw = "", columnRaw = null), + ), + ) + } +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt new file mode 100644 index 0000000000..02ce033d3b --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt @@ -0,0 +1,150 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import java.io.File +import java.text.Normalizer + +class ProjectValidationsTest { + @JvmField + @Rule + val tempFolder = TemporaryFolder() + + private fun makeValidProject( + parent: File, + name: String, + ): File { + val project = File(parent, name).apply { mkdirs() } + val appDir = File(project, "app").apply { mkdirs() } + File(appDir, "build.gradle.kts").writeText("// stub") + return project + } + + @Test + fun `resolves an existing project by name`() { + val root = tempFolder.newFolder("projects") + val project = makeValidProject(root, "MyApp") + + assertThat(findValidProjectByName(root, "MyApp")?.canonicalFile).isEqualTo(project.canonicalFile) + } + + @Test + fun `unknown project name yields null`() { + val root = tempFolder.newFolder("projects") + assertThat(findValidProjectByName(root, "DoesNotExist")).isNull() + } + + @Test + fun `NFC-normalized name matches an NFD on-disk project directory`() { + // Regression test: a deep link URL is typically NFC-normalized by web tooling, but an + // imported project directory (e.g. a git clone authored on macOS, which decomposes + // accented filenames to NFD) may not codepoint-match it even though the two look identical. + val root = tempFolder.newFolder("projects") + val nfc = Normalizer.normalize("Café", Normalizer.Form.NFC) + val nfd = Normalizer.normalize("Café", Normalizer.Form.NFD) + assertThat(nfd).isNotEqualTo(nfc) // sanity check: the two forms really are distinct strings + val project = makeValidProject(root, nfd) + + assertThat(findValidProjectByName(root, nfc)?.canonicalFile).isEqualTo(project.canonicalFile) + } + + @Test + fun `dot-dot traversal outside projectsRoot is rejected`() { + // Regression test: a bare File(projectsRoot, name) join let `name` escape projectsRoot + // entirely (e.g. name = "../outside"). A real deep link supplies this as a decoded URL + // segment, so a project sitting just outside the configured projects root must never be + // resolvable via a crafted project name. + // + // This exact input ("../outside" contains a "/") is actually short-circuited by + // findValidProjectByName's own separate name.contains("/") guard, never reaching + // resolveWithinDirectory's traversal logic -- see the test below for the single-segment + // ".." case a real deep link's URL path segment can actually carry (Uri.pathSegments never + // contains a literal "/" within one segment). + val base = tempFolder.newFolder("base") + val root = File(base, "projects").apply { mkdirs() } + makeValidProject(base, "outside") + + assertThat(findValidProjectByName(root, "../outside")).isNull() + } + + @Test + fun `a single-segment 'dot-dot' name is rejected`() { + // The reachable counterpart to the test above: a deep link's project-name URL segment can + // never contain "/" (Uri.pathSegments splits on it), so name = ".." alone -- not "../x" -- + // is the actual traversal shape resolveWithinDirectory's lexical check must catch. + // + // base is made a *valid* project (not just a bare directory) so this test actually exercises + // that lexical check: with a bare directory, findValidProjectByName would return null either + // way -- via the traversal check working correctly, or via isValidProjectDirectory rejecting + // an escaped-but-unmarked base -- so the assertion couldn't tell a traversal regression apart + // from a passing test. + val base = makeValidProject(tempFolder.root, "base") + val root = File(base, "projects").apply { mkdirs() } + + assertThat(findValidProjectByName(root, "..")).isNull() + } + + // The tri-state lookup findValidProjectByName now delegates to (ADFA-5067 review). These pin the + // two outcomes a unit test can actually produce; Unverifiable needs a real EACCES/EIO from the + // filesystem mid-call, which is not reliably provokable in a JVM test -- see the class docs. + @Test + fun `lookup reports Found for an existing project`() { + val root = tempFolder.newFolder("projects") + val project = makeValidProject(root, "MyApp") + + val lookup = lookupValidProjectByName(root, "MyApp") + + assertThat(lookup).isInstanceOf(ProjectNameLookup.Found::class.java) + assertThat((lookup as ProjectNameLookup.Found).dir.canonicalFile).isEqualTo(project.canonicalFile) + } + + @Test + fun `lookup reports NotFound for a name with no project`() { + val root = tempFolder.newFolder("projects") + + assertThat(lookupValidProjectByName(root, "DoesNotExist")).isEqualTo(ProjectNameLookup.NotFound) + } + + // A traversal attempt is a definite "not this project", not an unknown -- callers are allowed to + // remember a NotFound, and must not be handed something they have to treat as maybe-transient. + @Test + fun `lookup reports NotFound for a traversal attempt`() { + val root = tempFolder.newFolder("projects") + + assertThat(lookupValidProjectByName(root, "../etc")).isEqualTo(ProjectNameLookup.NotFound) + assertThat(lookupValidProjectByName(root, ".")).isEqualTo(ProjectNameLookup.NotFound) + assertThat(lookupValidProjectByName(root, "")).isEqualTo(ProjectNameLookup.NotFound) + } + + // findValidProjectByName is now a thin reduction of lookupValidProjectByName; this pins that the + // refactor did not change what the many existing callers see. + @Test + fun `findValidProjectByName still agrees with the lookup it delegates to`() { + val root = tempFolder.newFolder("projects") + makeValidProject(root, "MyApp") + + for (name in listOf("MyApp", "DoesNotExist", "../etc", ".", "")) { + val expected = (lookupValidProjectByName(root, name) as? ProjectNameLookup.Found)?.dir + assertThat(findValidProjectByName(root, name)).isEqualTo(expected) + } + } +} diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorFeatures.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorFeatures.kt index 5a0d016c48..d941fd5ac8 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorFeatures.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorFeatures.kt @@ -88,6 +88,17 @@ class EditorFeatures( } ?: Position.NONE override fun validateRange(range: Range) { + // The shared sentinels are never clamped. Range.NONE is a process-wide @JvmField whose two ends + // are the single Position.NONE instance, and the assignments below write line/column straight + // onto the caller's objects -- so clamping one here rewrites Position.NONE from (-1, -1) to real + // in-document coordinates for the rest of the process. Position has structural equals, so every + // later "nothing found" check comparing against Range.NONE / Position.NONE (GoToDefinition, + // FindUsages, OrganizeImports, CodeFormatProvider) would silently stop matching. Callers reach + // here with them legitimately: this very class hands them out as `?: Range.NONE` and + // `?: Position.NONE` defaults. + if (range === Range.NONE || range.start === Position.NONE || range.end === Position.NONE) { + return + } withEditor { val start = range.start val end = range.end @@ -140,7 +151,7 @@ class EditorFeatures( ): Boolean = withEditor { val columnCount = text.getColumnCount(line) - return@withEditor column >= 0 && (column < columnCount || allowColumnEqual && column == columnCount) + return@withEditor column >= 0 && (column < columnCount || (allowColumnEqual && column == columnCount)) } ?: false override fun append(text: CharSequence?): Int = diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 581cffcb83..9d24ea5d36 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -138,6 +138,15 @@ Do you want to open the last opened project? The project was:\n%s Close this project Last opened project doesn\'t exist. + This link could not be opened. + Finish setting up Code on the Go, then open this link again. + No project named \"%s\" was found. + File \"%s\" was not found in the project. + \"%s\" is not a valid line number. + \"%s\" is not a valid column number. + (no value given) + Could not scan projects for this link. + A project close is already in progress. Try again in a moment. Create new project Open a saved project Delete a saved project diff --git a/shared/src/main/java/com/itsaky/androidide/models/Locations.kt b/shared/src/main/java/com/itsaky/androidide/models/Locations.kt index dd1a1858a0..5d12d32f05 100644 --- a/shared/src/main/java/com/itsaky/androidide/models/Locations.kt +++ b/shared/src/main/java/com/itsaky/androidide/models/Locations.kt @@ -20,247 +20,273 @@ package com.itsaky.androidide.models import com.google.gson.annotations.SerializedName import java.nio.file.Path -data class Location(var file: Path, var range: Range) +data class Location( + var file: Path, + var range: Range, +) + +data class Position + @JvmOverloads + constructor( + @SerializedName("line") var line: Int, + @SerializedName("column") var column: Int, + @SerializedName("index") var index: Int = -1, + ) : Comparable { + fun requireIndex(): Int { + if (index == -1) { + throw IllegalArgumentException("No index provided") + } + return index + } -data class Position @JvmOverloads constructor( - @SerializedName("line") var line: Int, - @SerializedName("column") var column: Int, - @SerializedName("index") var index: Int = -1 -) : Comparable { + /** + * Makes the indices 0 if they are negative. + * + * A no-op on [NONE]. That is a process-wide `@JvmField` singleton whose whole meaning is + * (-1, -1), handed out freely as a "nothing here" default -- zeroing it in place would redefine + * the sentinel for every structural comparison in the process from then on. + */ + fun zeroIfNegative() { + if (this === NONE) { + return + } - fun requireIndex(): Int { - if (index == -1) { - throw IllegalArgumentException("No index provided") - } - return index - } + if (line < 0) { + line = 0 + } - /** Makes the indices 0 if they are negative. */ - fun zeroIfNegative() { - if (line < 0) { - line = 0 + if (column < 0) { + column = 0 + } } - if (column < 0) { - column = 0 + companion object { + @JvmField + val NONE = Position(-1, -1) } - } - - companion object { - @JvmField - val NONE = Position(-1, -1) - } + override fun compareTo(other: Position): Int { + val byLine = + when { + line < other.line -> -1 + line > other.line -> 1 + else -> 0 + } - override fun compareTo(other: Position): Int { + if (byLine != 0) { + return byLine + } - val byLine = - when { - line < other.line -> -1 - line > other.line -> 1 + return when { + column < other.column -> -1 + column > other.column -> 1 else -> 0 } - - if (byLine != 0) { - return byLine } - return when { - column < other.column -> -1 - column > other.column -> 1 - else -> 0 - } - } + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is Position) return false - override fun equals(other: Any?): Boolean { - if (this === other) return true - if (other !is Position) return false + if (line != other.line) return false + if (column != other.column) return false - if (line != other.line) return false - if (column != other.column) return false - - return true - } + return true + } - override fun hashCode(): Int { - var result = line - result = 31 * result + column - return result + override fun hashCode(): Int { + var result = line + result = 31 * result + column + return result + } } -} open class Range -@JvmOverloads -constructor( - @SerializedName("start") var start: Position = Position(0, 0), - @SerializedName("end") var end: Position = Position(0, 0) -) : Comparable { - - operator fun component1() = start - operator fun component2() = end + @JvmOverloads + constructor( + @SerializedName("start") var start: Position = Position(0, 0), + @SerializedName("end") var end: Position = Position(0, 0), + ) : Comparable { + operator fun component1() = start + + operator fun component2() = end + + constructor(src: Range) : this( + Position(src.start.line, src.start.column), + Position(src.end.line, src.end.column), + ) + + companion object { + @JvmField + val NONE = Range(Position.NONE, Position.NONE) + + @JvmStatic + fun pointRange( + line: Int, + column: Int, + ): Range = pointRange(Position(line, column)) + + @JvmStatic + fun pointRange(position: Position): Range { + // Two copies, not the same instance twice. Position's fields are `var` and + // EditorFeatures.validateRange clamps start and end in place, so aliasing one object as + // both ends makes each clamp move the other end too. For a POINT range that is currently + // harmless -- the two ends hold equal values and the column clamp depends only on the + // already-clamped line, so both writes compute the same result. This copies anyway + // because that is a property of today's clamp, not of the type: any future validate that + // treats start and end asymmetrically (ordering them, or clamping end against start) + // would silently corrupt every point range. It also keeps the caller's Position out of + // the returned Range, which callers do mutate. + return Range(position.copy(), position.copy()) + } + } - constructor(src: Range) : this( - Position(src.start.line, src.start.column), - Position(src.end.line, src.end.column) - ) + /** + * Validate the start and end positions. + * @see Position.zeroIfNegative() + */ + fun validate() { + // NONE's two ends ARE the single Position.NONE instance, so validating the sentinel would + // redefine it process-wide. zeroIfNegative guards itself for the same reason; this guard is + // stated separately because this is the call the editor pipeline actually makes. + if (this === NONE) { + return + } + start.zeroIfNegative() + end.zeroIfNegative() + } - companion object { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is Range) return false - @JvmField - val NONE = Range(Position.NONE, Position.NONE) + if (start != other.start) return false + if (end != other.end) return false - @JvmStatic - fun pointRange(line: Int, column: Int): Range { - return pointRange(Position(line, column)) + return true } - @JvmStatic - fun pointRange(position: Position): Range { - return Range(position, position) + override fun hashCode(): Int { + var result = start.hashCode() + result = 31 * result + end.hashCode() + return result } - } - - /** - * Validate the start and end positions. - * @see Position.zeroIfNegative() - */ - fun validate() { - start.zeroIfNegative() - end.zeroIfNegative() - } - override fun equals(other: Any?): Boolean { - if (this === other) return true - if (other !is Range) return false + override fun compareTo(other: Range): Int = start.compareTo(other.start) - if (start != other.start) return false - if (end != other.end) return false + fun compareByEnd(other: Range): Int = end.compareTo(other.end) - return true - } + fun contains(position: Position): Boolean = contains(position.line, position.column) - override fun hashCode(): Int { - var result = start.hashCode() - result = 31 * result + end.hashCode() - return result - } + fun contains( + line: Int, + column: Int, + ): Boolean { + if (line < start.line || line > end.line) { + return false + } - override fun compareTo(other: Range): Int = start.compareTo(other.start) + if (start.line == end.line) { + return column >= start.column && column <= end.column + } - fun compareByEnd(other: Range): Int = end.compareTo(other.end) + if (line == start.line) return column >= start.column + if (line == end.line) return column <= end.column - fun contains(position: Position): Boolean = contains(position.line, position.column) - fun contains(line: Int, column: Int): Boolean { - if (line < start.line || line > end.line) { return false } - if (start.line == end.line) { - return column >= start.column && column <= end.column - } - - if (line == start.line) return column >= start.column - if (line == end.line) return column <= end.column - - return false - } - - /** - * Check if this range is before the given position or not. - */ - fun precedesPosition(line: Int, column: Int): Boolean { - if (end.line < line) { - return true - } - - return end.line == line && end.column < column - } + /** + * Check if this range is before the given position or not. + */ + fun precedesPosition( + line: Int, + column: Int, + ): Boolean { + if (end.line < line) { + return true + } - /** - * Check if this range is after the given position or not. - */ - fun succeedsPosition(line: Int, column: Int): Boolean { - if (start.line > line) { - return true + return end.line == line && end.column < column } - return start.line == line && start.column > column - } - - /** - * Check if this range contains the given position or not. Return an integer result indicating - * where the index of the range containing this position might be. - * - * The return value will be used in a binary search. - */ - fun containsForBinarySearch(position: Position): Int { - - // The position might appear before this range - if (position.line < start.line) { - return -1 - } + /** + * Check if this range is after the given position or not. + */ + fun succeedsPosition( + line: Int, + column: Int, + ): Boolean { + if (start.line > line) { + return true + } - // The position might appear after this range - if (position.line > end.line) { - return 1 + return start.line == line && start.column > column } - // If start and end lines are same, compare by column indexes - if (start.line == end.line) { - - if (position.column < start.column) { + /** + * Check if this range contains the given position or not. Return an integer result indicating + * where the index of the range containing this position might be. + * + * The return value will be used in a binary search. + */ + fun containsForBinarySearch(position: Position): Int { + // The position might appear before this range + if (position.line < start.line) { return -1 } - if (position.column > end.column) { + // The position might appear after this range + if (position.line > end.line) { return 1 } - } - - // This range definitely contains the position. - return 0 - } - fun containsLine(line: Int): Boolean { - return start.line <= line && end.line >= line - } + // If start and end lines are same, compare by column indexes + if (start.line == end.line) { + if (position.column < start.column) { + return -1 + } - fun containsColumn(column: Int): Boolean { - return start.column <= column && end.column >= column - } + if (position.column > end.column) { + return 1 + } + } - fun containsRange(other: Range): Boolean { - if (!containsLine(other.start.line) || !containsLine(other.end.line)) { - return false + // This range definitely contains the position. + return 0 } - return containsColumn(other.start.column) && containsColumn(other.end.column) - } + fun containsLine(line: Int): Boolean = start.line <= line && end.line >= line - fun isSmallerThan(other: Range): Boolean { - return other.isBiggerThan(this) - } + fun containsColumn(column: Int): Boolean = start.column <= column && end.column >= column - fun isBiggerThan(other: Range): Boolean { + fun containsRange(other: Range): Boolean { + if (!containsLine(other.start.line) || !containsLine(other.end.line)) { + return false + } - if (equals(other)) { - return false + return containsColumn(other.start.column) && containsColumn(other.end.column) } - if (start.line < other.start.line && end.line > other.end.line) { - return true - } + fun isSmallerThan(other: Range): Boolean = other.isBiggerThan(this) - if (start.line == other.start.line && end.line == other.end.line) { - if (start.column <= other.start.column && end.column >= other.end.column) { + fun isBiggerThan(other: Range): Boolean { + if (equals(other)) { + return false + } + + if (start.line < other.start.line && end.line > other.end.line) { return true } - } - return false - } + if (start.line == other.start.line && end.line == other.end.line) { + if (start.column <= other.start.column && end.column >= other.end.column) { + return true + } + } + + return false + } - override fun toString(): String { - return "Range(start=$start, end=$end)" + override fun toString(): String = "Range(start=$start, end=$end)" } -}