From b96e80380cf64ac36c4820fcd54d07328a36ef82 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Mon, 24 Aug 2026 15:34:29 -0500 Subject: [PATCH 1/6] feat: Add file-targeted IdeEditorService.saveFile(File) saveCurrentFile() saves whichever tab has focus and returns true as soon as the save is dispatched. A plugin editing an unfocused file therefore had to steal focus first, and openFile() only posts the tab switch - so the save read a stale tab index, persisted the user's other tab, and reported success. saveFile(file) removes focus from the causal chain: it resolves the editor by File, blocks until the write completes, and returns whether the bytes are on disk. A clean buffer counts as saved - CodeEditorView.save() reports "nothing to do" and "write failed" with the same false - and a completed write is verified by byte length to catch truncation. The permission check follows the file-targeted surface (requireWrite + ensureFileAccessible) rather than writableCurrentFile, which inspects the focused file. saveCurrentFile() stays as the "save what the user is looking at" primitive. Guards: the call rejects the main thread up front, since the save itself runs there and blocking from it would deadlock until the timeout. setFilesSaving now resets under NonCancellable so a timed-out save cannot leave the Save action disabled for the session. --- .../editor/EditorHandlerActivity.kt | 51 +++++++++++++++++-- .../androidide/app/EditorProviderImpl.kt | 11 ++++ plugin-api/api/plugin-api.api | 2 + .../plugins/services/IdeServices.kt | 20 +++++++- .../plugins/manager/core/PluginManager.kt | 2 + .../manager/services/IdeEditorServiceImpl.kt | 8 +++ 6 files changed, 90 insertions(+), 4 deletions(-) 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 ecd7ff984f..030aaabbd3 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 @@ -114,6 +114,7 @@ import java.util.WeakHashMap import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicInteger import java.util.function.Consumer /** @@ -136,6 +137,9 @@ open class EditorHandlerActivity : private val fileTimestamps = ConcurrentHashMap() + /** Number of saves in flight; see [performFileSave]. */ + private val activeSaveCount = AtomicInteger(0) + private val pluginTabIndices = mutableMapOf() private val tabIndexToPluginId = mutableMapOf() private var lastAppliedPluginFontScale = EditorPreferences.editorFontScale @@ -938,6 +942,35 @@ open class EditorHandlerActivity : } } + /** + * Saves the buffer for [file] regardless of which tab has focus, and reports whether the + * bytes reached disk. Returns `false` when no open editor holds [file]. + * + * A clean buffer counts as saved: [CodeEditorView.save] reports "nothing to do" and + * "write failed" with the same `false`, so the two are separated here. + * + * Resolution and the write share one main-thread continuation, so a tab close cannot shift + * the index out from under the save. + */ + suspend fun saveFileResult(file: File): Boolean = + try { + performFileSave { + withContext(Dispatchers.Main.immediate) { + val view = getEditorForFile(file) ?: return@withContext false + if (!view.isModified && file.exists()) return@withContext true + val index = findIndexOfEditorByFile(file) + index >= 0 && saveResultInternal(index, SaveResult()) && file.exists() + } + } + } catch (err: CancellationException) { + throw err + } catch (err: Exception) { + // ContentReadWrite.writeTo reports a failed write by throwing; that must not escape + // into the plugin coroutine awaiting this call. + log.error("Failed to save {}", file.name, err) + false + } + override fun onConfigurationChanged(newConfig: Configuration) { super.onConfigurationChanged(newConfig) @@ -1011,17 +1044,29 @@ open class EditorHandlerActivity : getEditorForFile(file)?.isModified == true } + /** + * Runs [action] with the "files are saving" flag raised. + * + * Counted rather than a plain boolean: a plugin-thread save can overlap a UI save, and the + * first one to finish must not clear the flag while the other is still writing. + */ private suspend inline fun performFileSave(crossinline action: suspend () -> T): T { - setFilesSaving(true) + if (activeSaveCount.incrementAndGet() == 1) { + setFilesSaving(true) + } try { return action() } finally { - setFilesSaving(false) + if (activeSaveCount.decrementAndGet() == 0) { + setFilesSaving(false) + } } } private suspend fun setFilesSaving(saving: Boolean) { - withContext(Dispatchers.Main.immediate) { + // NonCancellable: a cancelled save (e.g. a plugin-side timeout) must still clear the + // flag, or SaveFileAction stays disabled for the rest of the session. + withContext(NonCancellable + Dispatchers.Main.immediate) { editorViewModel.areFilesSaving = saving } } 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 4fc16ff0fc..24db5b4d56 100644 --- a/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt +++ b/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt @@ -254,6 +254,17 @@ class EditorProviderImpl( return true } + /** + * Saves [file]'s buffer whatever tab has focus, suspending until the bytes are on disk. + * + * Suspending rather than blocking is what makes this safe to call from the main thread: + * the write itself runs there, so a blocking bridge would deadlock against it. + */ + override suspend fun saveFile(file: File): Boolean { + val activity = activity() ?: return false + return activity.saveFileResult(file) + } + // --- Buffer edits ------------------------------------------------------- override fun insertTextAtCursor(text: String): Boolean = diff --git a/plugin-api/api/plugin-api.api b/plugin-api/api/plugin-api.api index af250a9c96..8fc09ea703 100644 --- a/plugin-api/api/plugin-api.api +++ b/plugin-api/api/plugin-api.api @@ -1368,6 +1368,7 @@ public abstract interface class com/itsaky/androidide/plugins/services/IdeEditor public abstract fun replaceRange (Ljava/io/File;Lcom/itsaky/androidide/plugins/services/SelectionRange;Ljava/lang/String;)Z public abstract fun replaceSelection (Ljava/lang/String;)Z public abstract fun saveCurrentFile ()Z + public abstract fun saveFile (Ljava/io/File;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; public abstract fun showInlineSuggestion (Ljava/lang/String;)V public abstract fun showPeerCursor (Ljava/io/File;IILjava/lang/String;Ljava/lang/String;I)Z } @@ -1378,6 +1379,7 @@ public final class com/itsaky/androidide/plugins/services/IdeEditorService$Defau public static fun dismissInlineSuggestion (Lcom/itsaky/androidide/plugins/services/IdeEditorService;)V public static fun hidePeerCursor (Lcom/itsaky/androidide/plugins/services/IdeEditorService;Ljava/io/File;Ljava/lang/String;)Z public static fun removeContentChangeListener (Lcom/itsaky/androidide/plugins/services/IdeEditorService;Lcom/itsaky/androidide/plugins/services/EditorContentChangeListener;)V + public static fun saveFile (Lcom/itsaky/androidide/plugins/services/IdeEditorService;Ljava/io/File;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; public static fun showInlineSuggestion (Lcom/itsaky/androidide/plugins/services/IdeEditorService;Ljava/lang/String;)V public static fun showPeerCursor (Lcom/itsaky/androidide/plugins/services/IdeEditorService;Ljava/io/File;IILjava/lang/String;Ljava/lang/String;I)Z } diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt index b0595c3f2b..8fd4731c23 100644 --- a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt @@ -172,10 +172,28 @@ interface IdeEditorService { /** * Schedules a save of the active editor tab. Runs asynchronously; a `true` return means * the save was dispatched, not that the buffer has been flushed to disk. Poll - * [isFileModified] on the current file to confirm completion. + * [isFileModified] on the current file to confirm completion. Prefer [saveFile] when you + * know which file you want persisted - this one follows focus, which the user controls. */ fun saveCurrentFile(): Boolean + /** + * Saves [file]'s open buffer to disk, whatever tab currently has focus, and suspends until + * the write completes - unlike [saveCurrentFile], which only reports that a save was + * dispatched for the focused tab. + * + * Safe to call from any dispatcher, the main one included: the write is marshalled to the + * editor thread and nothing blocks while it runs. Wrap the call in `withTimeout` if your + * plugin needs to bound how long it waits. + * + * Returns `true` when the buffer is on disk (including "was already clean"), `false` when + * the file has no open editor, the caller lacks FILESYSTEM_WRITE, the path is outside the + * plugin's allowed roots, or the write failed. + * + * Default-implemented (no-op) so adding it is a backward-compatible interface extension. + */ + suspend fun saveFile(file: File): Boolean = false + fun insertTextAtCursor(text: String): Boolean fun replaceSelection(text: String): Boolean diff --git a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt index 50d401dfd6..bcd2d84d66 100644 --- a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt +++ b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt @@ -190,6 +190,8 @@ class PluginManager private constructor( override fun saveCurrentFile(): Boolean = current()?.saveCurrentFile() ?: false + override suspend fun saveFile(file: File): Boolean = current()?.saveFile(file) ?: false + override fun insertTextAtCursor(text: String): Boolean = current()?.insertTextAtCursor(text) ?: false override fun replaceSelection(text: String): Boolean = current()?.replaceSelection(text) ?: false diff --git a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt index 388bc692e4..5fb8fd8e5b 100644 --- a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt +++ b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt @@ -98,6 +98,8 @@ class IdeEditorServiceImpl( fun saveCurrentFile(): Boolean = false + suspend fun saveFile(file: File): Boolean = false + fun insertTextAtCursor(text: String): Boolean = false fun replaceSelection(text: String): Boolean = false @@ -317,6 +319,12 @@ class IdeEditorServiceImpl( return editorProvider.saveCurrentFile() } + override suspend fun saveFile(file: File): Boolean { + requireWrite() + ensureFileAccessible(file) + return editorProvider.saveFile(file) + } + override fun insertTextAtCursor(text: String): Boolean { if (!writableCurrentFile()) return false return editorProvider.insertTextAtCursor(text) From a49b6a6760034162b38292a09dd496855b0b3cc4 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Wed, 26 Aug 2026 13:14:05 -0500 Subject: [PATCH 2/6] fix(editor): keep areFilesSaving raised across overlapping saves Move the count into the flag's main-thread section: off-main it decremented to zero and queued false, which landed after a main-thread save had set true inline. --- .../editor/EditorHandlerActivity.kt | 40 ++++++-- .../editor/OverlappingSaveFlagTest.kt | 98 +++++++++++++++++++ .../plugins/services/IdeServices.kt | 6 +- 3 files changed, 132 insertions(+), 12 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt 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 030aaabbd3..c04a45574b 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 @@ -29,6 +29,7 @@ import android.view.KeyEvent import android.view.View import android.view.ViewGroup.LayoutParams import android.widget.TextView +import androidx.annotation.VisibleForTesting import androidx.collection.MutableIntObjectMap import androidx.core.content.res.ResourcesCompat import androidx.core.view.GravityCompat @@ -137,7 +138,7 @@ open class EditorHandlerActivity : private val fileTimestamps = ConcurrentHashMap() - /** Number of saves in flight; see [performFileSave]. */ + /** Number of saves in flight. Main-thread confined; see [beginFileSave]. */ private val activeSaveCount = AtomicInteger(0) private val pluginTabIndices = mutableMapOf() @@ -1051,23 +1052,42 @@ open class EditorHandlerActivity : * first one to finish must not clear the flag while the other is still writing. */ private suspend inline fun performFileSave(crossinline action: suspend () -> T): T { - if (activeSaveCount.incrementAndGet() == 1) { - setFilesSaving(true) - } + beginFileSave() try { return action() } finally { - if (activeSaveCount.decrementAndGet() == 0) { - setFilesSaving(false) + endFileSave() + } + } + + /** + * Raises the saving flag for one save. + * + * The count moves in the same main-thread section as the flag, so the counter's ordering + * *is* the flag's ordering. Bumping the count off-main instead let a finished off-main + * save's queued `false` land after a main-thread save had already written `true` inline + * (`Main.immediate` skips the queue when it is already on main), leaving + * [EditorViewModel.areFilesSaving] false while that save was still writing. + * + * NonCancellable: a cancelled save (e.g. a plugin-side timeout) must still reach its + * matching [endFileSave], or SaveFileAction stays disabled for the rest of the session. + */ + @VisibleForTesting + internal suspend fun beginFileSave() { + withContext(NonCancellable + Dispatchers.Main.immediate) { + if (activeSaveCount.incrementAndGet() == 1) { + editorViewModel.areFilesSaving = true } } } - private suspend fun setFilesSaving(saving: Boolean) { - // NonCancellable: a cancelled save (e.g. a plugin-side timeout) must still clear the - // flag, or SaveFileAction stays disabled for the rest of the session. + /** Lowers the saving flag once the last in-flight save finishes. See [beginFileSave]. */ + @VisibleForTesting + internal suspend fun endFileSave() { withContext(NonCancellable + Dispatchers.Main.immediate) { - editorViewModel.areFilesSaving = saving + if (activeSaveCount.decrementAndGet() == 0) { + editorViewModel.areFilesSaving = false + } } } diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt new file mode 100644 index 0000000000..edf80915dd --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt @@ -0,0 +1,98 @@ +/* + * 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.os.Looper +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.app.BaseApplication +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.runBlocking +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import org.robolectric.shadows.ShadowLooper +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import kotlin.concurrent.thread + +/** + * Overlapping saves must never leave `areFilesSaving` false while a save is still writing - + * SaveFileAction re-enables itself off that flag, so a false gap lets the user fire a second + * save into an in-flight write. + * + * The interleaving is forced, not timed. Robolectric's paused main looper holds the off-main + * save's completion hop in the queue while the main-thread save runs its own hop inline + * (`Main.immediate` skips the queue when already on main) - exactly the ordering inversion at + * issue. + * + * - BUGGED: the count moves off-main, so the finishing off-main save decrements to 0 and + * queues `false`; the main-thread save then sees 0 -> 1 and writes `true` inline; the + * queued `false` runs last -> flag false mid-save -> test FAILS. + * - FIXED: count and flag move together on main, so the queued decrement lands as 2 -> 1 and + * writes nothing -> flag stays true -> test PASSES. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = OverlappingSaveFlagTest.TestApp::class) +class OverlappingSaveFlagTest { + open class TestApp : BaseApplication() + + @Test + fun givenOverlappingSaves_whenTheOffMainSaveCompletionDrains_thenTheSavingFlagStaysRaised() { + val activity = Robolectric.buildActivity(EditorHandlerActivity::class.java).get() + val mainLooper = shadowOf(Looper.getMainLooper()) + + // Save A begins on the main thread: the hop runs inline, raising the flag. + runBlocking { activity.beginFileSave() } + assertThat(activity.editorViewModel.areFilesSaving).isTrue() + + // Save A finishes off-main. Its completion hop is posted to the paused main looper and + // parks there; the worker stays blocked until we idle the looper. + val ended = CountDownLatch(1) + val worker = + thread { + runBlocking(Dispatchers.IO) { activity.endFileSave() } + ended.countDown() + } + awaitPostToMain(mainLooper) + + // Save B begins on the main thread while A's hop is still queued. + runBlocking { activity.beginFileSave() } + + // Drain A's queued completion. B is still writing, so the flag must stay raised. + mainLooper.idle() + assertThat(ended.await(10, TimeUnit.SECONDS)).isTrue() + worker.join() + assertThat(activity.editorViewModel.areFilesSaving).isTrue() + + // Only B finishing lowers it. + runBlocking { activity.endFileSave() } + assertThat(activity.editorViewModel.areFilesSaving).isFalse() + } + + /** Blocks until the worker's main-thread hop is sitting in the paused looper's queue. */ + private fun awaitPostToMain(mainLooper: ShadowLooper) { + val deadline = System.currentTimeMillis() + 10_000 + while (mainLooper.isIdle && System.currentTimeMillis() < deadline) { + Thread.sleep(1) + } + assertThat(mainLooper.isIdle).isFalse() + } +} diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt index 8fd4731c23..4f2678c4e4 100644 --- a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt @@ -187,8 +187,10 @@ interface IdeEditorService { * plugin needs to bound how long it waits. * * Returns `true` when the buffer is on disk (including "was already clean"), `false` when - * the file has no open editor, the caller lacks FILESYSTEM_WRITE, the path is outside the - * plugin's allowed roots, or the write failed. + * the file has no open editor or the write failed. Authorization failures throw + * [SecurityException] rather than returning `false`, as they do for every other write + * method here: the caller lacks FILESYSTEM_WRITE, or [file] is outside the plugin's + * allowed roots. * * Default-implemented (no-op) so adding it is a backward-compatible interface extension. */ From e558b10a5ad448eefae08667faa2e8064ac3cdeb Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Wed, 26 Aug 2026 13:58:02 -0500 Subject: [PATCH 3/6] test(editor): reap the overlapping-save worker on assertion failure A failed assertion skipped the looper drain, parking the non-daemon worker on a main-thread hop that never ran. Wrap the body in try/finally, drain and bound the join there, and mark the worker daemon. --- .../editor/OverlappingSaveFlagTest.kt | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt index edf80915dd..92b0905b0c 100644 --- a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt @@ -67,32 +67,43 @@ class OverlappingSaveFlagTest { // parks there; the worker stays blocked until we idle the looper. val ended = CountDownLatch(1) val worker = - thread { + thread(isDaemon = true) { runBlocking(Dispatchers.IO) { activity.endFileSave() } ended.countDown() } - awaitPostToMain(mainLooper) - // Save B begins on the main thread while A's hop is still queued. - runBlocking { activity.beginFileSave() } + try { + awaitPostToMain(mainLooper) - // Drain A's queued completion. B is still writing, so the flag must stay raised. - mainLooper.idle() - assertThat(ended.await(10, TimeUnit.SECONDS)).isTrue() - worker.join() - assertThat(activity.editorViewModel.areFilesSaving).isTrue() + // Save B begins on the main thread while A's hop is still queued. + runBlocking { activity.beginFileSave() } + + // Drain A's queued completion. B is still writing, so the flag must stay raised. + mainLooper.idle() + assertThat(ended.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue() + assertThat(activity.editorViewModel.areFilesSaving).isTrue() - // Only B finishing lowers it. - runBlocking { activity.endFileSave() } - assertThat(activity.editorViewModel.areFilesSaving).isFalse() + // Only B finishing lowers it. + runBlocking { activity.endFileSave() } + assertThat(activity.editorViewModel.areFilesSaving).isFalse() + } finally { + // A failed assertion above can leave the worker parked on a main-thread hop that + // never runs; drain the queue and reap it rather than leak a blocked thread. + mainLooper.idle() + worker.join(TIMEOUT_MS) + } } /** Blocks until the worker's main-thread hop is sitting in the paused looper's queue. */ private fun awaitPostToMain(mainLooper: ShadowLooper) { - val deadline = System.currentTimeMillis() + 10_000 + val deadline = System.currentTimeMillis() + TIMEOUT_MS while (mainLooper.isIdle && System.currentTimeMillis() < deadline) { Thread.sleep(1) } assertThat(mainLooper.isIdle).isFalse() } + + private companion object { + const val TIMEOUT_MS = 10_000L + } } From 89f736cceadcb2738193c3245e62a266ca7d2997 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Thu, 27 Aug 2026 10:20:32 -0500 Subject: [PATCH 4/6] fix(editor): address PR review on the file-targeted save F01/F13 The save counter moves into EditorViewModel, next to the areFilesSaving flag it guards. The ViewModel is retained across activity recreation while the activity is not, so a save still running under NonCancellable against a destroyed instance no longer clears the flag for a save the recreated instance had already started. Being main-confined by construction now, it is a plain Int rather than an AtomicInteger. F04 The count is floored at zero. Left negative by an unbalanced call, the "reached zero" test never matched again and SaveFileAction stayed disabled for the life of the ViewModel. F12 saveFileResult resolves the editor before raising the flag, so a save with nothing to do (file not open, or buffer already clean) no longer emits true/false for a no-op. F03 The file stat runs on Dispatchers.IO; debug builds install StrictMode's detectDiskReads on the main thread. F05/F07 The write takes the CodeEditorView, not a tab index. The file was being resolved twice through two different indexing schemes with nothing asserting they agreed, and the tab to unmark was resolved from an index captured before a suspending write - a tab closed during a large write stripped the asterisk from the wrong tab. F02 The SaveResult is consumed, as every other save path does: a plugin that saves a Gradle script gets the sync prompt, and one that saves a layout gets generateSources() so R fields for its new resources exist. F06/F08 EditorProviderImpl.saveFile bounds its wait, and its KDoc no longer claims the write runs on the main thread - CodeEditorView.save marshals it to its own write thread. Both KDocs now say plainly that a runBlocking bridge on the main thread deadlocks on the resumption hop. F09 PLUGIN_API_CHANGELOG.md records saveFile so an author can floor plugin.min_ide_version rather than hit the silent default false. F10 The overlapping-save test drains the looper before waiting on it. isIdle reports "nothing queued", not "the worker posted", so a leftover startup message satisfied it immediately. F11 Adds coverage: the plugin-facing SecurityException contract on IdeEditorServiceImpl.saveFile, the no-open-editor and detached-provider false paths, the flag not flapping for a no-op save, and the counter's overlap and floor semantics. Still uncovered - they need a live editor view: clean-buffer-returns-true, a write throw becoming false, and the CancellationException rethrow. Co-Authored-By: Claude Opus 5 (1M context) --- .../editor/EditorHandlerActivity.kt | 86 ++++++++----- .../androidide/app/EditorProviderImpl.kt | 23 +++- .../androidide/viewmodel/EditorViewModel.kt | 35 ++++++ .../editor/OverlappingSaveFlagTest.kt | 14 ++- .../activities/editor/SaveFileResultTest.kt | 114 ++++++++++++++++++ .../viewmodel/EditorViewModelSaveFlagTest.kt | 70 +++++++++++ docs/PLUGIN_API_CHANGELOG.md | 17 +++ .../plugins/services/IdeServices.kt | 9 +- plugin-manager/build.gradle.kts | 6 + .../IdeEditorServiceImplSaveFileTest.kt | 102 ++++++++++++++++ 10 files changed, 440 insertions(+), 36 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt create mode 100644 app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt create mode 100644 plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImplSaveFileTest.kt 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 c04a45574b..ed36efe6f6 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 @@ -115,7 +115,6 @@ import java.util.WeakHashMap import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicBoolean -import java.util.concurrent.atomic.AtomicInteger import java.util.function.Consumer /** @@ -138,9 +137,6 @@ open class EditorHandlerActivity : private val fileTimestamps = ConcurrentHashMap() - /** Number of saves in flight. Main-thread confined; see [beginFileSave]. */ - private val activeSaveCount = AtomicInteger(0) - private val pluginTabIndices = mutableMapOf() private val tabIndexToPluginId = mutableMapOf() private var lastAppliedPluginFontScale = EditorPreferences.editorFontScale @@ -950,27 +946,50 @@ open class EditorHandlerActivity : * A clean buffer counts as saved: [CodeEditorView.save] reports "nothing to do" and * "write failed" with the same `false`, so the two are separated here. * - * Resolution and the write share one main-thread continuation, so a tab close cannot shift - * the index out from under the save. + * The editor is handed to the write as a view, never as a tab index: an index is only + * valid until the first suspension, and resolving [file] twice through two different + * indexing schemes could save a different open buffer than the one that was checked. */ - suspend fun saveFileResult(file: File): Boolean = + suspend fun saveFileResult(file: File): Boolean { try { - performFileSave { + // Off-main: debug builds install StrictMode's detectDiskReads on the main thread. + val existedBefore = withContext(Dispatchers.IO) { file.exists() } + + val (view, alreadyClean) = withContext(Dispatchers.Main.immediate) { - val view = getEditorForFile(file) ?: return@withContext false - if (!view.isModified && file.exists()) return@withContext true - val index = findIndexOfEditorByFile(file) - index >= 0 && saveResultInternal(index, SaveResult()) && file.exists() + val editor = getEditorForFile(file) + editor to (editor != null && !editor.isModified && existedBefore) } + if (view == null) return false + + // Nothing to write. Returning before [performFileSave] keeps the saving flag from + // flapping true/false for a no-op, which SaveFileAction observes to enable itself. + if (alreadyClean) return true + + val result = SaveResult() + if (!performFileSave { saveEditorInternal(view, result) }) return false + if (!withContext(Dispatchers.IO) { file.exists() }) return false + + // The same follow-ups the UI save paths run (see [saveAll] and + // SaveFileAction.postExec). Without them a plugin that edits a Gradle script gets + // no sync prompt, and one that edits a layout gets no R fields for the resources + // it just added, so the next build fails on unresolved references. + if (result.gradleSaved) { + withContext(Dispatchers.Main.immediate) { editorViewModel.isSyncNeeded = true } } + if (result.xmlSaved) { + ProjectManagerImpl.getInstance().generateSources() + } + return true } catch (err: CancellationException) { throw err } catch (err: Exception) { // ContentReadWrite.writeTo reports a failed write by throwing; that must not escape // into the plugin coroutine awaiting this call. log.error("Failed to save {}", file.name, err) - false + return false } + } override fun onConfigurationChanged(newConfig: Configuration) { super.onConfigurationChanged(newConfig) @@ -995,8 +1014,23 @@ open class EditorHandlerActivity : return false } - val frag = getEditorAtIndex(index) ?: return false - val fileName = frag.file?.name ?: return false + return saveEditorInternal(getEditorAtIndex(index) ?: return false, result) + } + + /** + * Saves [frag]'s buffer and records what kind of file it was in [result]. + * + * Takes the view rather than a tab index because an index only stays valid until the first + * suspension: [CodeEditorView.save] hops to the editor's write thread, and a tab closed + * while it runs shifts every index after it. The tab to unmark is re-resolved from the + * file afterwards for the same reason. + */ + private suspend fun saveEditorInternal( + frag: CodeEditorView, + result: SaveResult, + ): Boolean { + val savedFile = frag.file ?: return false + val fileName = savedFile.name run { // Must be called before frag.save() @@ -1006,9 +1040,7 @@ open class EditorHandlerActivity : return false } - frag.file?.let { savedFile -> - fileTimestamps[savedFile.absolutePath] = savedFile.lastModified() - } + fileTimestamps[savedFile.absolutePath] = savedFile.lastModified() val isGradle = fileName.endsWith(".gradle") || fileName.endsWith(".gradle.kts") val isXml: Boolean = fileName.endsWith(".xml") @@ -1028,7 +1060,7 @@ open class EditorHandlerActivity : editorViewModel.areFilesModified = hasUnsaved // set tab as unmodified - val tabPosition = getTabPositionForFileIndex(index) + val tabPosition = getTabPositionForFileIndex(findIndexOfEditorByFile(savedFile)) if (tabPosition < 0) return@withContext val tab = content.tabs.getTabAt(tabPosition) ?: return@withContext val text = tab.text?.toString() ?: return@withContext @@ -1063,10 +1095,10 @@ open class EditorHandlerActivity : /** * Raises the saving flag for one save. * - * The count moves in the same main-thread section as the flag, so the counter's ordering - * *is* the flag's ordering. Bumping the count off-main instead let a finished off-main - * save's queued `false` land after a main-thread save had already written `true` inline - * (`Main.immediate` skips the queue when it is already on main), leaving + * The count moves in the same main-thread section as the flag it guards, so the counter's + * ordering *is* the flag's ordering. Bumping the count off-main instead let a finished + * off-main save's queued `false` land after a main-thread save had already written `true` + * inline (`Main.immediate` skips the queue when it is already on main), leaving * [EditorViewModel.areFilesSaving] false while that save was still writing. * * NonCancellable: a cancelled save (e.g. a plugin-side timeout) must still reach its @@ -1075,9 +1107,7 @@ open class EditorHandlerActivity : @VisibleForTesting internal suspend fun beginFileSave() { withContext(NonCancellable + Dispatchers.Main.immediate) { - if (activeSaveCount.incrementAndGet() == 1) { - editorViewModel.areFilesSaving = true - } + editorViewModel.beginFileSave() } } @@ -1085,9 +1115,7 @@ open class EditorHandlerActivity : @VisibleForTesting internal suspend fun endFileSave() { withContext(NonCancellable + Dispatchers.Main.immediate) { - if (activeSaveCount.decrementAndGet() == 0) { - editorViewModel.areFilesSaving = false - } + editorViewModel.endFileSave() } } 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 24db5b4d56..a3807d7fc5 100644 --- a/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt +++ b/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt @@ -16,6 +16,7 @@ import com.itsaky.androidide.plugins.services.SelectionRange import io.github.rosemoe.sora.text.Content import io.github.rosemoe.sora.widget.CodeEditor import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeoutOrNull import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode @@ -257,12 +258,23 @@ class EditorProviderImpl( /** * Saves [file]'s buffer whatever tab has focus, suspending until the bytes are on disk. * - * Suspending rather than blocking is what makes this safe to call from the main thread: - * the write itself runs there, so a blocking bridge would deadlock against it. + * Bounded by [SAVE_TIMEOUT_MS] for the same reason [onMain] is bounded: a wedged editor + * write must not park a plugin's coroutine for the rest of the session. A timeout reports + * as a failed save. + * + * Must be awaited from a coroutine, never bridged with `runBlocking` on the main thread. + * `CodeEditorView.save` runs the write on its own thread and resumes via + * `Dispatchers.Main.immediate`; `runBlocking` parks the main thread without draining the + * Android looper, so that resumption would never run. The timeout does not rescue that + * case either - the cancellation has to resume on the same blocked looper. */ override suspend fun saveFile(file: File): Boolean { val activity = activity() ?: return false - return activity.saveFileResult(file) + return withTimeoutOrNull(SAVE_TIMEOUT_MS) { activity.saveFileResult(file) } + ?: run { + log.warn("Save of {} did not complete within {}ms; aborting", file.name, SAVE_TIMEOUT_MS) + false + } } // --- Buffer edits ------------------------------------------------------- @@ -502,6 +514,11 @@ class EditorProviderImpl( companion object { private const val MAIN_EDIT_TIMEOUT_SECONDS = 5L + + // Generous next to MAIN_EDIT_TIMEOUT_SECONDS: this one covers a whole file write, not + // a single main-thread hop, and a large buffer on slow storage legitimately takes + // seconds. + private const val SAVE_TIMEOUT_MS = 30_000L private val log = LoggerFactory.getLogger(EditorProviderImpl::class.java) } } diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt index 6d80d40d7b..58ae01bb52 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt @@ -121,6 +121,41 @@ class EditorViewModel : ViewModel() { _filesSaving.value = value } + /** + * Saves in flight, paired with [areFilesSaving]. + * + * Lives here rather than in the editor activity because this ViewModel is retained across + * activity recreation while the activity is not: a save started before a dark-mode, locale + * or display-size change keeps running under `NonCancellable` against the old instance, and + * a per-instance counter would let that completion clear the flag for a save the new + * instance had already started. + * + * Main-thread confined - callers hop to Main before touching it, as the flag itself + * requires. + */ + private var activeSaveCount = 0 + + /** Raises [areFilesSaving] for the first of any number of overlapping saves. */ + fun beginFileSave() { + if (++activeSaveCount == 1) { + areFilesSaving = true + } + } + + /** + * Lowers [areFilesSaving] once the last in-flight save finishes. + * + * Floored at zero: an unbalanced call must not drive the count negative, or the "reached + * zero" test never matches again and SaveFileAction stays disabled + * (`enabled = areFilesModified && !areFilesSaving`) for the life of this ViewModel. + */ + fun endFileSave() { + activeSaveCount = (activeSaveCount - 1).coerceAtLeast(0) + if (activeSaveCount == 0) { + areFilesSaving = false + } + } + var openedFilesCache: OpenedFilesCache? get() = _openedFiles.value set(value) { diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt index 92b0905b0c..daffa1355d 100644 --- a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt @@ -59,6 +59,12 @@ class OverlappingSaveFlagTest { val activity = Robolectric.buildActivity(EditorHandlerActivity::class.java).get() val mainLooper = shadowOf(Looper.getMainLooper()) + // Drain whatever application startup and activity construction left queued. Afterwards + // the worker below is the only thing that can put a message back, which is what + // awaitPostToMain waits on. + mainLooper.idle() + assertThat(mainLooper.isIdle).isTrue() + // Save A begins on the main thread: the hop runs inline, raising the flag. runBlocking { activity.beginFileSave() } assertThat(activity.editorViewModel.areFilesSaving).isTrue() @@ -94,7 +100,13 @@ class OverlappingSaveFlagTest { } } - /** Blocks until the worker's main-thread hop is sitting in the paused looper's queue. */ + /** + * Blocks until the worker's main-thread hop is sitting in the paused looper's queue. + * + * Sound only because the caller drained the queue first: `isIdle` reports "nothing queued", + * not "the worker has posted", so any leftover startup message would satisfy it + * immediately and the test would go on to idle that message instead of the worker's hop. + */ private fun awaitPostToMain(mainLooper: ShadowLooper) { val deadline = System.currentTimeMillis() + TIMEOUT_MS while (mainLooper.isIdle && System.currentTimeMillis() < deadline) { diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt new file mode 100644 index 0000000000..16e5a6beda --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt @@ -0,0 +1,114 @@ +/* + * 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.os.Looper +import androidx.lifecycle.Observer +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.app.BaseApplication +import com.itsaky.androidide.app.EditorProviderImpl +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.runBlocking +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import org.robolectric.shadows.ShadowLooper +import java.io.File +import java.util.concurrent.atomic.AtomicReference +import kotlin.concurrent.thread + +/** Covers the file-targeted save path's outcomes that need no live editor view. */ +@RunWith(RobolectricTestRunner::class) +@Config(application = SaveFileResultTest.TestApp::class) +class SaveFileResultTest { + open class TestApp : BaseApplication() + + @Test + fun givenNoOpenEditorForTheFile_whenSaved_thenItFailsWithoutRaisingTheSavingFlag() { + val activity = Robolectric.buildActivity(EditorHandlerActivity::class.java).get() + val mainLooper = shadowOf(Looper.getMainLooper()) + mainLooper.idle() + + // Every emission, not just the final value: raising the flag for a save with nothing to + // do churns SaveFileAction's enabled state through invalidateOptionsMenu even though it + // settles back where it started. + val emissions = mutableListOf() + val observer = Observer { emissions.add(it) } + activity.editorViewModel._filesSaving.observeForever(observer) + + try { + val result = pumpMainUntil(mainLooper) { activity.saveFileResult(tempFile()) } + + assertThat(result).isFalse() + // Only observeForever's replay of the current value. + assertThat(emissions).containsExactly(false) + } finally { + activity.editorViewModel._filesSaving.removeObserver(observer) + } + } + + @Test + fun givenADetachedProvider_whenSaved_thenItReportsFailureRatherThanThrowing() { + val activity = Robolectric.buildActivity(EditorHandlerActivity::class.java).get() + val provider = EditorProviderImpl(activity) + + // What the activity's onDestroy does; the weak activity reference is cleared with it. + provider.dispose() + + // No suspension happens once the activity is gone, so blocking here cannot deadlock. + assertThat(runBlocking { provider.saveFile(tempFile()) }).isFalse() + } + + private fun tempFile() = + File.createTempFile("save-file-result-", ".kt").apply { + writeText("val answer = 42\n") + deleteOnExit() + } + + /** + * Runs [block] on a worker thread while draining the main looper, and returns its result. + * + * `saveFileResult` resumes through the main dispatcher, so `runBlocking` on Robolectric's + * main thread would park the very looper the resumption needs - the deadlock the plugin + * API documents. The worker keeps the main thread free to drain those hops. + */ + private fun pumpMainUntil( + mainLooper: ShadowLooper, + block: suspend () -> Boolean, + ): Boolean { + val outcome = AtomicReference?>(null) + val worker = + thread(isDaemon = true) { + outcome.set(runCatching { runBlocking(Dispatchers.IO) { block() } }) + } + val deadline = System.currentTimeMillis() + TIMEOUT_MS + while (outcome.get() == null && System.currentTimeMillis() < deadline) { + mainLooper.idle() + Thread.sleep(1) + } + worker.join(TIMEOUT_MS) + return checkNotNull(outcome.get()) { "save did not complete within ${TIMEOUT_MS}ms" }.getOrThrow() + } + + private companion object { + const val TIMEOUT_MS = 10_000L + } +} diff --git a/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt b/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt new file mode 100644 index 0000000000..c296af837f --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt @@ -0,0 +1,70 @@ +/* + * 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.viewmodel + +import androidx.arch.core.executor.testing.InstantTaskExecutorRule +import com.google.common.truth.Truth.assertThat +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TestRule + +/** + * The save counter is paired with `areFilesSaving` here, in the retained ViewModel, rather than + * in the editor activity: a save running under `NonCancellable` outlives the activity instance + * that started it, so a per-instance counter would let it clear the flag for a save the + * recreated instance had already started. + */ +class EditorViewModelSaveFlagTest { + @get:Rule + var rule: TestRule = InstantTaskExecutorRule() + + private lateinit var viewModel: EditorViewModel + + @Before + fun setUp() { + viewModel = EditorViewModel() + } + + @Test + fun givenOverlappingSaves_whenTheFirstFinishes_thenTheFlagStaysRaisedUntilTheLastDoes() { + viewModel.beginFileSave() + viewModel.beginFileSave() + assertThat(viewModel.areFilesSaving).isTrue() + + viewModel.endFileSave() + assertThat(viewModel.areFilesSaving).isTrue() + + viewModel.endFileSave() + assertThat(viewModel.areFilesSaving).isFalse() + } + + @Test + fun givenAnUnbalancedEnd_whenTheNextSaveRuns_thenTheFlagStillTracksIt() { + // Floored at zero. Left negative, the "reached zero" test never matches again and + // SaveFileAction stays disabled for the life of this ViewModel. + viewModel.endFileSave() + assertThat(viewModel.areFilesSaving).isFalse() + + viewModel.beginFileSave() + assertThat(viewModel.areFilesSaving).isTrue() + + viewModel.endFileSave() + assertThat(viewModel.areFilesSaving).isFalse() + } +} diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index 1611f20b16..4c6fd904a2 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -35,6 +35,23 @@ need a source change, a recompile, or both · `tooling` = API-stability milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed]** = diffed from `plugin-api/src` history (predates the dump; symbol-accurate). +### 26.36 — unreleased +- **added — File-targeted editor save** _(ADFA-5259)_ + Save a named file's open buffer and find out whether the bytes actually landed. + `saveCurrentFile` follows whichever tab the user has focused and returns as soon + as a save is dispatched, so a plugin that edits one file and then persists it can + save a different file, or read the file back before the write finishes. + `IdeEditorService.saveFile(File): Boolean` (`suspend`, `default` returning + `false`) resolves the editor by file, suspends until the write completes, and + reports the on-disk outcome — `true` also when the buffer was already clean, + `false` when the file has no open editor or the write failed. Throws + `SecurityException` on an authorization failure (no `FILESYSTEM_WRITE`, or a path + outside the plugin's allowed roots), as the other write methods do. Await it from + a coroutine; a `runBlocking` bridge on the main thread deadlocks. + + Because the method is `default`, an older IDE silently returns `false` instead of + saving — floor `plugin.min_ide_version` at `26.36` if you call it. + ### 26.33 — 2026-08-12 - **added — Plugin-contributed agent tools** _(ADFA-2592)_ **[verified]** Any `.cgp` can add tools to the AI agent, whose tool set was previously fixed at diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt index 4f2678c4e4..d550f69bcd 100644 --- a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt @@ -182,9 +182,12 @@ interface IdeEditorService { * the write completes - unlike [saveCurrentFile], which only reports that a save was * dispatched for the focused tab. * - * Safe to call from any dispatcher, the main one included: the write is marshalled to the - * editor thread and nothing blocks while it runs. Wrap the call in `withTimeout` if your - * plugin needs to bound how long it waits. + * Await it from a coroutine on any dispatcher, the main one included: the write is + * marshalled to the editor's own write thread and nothing blocks while it runs. Do not + * bridge it with `runBlocking` on the main thread - `runBlocking` parks the main thread + * without draining its looper, and the write resumes through the main dispatcher, so the + * call would never complete. The IDE bounds its own wait, so a `withTimeout` of your own + * is optional. * * Returns `true` when the buffer is on disk (including "was already clean"), `false` when * the file has no open editor or the write failed. Authorization failures throw diff --git a/plugin-manager/build.gradle.kts b/plugin-manager/build.gradle.kts index 13462875e2..88afe5855b 100644 --- a/plugin-manager/build.gradle.kts +++ b/plugin-manager/build.gradle.kts @@ -11,6 +11,12 @@ android { lint { abortOnError = false } + + // IdeEditorServiceImpl logs through android.util.Log on a denied path; without this the + // stubbed Log throws before the SecurityException the test is asserting on. + testOptions { + unitTests.isReturnDefaultValues = true + } } kotlin { diff --git a/plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImplSaveFileTest.kt b/plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImplSaveFileTest.kt new file mode 100644 index 0000000000..2632cc6642 --- /dev/null +++ b/plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImplSaveFileTest.kt @@ -0,0 +1,102 @@ +package com.itsaky.androidide.plugins.manager.services + +import com.itsaky.androidide.plugins.PluginPermission +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.io.File + +/** + * The plugin-facing contract of `IdeEditorService.saveFile`: an authorization failure throws + * rather than returning `false`, so a plugin can tell "denied" from "the write did not land". + */ +class IdeEditorServiceImplSaveFileTest { + private lateinit var projectRoot: File + private lateinit var provider: RecordingProvider + + @Before + fun setUp() { + projectRoot = + File.createTempFile("ide-editor-service-", "").apply { + delete() + mkdirs() + } + provider = RecordingProvider() + } + + @After + fun tearDown() { + projectRoot.deleteRecursively() + } + + @Test + fun saveFileDelegatesToTheProviderForAnAllowedFile() { + val file = File(projectRoot, "Main.kt") + provider.result = true + + assertTrue(runBlocking { service().saveFile(file) }) + assertEquals(file, provider.saved) + } + + @Test + fun saveFileReportsTheProvidersFailureVerbatim() { + provider.result = false + + assertFalse(runBlocking { service().saveFile(File(projectRoot, "Main.kt")) }) + } + + @Test(expected = SecurityException::class) + fun saveFileWithoutWritePermissionThrowsSecurityException() { + runBlocking { service(permissions = emptySet()).saveFile(File(projectRoot, "Main.kt")) } + } + + @Test(expected = SecurityException::class) + fun saveFileOutsideTheAllowedRootsThrowsSecurityException() { + runBlocking { service().saveFile(File("/etc/hosts")) } + } + + @Test + fun saveFileDeniedByPermissionNeverReachesTheProvider() { + runCatching { runBlocking { service(permissions = emptySet()).saveFile(File(projectRoot, "Main.kt")) } } + + assertNull(provider.saved) + } + + // The production allowlist only permits fixed on-device roots, so a test rooted at a temp + // dir must supply its own validator that permits paths under it. + private fun service(permissions: Set = setOf(PluginPermission.FILESYSTEM_WRITE)) = + IdeEditorServiceImpl( + pluginId = "test-plugin", + permissions = permissions, + editorProvider = provider, + pathValidator = + object : IdeEditorServiceImpl.PathValidator { + override fun isPathAllowed(file: File) = file.canonicalFile.toPath().startsWith(projectRoot.canonicalFile.toPath()) + + override fun getAllowedPaths() = listOf(projectRoot.absolutePath) + }, + ) + + private class RecordingProvider : IdeEditorServiceImpl.EditorProvider { + var result = false + var saved: File? = null + + override fun getCurrentFile(): File? = null + + override fun getOpenFiles(): List = emptyList() + + override fun isFileOpen(file: File): Boolean = false + + override fun getCurrentSelection(): String? = null + + override suspend fun saveFile(file: File): Boolean { + saved = file + return result + } + } +} From 2db65e673da5670c634033b1fb7594a005401dda Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Thu, 27 Aug 2026 11:37:11 -0500 Subject: [PATCH 5/6] fix(editor): address second review round on the file-targeted save beginFileSave() moves inside performFileSave's try. Its block is guaranteed to run - the context's job is NonCancellable - but withContext still honours prompt cancellation on resume when the caller is off-main, so it could increment the count and then throw past an un-armed finally. The retained counter made that latch areFilesSaving on for the rest of the session. OverlappingSaveFlagTest now pins the precondition: the flag is raised even though the call throws. The write runs off the main thread on every path. saveFileResult, saveResult and saveAllResult each dispatch to IO, because saveEditorInternal stats the file - CodeEditorView.save's own pre-check plus the timestamp bookkeeping - and both plugin entry points could arrive on Main: saveFile from a plugin coroutine, saveCurrentFile via lifecycleScope. That makes the public KDoc's "any dispatcher, the main one included" true rather than aspirational. Editor and view-container access moves back onto Main: frag.file and frag.isModified are read in one Main.immediate hop, hasUnsavedFiles() and getEditorAtIndex() get their own. CodeEditorView.save's internal ordering is unchanged and shared with saveAll, so it stays out of scope here. The write plus its bookkeeping is NonCancellable. CodeEditorView.save is not cancellation-atomic: cut between writeTo and markUnmodified(), or before the tab loses its asterisk, it leaves the bytes on disk with the buffer still flagged dirty - so saveFile could report false for a file that was written. Scoped per file inside saveEditorInternal rather than at the call site, so every entry point gets it and a multi-file save can still stop between files. The changelog said an older IDE "silently returns false". It does not: the default body keeps the addition compatible for implementers of the interface, but a caller compiled against 26.36 emits INVOKEINTERFACE against a host interface that has no saveFile, so the call site fails with NoSuchMethodError. Corrected in both the changelog and the plugin-api KDoc. Drops the GPL header from the two new test files, whose URL goes off-device. Matches the majority of app test files (26 of 43) and every plugin-manager test; the 2983 existing files that carry it are left alone. Co-Authored-By: Claude Opus 5 (1M context) --- .../editor/EditorHandlerActivity.kt | 106 ++++++++++++------ .../androidide/app/EditorProviderImpl.kt | 14 ++- .../editor/OverlappingSaveFlagTest.kt | 43 +++++++ .../activities/editor/SaveFileResultTest.kt | 17 --- .../viewmodel/EditorViewModelSaveFlagTest.kt | 17 --- docs/PLUGIN_API_CHANGELOG.md | 7 +- .../plugins/services/IdeServices.kt | 11 +- 7 files changed, 136 insertions(+), 79 deletions(-) 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 ed36efe6f6..e2c0e6c3b0 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 @@ -918,24 +918,33 @@ open class EditorHandlerActivity : return result.gradleSaved } - override suspend fun saveAllResult(progressConsumer: ((Int, Int) -> Unit)?): SaveResult { - return performFileSave { - val result = SaveResult() - for (i in 0 until editorViewModel.getOpenedFileCount()) { - saveResultInternal(i, result) - progressConsumer?.invoke(i + 1, editorViewModel.getOpenedFileCount()) - } + override suspend fun saveAllResult(progressConsumer: ((Int, Int) -> Unit)?): SaveResult = + // IO: saveEditorInternal stats the file, and callers reach here on their own + // dispatchers (SaveFileAction runs off-main, AbstractModuleAssemblerAction does not + // promise one). + withContext(Dispatchers.IO) { + performFileSave { + val result = SaveResult() + for (i in 0 until editorViewModel.getOpenedFileCount()) { + saveResultInternal(i, result) + progressConsumer?.invoke(i + 1, editorViewModel.getOpenedFileCount()) + } - return@performFileSave result + return@performFileSave result + } } - } override suspend fun saveResult( index: Int, result: SaveResult, ) { - performFileSave { - saveResultInternal(index, result) + // IO for the same reason as saveAllResult - and this one is reached from + // EditorProviderImpl.saveCurrentFile, which launches on lifecycleScope's main + // dispatcher. + withContext(Dispatchers.IO) { + performFileSave { + saveResultInternal(index, result) + } } } @@ -967,7 +976,14 @@ open class EditorHandlerActivity : if (alreadyClean) return true val result = SaveResult() - if (!performFileSave { saveEditorInternal(view, result) }) return false + // IO because the write path stats the file ([CodeEditorView.save]'s own pre-check + // and the timestamp bookkeeping), and this is reachable from a plugin coroutine + // running on Main. The write's cancellation atomicity lives in saveEditorInternal. + val saved = + withContext(Dispatchers.IO) { + performFileSave { saveEditorInternal(view, result) } + } + if (!saved) return false if (!withContext(Dispatchers.IO) { file.exists() }) return false // The same follow-ups the UI save paths run (see [saveAll] and @@ -1014,7 +1030,10 @@ open class EditorHandlerActivity : return false } - return saveEditorInternal(getEditorAtIndex(index) ?: return false, result) + // getEditorAtIndex walks the editor container, so it is resolved on Main; callers + // reach here from Dispatchers.IO (see saveAllAsync). + val frag = withContext(Dispatchers.Main.immediate) { getEditorAtIndex(index) } ?: return false + return saveEditorInternal(frag, result) } /** @@ -1024,20 +1043,31 @@ open class EditorHandlerActivity : * suspension: [CodeEditorView.save] hops to the editor's write thread, and a tab closed * while it runs shifts every index after it. The tab to unmark is re-resolved from the * file afterwards for the same reason. + * + * Call it off the main thread - it stats the file and delegates to [CodeEditorView.save], + * which marshals its own UI work. Editor state is read back on Main. + * + * The write and the bookkeeping that follows it are `NonCancellable`: [CodeEditorView.save] + * is not cancellation-atomic, so cut between `writeTo` and its `markUnmodified()` - or + * before the tab below loses its asterisk - it would leave the bytes on disk with the + * buffer still flagged dirty. Scoped per file, so a multi-file save can still stop between + * files. */ private suspend fun saveEditorInternal( frag: CodeEditorView, result: SaveResult, ): Boolean { - val savedFile = frag.file ?: return false + // Editor state lives on the view: read it on Main, and read isModified before + // frag.save() clears it. + val (savedFile, modified) = + withContext(Dispatchers.Main.immediate) { + frag.file?.let { it to frag.isModified } + } ?: return false val fileName = savedFile.name - run { - // Must be called before frag.save() - // Otherwise, it'll always return false - val modified = frag.isModified + return withContext(NonCancellable) { if (!frag.save()) { - return false + return@withContext false } fileTimestamps[savedFile.absolutePath] = savedFile.lastModified() @@ -1051,25 +1081,26 @@ open class EditorHandlerActivity : if (!result.xmlSaved) { result.xmlSaved = modified && isXml } - } - val hasUnsaved = hasUnsavedFiles() + // Walks the editor container, so it belongs on Main like the tab update below. + val hasUnsaved = withContext(Dispatchers.Main.immediate) { hasUnsavedFiles() } - withContext(Dispatchers.Main) { - val content = contentOrNull ?: return@withContext - editorViewModel.areFilesModified = hasUnsaved - - // set tab as unmodified - val tabPosition = getTabPositionForFileIndex(findIndexOfEditorByFile(savedFile)) - if (tabPosition < 0) return@withContext - val tab = content.tabs.getTabAt(tabPosition) ?: return@withContext - val text = tab.text?.toString() ?: return@withContext - if (text.startsWith('*')) { - tab.text = text.substring(1) + withContext(Dispatchers.Main) { + val content = contentOrNull ?: return@withContext + editorViewModel.areFilesModified = hasUnsaved + + // set tab as unmodified + val tabPosition = getTabPositionForFileIndex(findIndexOfEditorByFile(savedFile)) + if (tabPosition < 0) return@withContext + val tab = content.tabs.getTabAt(tabPosition) ?: return@withContext + val text = tab.text?.toString() ?: return@withContext + if (text.startsWith('*')) { + tab.text = text.substring(1) + } } - } - return true + true + } } private fun hasUnsavedFiles() = @@ -1084,8 +1115,13 @@ open class EditorHandlerActivity : * first one to finish must not clear the flag while the other is still writing. */ private suspend inline fun performFileSave(crossinline action: suspend () -> T): T { - beginFileSave() try { + // Inside the try, not before it: [beginFileSave]'s block is guaranteed to run (its + // context's job is NonCancellable), but `withContext` still honours prompt + // cancellation on *resume* when the caller is off-main - so it can increment the + // count and then throw. Outside the try that increment never gets its decrement, + // and areFilesSaving latches on for the life of the retained ViewModel. + beginFileSave() return action() } finally { endFileSave() 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 a3807d7fc5..461e47b08c 100644 --- a/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt +++ b/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt @@ -259,8 +259,12 @@ class EditorProviderImpl( * Saves [file]'s buffer whatever tab has focus, suspending until the bytes are on disk. * * Bounded by [SAVE_TIMEOUT_MS] for the same reason [onMain] is bounded: a wedged editor - * write must not park a plugin's coroutine for the rest of the session. A timeout reports - * as a failed save. + * must not park a plugin's coroutine for the rest of the session. The bound covers the + * phases that need the main thread - resolving the editor is what can wedge. Once bytes + * start moving the save runs to completion under `NonCancellable`, because + * `CodeEditorView.save` is not cancellation-atomic: interrupted between the write and its + * `markUnmodified()` it would leave the file written but the buffer flagged dirty. So a + * `false` from here never means "written, but reported unwritten". * * Must be awaited from a coroutine, never bridged with `runBlocking` on the main thread. * `CodeEditorView.save` runs the write on its own thread and resumes via @@ -515,9 +519,9 @@ class EditorProviderImpl( companion object { private const val MAIN_EDIT_TIMEOUT_SECONDS = 5L - // Generous next to MAIN_EDIT_TIMEOUT_SECONDS: this one covers a whole file write, not - // a single main-thread hop, and a large buffer on slow storage legitimately takes - // seconds. + // Generous next to MAIN_EDIT_TIMEOUT_SECONDS: this covers resolving the editor plus a + // whole file write, not a single main-thread hop, and a large buffer on slow storage + // legitimately takes seconds. private const val SAVE_TIMEOUT_MS = 30_000L private val log = LoggerFactory.getLogger(EditorProviderImpl::class.java) } diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt index daffa1355d..95ee134af3 100644 --- a/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt @@ -20,7 +20,11 @@ package com.itsaky.androidide.activities.editor import android.os.Looper import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.app.BaseApplication +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Test import org.junit.runner.RunWith @@ -31,6 +35,7 @@ import org.robolectric.annotation.Config import org.robolectric.shadows.ShadowLooper import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference import kotlin.concurrent.thread /** @@ -100,6 +105,44 @@ class OverlappingSaveFlagTest { } } + /** + * `beginFileSave` raises the flag from inside a `NonCancellable` block, but `withContext` + * still honours prompt cancellation when it resumes - so it can raise the flag and *then* + * throw. That is why `performFileSave` calls it inside the `try`: called before it, the + * increment would never reach the `finally`'s decrement and `areFilesSaving` would latch on + * for the life of the retained ViewModel. + */ + @Test + fun givenTheOuterJobIsCancelledDuringTheBeginHop_thenTheFlagIsRaisedAndTheCallStillThrows() { + val activity = Robolectric.buildActivity(EditorHandlerActivity::class.java).get() + val mainLooper = shadowOf(Looper.getMainLooper()) + mainLooper.idle() + + val job = Job() + val thrown = AtomicReference(null) + val finished = CountDownLatch(1) + CoroutineScope(job + Dispatchers.IO).launch { + try { + activity.beginFileSave() + } catch (err: Throwable) { + thrown.set(err) + } finally { + finished.countDown() + } + } + + // The hop is queued on the paused looper; cancel the outer job before draining it. + awaitPostToMain(mainLooper) + job.cancel() + mainLooper.idle() + assertThat(finished.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue() + + // The block ran despite the cancellation, so the count was incremented ... + assertThat(activity.editorViewModel.areFilesSaving).isTrue() + // ... and the caller still saw a throw, which only a `finally` can balance. + assertThat(thrown.get()).isInstanceOf(CancellationException::class.java) + } + /** * Blocks until the worker's main-thread hop is sitting in the paused looper's queue. * diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt index 16e5a6beda..bd58b7350e 100644 --- a/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt @@ -1,20 +1,3 @@ -/* - * 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.os.Looper diff --git a/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt b/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt index c296af837f..b5502dfe7e 100644 --- a/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt +++ b/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt @@ -1,20 +1,3 @@ -/* - * 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.viewmodel import androidx.arch.core.executor.testing.InstantTaskExecutorRule diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index 4c6fd904a2..b4af2c3708 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -49,8 +49,11 @@ milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed] outside the plugin's allowed roots), as the other write methods do. Await it from a coroutine; a `runBlocking` bridge on the main thread deadlocks. - Because the method is `default`, an older IDE silently returns `false` instead of - saving — floor `plugin.min_ide_version` at `26.36` if you call it. + Because an older IDE's `IdeEditorService` has no `saveFile` at all, calling it + there fails with `NoSuchMethodError` at the call site — floor + `plugin.min_ide_version` at `26.36` if you call it. The `default` body only keeps + the addition compatible for *implementers* of the interface; it does nothing for a + caller running against an older host. ### 26.33 — 2026-08-12 - **added — Plugin-contributed agent tools** _(ADFA-2592)_ **[verified]** diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt index d550f69bcd..a1a1e17e86 100644 --- a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt @@ -186,8 +186,10 @@ interface IdeEditorService { * marshalled to the editor's own write thread and nothing blocks while it runs. Do not * bridge it with `runBlocking` on the main thread - `runBlocking` parks the main thread * without draining its looper, and the write resumes through the main dispatcher, so the - * call would never complete. The IDE bounds its own wait, so a `withTimeout` of your own - * is optional. + * call would never complete. The IDE bounds its own wait, so a `withTimeout` of your own is + * optional - and cannot abandon a write already in progress: once bytes start moving the + * save runs to completion, so cancelling never leaves the file written but the buffer + * flagged dirty. * * Returns `true` when the buffer is on disk (including "was already clean"), `false` when * the file has no open editor or the write failed. Authorization failures throw @@ -195,7 +197,10 @@ interface IdeEditorService { * method here: the caller lacks FILESYSTEM_WRITE, or [file] is outside the plugin's * allowed roots. * - * Default-implemented (no-op) so adding it is a backward-compatible interface extension. + * The default body makes this a compatible addition for *implementers* of the interface. It + * does nothing for a *caller*: an older IDE ships an `IdeEditorService` with no `saveFile` + * at all, so the call site fails with `NoSuchMethodError`. Floor `plugin.min_ide_version` + * at the release that introduced this - see PLUGIN_API_CHANGELOG.md. */ suspend fun saveFile(file: File): Boolean = false From e56a7707b74489ea70f6ac373094cdedad2342a0 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Thu, 27 Aug 2026 12:41:37 -0500 Subject: [PATCH 6/6] fix(editor): report why a file save ended, not just whether it did A save's answer is now a FileSaveOutcome - WRITTEN, ALREADY_CLEAN, NOT_OPEN or FAILED - recorded in a holder the caller passes in, rather than a bare Boolean returned through a scope that may already be cancelled. Three of the review findings were the same shape: a Boolean cannot say what happened. The bound in EditorProviderImpl.saveFile no longer turns a completed write into false. NonCancellable made the write atomic, but the resume still threw TimeoutCancellationException into withTimeoutOrNull, which yielded null. The outcome is set from inside the NonCancellable section, so it survives the cancellation and is what saveFile returns; the log line no longer claims to have aborted something that ran to completion. CodeEditorView.save's "nothing to do" false is no longer reported as a failed write. Past the pre-check, an unmodified buffer means a concurrent UI save-all wrote this same content and marked it clean, so that is ALREADY_CLEAN, not FAILED. And an IllegalStateException from save() can only be the binding getter's "Binding has been destroyed", raised by markUnmodified()/notifySaved() after the write - the write section itself uses the nullable _binding? - so a tab closed mid-save loses its bookkeeping, not its bytes. Every claim that content reached disk is checked against disk before it is returned. The post-write follow-ups moved inside NonCancellable. isSyncNeeded and generateSources() sat outside it, and the file.exists() check above them was itself a suspension point, so a cancellation landing after the write dropped the sync prompt for a Gradle script already on disk, and the R fields for a layout's new resources. IdeEditorServiceImpl.saveFile wraps ensureFileAccessible in Dispatchers.IO - it stats the filesystem through the host pathValidator or canonicalPath, one frame above the hop added to keep this off the main thread. areFilesModified is computed in the block that writes it. Sampling it in an earlier Main.immediate hop and consuming it in a later queued one let an edit made in another tab meanwhile be clobbered by a stale false, greying out Save over a dirty buffer. Also drops a queue round-trip from the NonCancellable section. Reverts the module-wide isReturnDefaultValues in plugin-manager. It was there for two Log.d calls; those now go through slf4j, as the module's other service impls already do, so no android.jar stub is needed and the other four test classes keep failing loudly on unmocked Android calls. android.util.Log was IdeEditorServiceImpl's only android import. Drops the claim that the default body keeps saveFile compatible for implementers. plugin-api sets no -Xjvm-default, so it compiles to an abstract interface method plus a DefaultImpls static - both in the ABI dump - and a previously-compiled implementer would get AbstractMethodError, not the default. --- .../editor/EditorHandlerActivity.kt | 109 +++++++++++++----- .../activities/editor/FileSaveOutcome.kt | 28 +++++ .../androidide/app/EditorProviderImpl.kt | 34 ++++-- .../activities/editor/SaveFileResultTest.kt | 6 +- docs/PLUGIN_API_CHANGELOG.md | 9 +- .../plugins/services/IdeServices.kt | 10 +- plugin-manager/build.gradle.kts | 6 - .../manager/services/IdeEditorServiceImpl.kt | 24 +++- 8 files changed, 164 insertions(+), 62 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/activities/editor/FileSaveOutcome.kt 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 e2c0e6c3b0..92ae21f419 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 @@ -115,6 +115,7 @@ import java.util.WeakHashMap import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference import java.util.function.Consumer /** @@ -958,8 +959,16 @@ open class EditorHandlerActivity : * The editor is handed to the write as a view, never as a tab index: an index is only * valid until the first suspension, and resolving [file] twice through two different * indexing schemes could save a different open buffer than the one that was checked. + * + * [outcome] receives the verdict from *inside* the save's `NonCancellable` section, so it + * survives a caller whose timeout elapses mid-write: the return value cannot report that + * (the resume throws [CancellationException] first), but [outcome] still says what reached + * disk. Read it instead of the return value when the await may be cut short. */ - suspend fun saveFileResult(file: File): Boolean { + internal suspend fun saveFileResult( + file: File, + outcome: AtomicReference = AtomicReference(FileSaveOutcome.FAILED), + ): Boolean { try { // Off-main: debug builds install StrictMode's detectDiskReads on the main thread. val existedBefore = withContext(Dispatchers.IO) { file.exists() } @@ -969,40 +978,56 @@ open class EditorHandlerActivity : val editor = getEditorForFile(file) editor to (editor != null && !editor.isModified && existedBefore) } - if (view == null) return false + if (view == null) { + outcome.set(FileSaveOutcome.NOT_OPEN) + return false + } // Nothing to write. Returning before [performFileSave] keeps the saving flag from // flapping true/false for a no-op, which SaveFileAction observes to enable itself. - if (alreadyClean) return true + if (alreadyClean) { + outcome.set(FileSaveOutcome.ALREADY_CLEAN) + return true + } - val result = SaveResult() // IO because the write path stats the file ([CodeEditorView.save]'s own pre-check // and the timestamp bookkeeping), and this is reachable from a plugin coroutine - // running on Main. The write's cancellation atomicity lives in saveEditorInternal. - val saved = - withContext(Dispatchers.IO) { - performFileSave { saveEditorInternal(view, result) } + // running on Main. + // + // NonCancellable spans the write *and* the follow-ups it implies: cancelled + // between them, a Gradle script would land on disk with no sync prompt and a + // layout with no regenerated R fields - the very failures the block below exists + // to prevent, reached by the cancellation path instead. + withContext(Dispatchers.IO + NonCancellable) { + val result = SaveResult() + val saved = performFileSave { saveEditorInternal(view, result) } + // Every claim that the content is on disk is checked against disk. Covers the + // corners where CodeEditorView.save returns false without writing and without + // the buffer being clean either - an archive extension, a null text. + outcome.set( + if (saved.reachedDisk && !file.exists()) FileSaveOutcome.FAILED else saved, + ) + if (outcome.get() != FileSaveOutcome.WRITTEN) return@withContext + + // The same follow-ups the UI save paths run (see [saveAll] and + // SaveFileAction.postExec). Without them a plugin that edits a Gradle script + // gets no sync prompt, and one that edits a layout gets no R fields for the + // resources it just added, so the next build fails on unresolved references. + if (result.gradleSaved) { + withContext(Dispatchers.Main.immediate) { editorViewModel.isSyncNeeded = true } + } + if (result.xmlSaved) { + ProjectManagerImpl.getInstance().generateSources() } - if (!saved) return false - if (!withContext(Dispatchers.IO) { file.exists() }) return false - - // The same follow-ups the UI save paths run (see [saveAll] and - // SaveFileAction.postExec). Without them a plugin that edits a Gradle script gets - // no sync prompt, and one that edits a layout gets no R fields for the resources - // it just added, so the next build fails on unresolved references. - if (result.gradleSaved) { - withContext(Dispatchers.Main.immediate) { editorViewModel.isSyncNeeded = true } - } - if (result.xmlSaved) { - ProjectManagerImpl.getInstance().generateSources() } - return true + return outcome.get().reachedDisk } catch (err: CancellationException) { throw err } catch (err: Exception) { // ContentReadWrite.writeTo reports a failed write by throwing; that must not escape // into the plugin coroutine awaiting this call. log.error("Failed to save {}", file.name, err) + outcome.set(FileSaveOutcome.FAILED) return false } } @@ -1033,7 +1058,8 @@ open class EditorHandlerActivity : // getEditorAtIndex walks the editor container, so it is resolved on Main; callers // reach here from Dispatchers.IO (see saveAllAsync). val frag = withContext(Dispatchers.Main.immediate) { getEditorAtIndex(index) } ?: return false - return saveEditorInternal(frag, result) + // Only a write counts here, preserving what this returned before it reported outcomes. + return saveEditorInternal(frag, result) == FileSaveOutcome.WRITTEN } /** @@ -1056,18 +1082,39 @@ open class EditorHandlerActivity : private suspend fun saveEditorInternal( frag: CodeEditorView, result: SaveResult, - ): Boolean { + ): FileSaveOutcome { // Editor state lives on the view: read it on Main, and read isModified before // frag.save() clears it. val (savedFile, modified) = withContext(Dispatchers.Main.immediate) { frag.file?.let { it to frag.isModified } - } ?: return false + } ?: return FileSaveOutcome.NOT_OPEN val fileName = savedFile.name return withContext(NonCancellable) { - if (!frag.save()) { - return@withContext false + val wrote = + try { + frag.save() + } catch (err: IllegalStateException) { + // The only IllegalStateException [CodeEditorView.save] can raise is its + // `binding` getter's "Binding has been destroyed", and the two calls that + // go through it - markUnmodified() and notifySaved() - both run *after* the + // write (the write section itself uses the nullable `_binding?`). So a tab + // closed mid-save loses its bookkeeping, not its bytes. + log.warn("Editor for {} was disposed after its write completed", fileName, err) + return@withContext if (savedFile.exists()) { + FileSaveOutcome.WRITTEN + } else { + FileSaveOutcome.FAILED + } + } + + if (!wrote) { + // save() reports "nothing to do" and "the write failed" with the same false. + // The buffer was sampled clean moments ago on Main, so an unmodified buffer here + // is the former - typically a concurrent UI save-all wrote this same content and + // marked it unmodified. + return@withContext if (modified) FileSaveOutcome.FAILED else FileSaveOutcome.ALREADY_CLEAN } fileTimestamps[savedFile.absolutePath] = savedFile.lastModified() @@ -1082,12 +1129,12 @@ open class EditorHandlerActivity : result.xmlSaved = modified && isXml } - // Walks the editor container, so it belongs on Main like the tab update below. - val hasUnsaved = withContext(Dispatchers.Main.immediate) { hasUnsavedFiles() } - withContext(Dispatchers.Main) { val content = contentOrNull ?: return@withContext - editorViewModel.areFilesModified = hasUnsaved + // Computed here rather than in an earlier hop: this block is queued, and a + // snapshot taken before it ran would clobber an edit made in another tab + // meanwhile - leaving Save greyed out over a dirty buffer. + editorViewModel.areFilesModified = hasUnsavedFiles() // set tab as unmodified val tabPosition = getTabPositionForFileIndex(findIndexOfEditorByFile(savedFile)) @@ -1099,7 +1146,7 @@ open class EditorHandlerActivity : } } - true + FileSaveOutcome.WRITTEN } } diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/FileSaveOutcome.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/FileSaveOutcome.kt new file mode 100644 index 0000000000..6c13aba1ed --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/FileSaveOutcome.kt @@ -0,0 +1,28 @@ +package com.itsaky.androidide.activities.editor + +/** + * How one file's save ended. + * + * `CodeEditorView.save` reports "nothing to do" and "the write failed" with the same `false`, + * and the two mean opposite things to a caller that wants to know whether its content is on + * disk. A save also has an outcome even when the coroutine awaiting it is cancelled before it + * can return one, so the verdict is recorded rather than only returned. + */ +internal enum class FileSaveOutcome { + /** The buffer was written to disk on this call. */ + WRITTEN, + + /** Nothing to write: the buffer already matched what is on disk. */ + ALREADY_CLEAN, + + /** No open editor holds the file. */ + NOT_OPEN, + + /** A write was attempted and did not land. */ + FAILED, + ; + + /** Whether the buffer's content is on disk, regardless of which call put it there. */ + val reachedDisk: Boolean + get() = this == WRITTEN || this == ALREADY_CLEAN +} 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 461e47b08c..189eb6f287 100644 --- a/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt +++ b/app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt @@ -4,6 +4,7 @@ import android.os.Handler import android.os.Looper import androidx.lifecycle.lifecycleScope import com.itsaky.androidide.activities.editor.EditorHandlerActivity +import com.itsaky.androidide.activities.editor.FileSaveOutcome import com.itsaky.androidide.activities.editor.PeerCursorOverlayManager import com.itsaky.androidide.editor.ui.IDEEditor import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent @@ -259,12 +260,14 @@ class EditorProviderImpl( * Saves [file]'s buffer whatever tab has focus, suspending until the bytes are on disk. * * Bounded by [SAVE_TIMEOUT_MS] for the same reason [onMain] is bounded: a wedged editor - * must not park a plugin's coroutine for the rest of the session. The bound covers the - * phases that need the main thread - resolving the editor is what can wedge. Once bytes - * start moving the save runs to completion under `NonCancellable`, because - * `CodeEditorView.save` is not cancellation-atomic: interrupted between the write and its - * `markUnmodified()` it would leave the file written but the buffer flagged dirty. So a - * `false` from here never means "written, but reported unwritten". + * must not park a plugin's coroutine for the rest of the session. Once bytes start moving + * the save runs to completion under `NonCancellable`, because `CodeEditorView.save` is not + * cancellation-atomic: interrupted between the write and its `markUnmodified()` it would + * leave the file written but the buffer flagged dirty. + * + * The bound therefore cannot abort a write in progress, only outlast it - so the answer + * comes from the outcome the save records, not from whether this coroutine got to see it + * return. That is what keeps a `false` here from meaning "written, but reported unwritten". * * Must be awaited from a coroutine, never bridged with `runBlocking` on the main thread. * `CodeEditorView.save` runs the write on its own thread and resumes via @@ -274,11 +277,20 @@ class EditorProviderImpl( */ override suspend fun saveFile(file: File): Boolean { val activity = activity() ?: return false - return withTimeoutOrNull(SAVE_TIMEOUT_MS) { activity.saveFileResult(file) } - ?: run { - log.warn("Save of {} did not complete within {}ms; aborting", file.name, SAVE_TIMEOUT_MS) - false - } + val outcome = AtomicReference(FileSaveOutcome.FAILED) + val returned = withTimeoutOrNull(SAVE_TIMEOUT_MS) { activity.saveFileResult(file, outcome) } + val reachedDisk = outcome.get().reachedDisk + if (returned == null) { + // Not "aborting": the write is NonCancellable, so it either had not started or ran + // to completion regardless of this bound. + log.warn( + "Save of {} outran the {}ms bound; outcome was {}", + file.name, + SAVE_TIMEOUT_MS, + outcome.get(), + ) + } + return reachedDisk } // --- Buffer edits ------------------------------------------------------- diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt index bd58b7350e..ee8dca44b6 100644 --- a/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt @@ -38,9 +38,13 @@ class SaveFileResultTest { activity.editorViewModel._filesSaving.observeForever(observer) try { - val result = pumpMainUntil(mainLooper) { activity.saveFileResult(tempFile()) } + // The outcome holder is what a caller reads when its own await may be cut short, so + // it has to be populated on the early-return paths too, not just around the write. + val outcome = AtomicReference(FileSaveOutcome.FAILED) + val result = pumpMainUntil(mainLooper) { activity.saveFileResult(tempFile(), outcome) } assertThat(result).isFalse() + assertThat(outcome.get()).isEqualTo(FileSaveOutcome.NOT_OPEN) // Only observeForever's replay of the current value. assertThat(emissions).containsExactly(false) } finally { diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index b4af2c3708..f35d285135 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -51,9 +51,12 @@ milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed] Because an older IDE's `IdeEditorService` has no `saveFile` at all, calling it there fails with `NoSuchMethodError` at the call site — floor - `plugin.min_ide_version` at `26.36` if you call it. The `default` body only keeps - the addition compatible for *implementers* of the interface; it does nothing for a - caller running against an older host. + `plugin.min_ide_version` at `26.36` if you call it. The Kotlin `default` body is + not a Java default method: `plugin-api` sets no `-Xjvm-default`, so the compiler + emits an abstract interface method plus a `DefaultImpls` static (both visible in + the ABI dump). It therefore rescues neither a caller nor an implementer compiled + against an older `plugin-api` — the latter gets `AbstractMethodError`. Only the + host's own implementers benefit, and they are recompiled with it. ### 26.33 — 2026-08-12 - **added — Plugin-contributed agent tools** _(ADFA-2592)_ **[verified]** diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt index a1a1e17e86..82d08ddbc9 100644 --- a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt @@ -197,10 +197,12 @@ interface IdeEditorService { * method here: the caller lacks FILESYSTEM_WRITE, or [file] is outside the plugin's * allowed roots. * - * The default body makes this a compatible addition for *implementers* of the interface. It - * does nothing for a *caller*: an older IDE ships an `IdeEditorService` with no `saveFile` - * at all, so the call site fails with `NoSuchMethodError`. Floor `plugin.min_ide_version` - * at the release that introduced this - see PLUGIN_API_CHANGELOG.md. + * The default body is a convenience for the host's own implementers, not a compatibility + * shim: this module sets no `-Xjvm-default`, so it compiles to an abstract interface method + * plus a `DefaultImpls` static rather than a Java default method. An older IDE ships an + * `IdeEditorService` with no `saveFile` at all, so a call there fails with + * `NoSuchMethodError`. Floor `plugin.min_ide_version` at the release that introduced this - + * see PLUGIN_API_CHANGELOG.md. */ suspend fun saveFile(file: File): Boolean = false diff --git a/plugin-manager/build.gradle.kts b/plugin-manager/build.gradle.kts index 88afe5855b..13462875e2 100644 --- a/plugin-manager/build.gradle.kts +++ b/plugin-manager/build.gradle.kts @@ -11,12 +11,6 @@ android { lint { abortOnError = false } - - // IdeEditorServiceImpl logs through android.util.Log on a denied path; without this the - // stubbed Log throws before the SecurityException the test is asserting on. - testOptions { - unitTests.isReturnDefaultValues = true - } } kotlin { diff --git a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt index 5fb8fd8e5b..7de586be3e 100644 --- a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt +++ b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt @@ -2,7 +2,6 @@ package com.itsaky.androidide.plugins.manager.services -import android.util.Log import com.itsaky.androidide.plugins.PluginPermission import com.itsaky.androidide.plugins.services.CursorPosition import com.itsaky.androidide.plugins.services.EditorContentChangeListener @@ -10,6 +9,9 @@ import com.itsaky.androidide.plugins.services.FileChangeListener import com.itsaky.androidide.plugins.services.IdeEditorService import com.itsaky.androidide.plugins.services.SelectionRange import com.itsaky.androidide.utils.Environment +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext +import org.slf4j.LoggerFactory import java.io.File import java.util.concurrent.CopyOnWriteArrayList @@ -321,8 +323,13 @@ class IdeEditorServiceImpl( override suspend fun saveFile(file: File): Boolean { requireWrite() - ensureFileAccessible(file) - return editorProvider.saveFile(file) + // IO: ensureFileAccessible stats the filesystem - the host pathValidator, or + // canonicalPath in the default allowlist - and this method invites callers to await it + // on any dispatcher, the main one included. + return withContext(Dispatchers.IO) { + ensureFileAccessible(file) + editorProvider.saveFile(file) + } } override fun insertTextAtCursor(text: String): Boolean { @@ -481,7 +488,7 @@ class IdeEditorServiceImpl( pathValidator?.let { validator -> val ok = runCatching { validator.isPathAllowed(file) }.getOrDefault(false) if (!ok) { - Log.d(TAG, "[$pluginId] pathValidator rejected ${file.absolutePath}") + log.debug("[{}] pathValidator rejected {}", pluginId, file.absolutePath) } return ok } @@ -493,7 +500,12 @@ class IdeEditorServiceImpl( val allowed = isFileAccessAllowedDefault(file) if (!allowed) { - Log.d(TAG, "[$pluginId] static allowlist rejected ${file.absolutePath}; allowed roots=$defaultAllowedPaths") + log.debug( + "[{}] static allowlist rejected {}; allowed roots={}", + pluginId, + file.absolutePath, + defaultAllowedPaths, + ) } return allowed } @@ -524,6 +536,6 @@ class IdeEditorServiceImpl( } companion object { - private const val TAG = "IdeEditorService" + private val log = LoggerFactory.getLogger(IdeEditorServiceImpl::class.java) } }