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..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 @@ -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 @@ -114,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 /** @@ -917,24 +919,116 @@ 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) + } + } + } + + /** + * 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. + * + * 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. + */ + 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() } + + val (view, alreadyClean) = + withContext(Dispatchers.Main.immediate) { + val editor = getEditorForFile(file) + editor to (editor != null && !editor.isModified && existedBefore) + } + 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) { + outcome.set(FileSaveOutcome.ALREADY_CLEAN) + return true + } + + // 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. + // + // 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() + } + } + 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 } } @@ -961,21 +1055,70 @@ open class EditorHandlerActivity : return false } - val frag = getEditorAtIndex(index) ?: return false - val fileName = frag.file?.name ?: return false + // 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 + // Only a write counts here, preserving what this returned before it reported outcomes. + return saveEditorInternal(frag, result) == FileSaveOutcome.WRITTEN + } - run { - // Must be called before frag.save() - // Otherwise, it'll always return false - val modified = frag.isModified - if (!frag.save()) { - return false - } + /** + * 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. + * + * 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, + ): 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 FileSaveOutcome.NOT_OPEN + val fileName = savedFile.name + + return withContext(NonCancellable) { + 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 + } + } - frag.file?.let { savedFile -> - fileTimestamps[savedFile.absolutePath] = savedFile.lastModified() + 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() + val isGradle = fileName.endsWith(".gradle") || fileName.endsWith(".gradle.kts") val isXml: Boolean = fileName.endsWith(".xml") if (!result.gradleSaved) { @@ -985,25 +1128,26 @@ open class EditorHandlerActivity : if (!result.xmlSaved) { result.xmlSaved = modified && isXml } - } - val hasUnsaved = hasUnsavedFiles() - - withContext(Dispatchers.Main) { - val content = contentOrNull ?: return@withContext - editorViewModel.areFilesModified = hasUnsaved - - // set tab as unmodified - val tabPosition = getTabPositionForFileIndex(index) - 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 + // 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)) + 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 + FileSaveOutcome.WRITTEN + } } private fun hasUnsavedFiles() = @@ -1011,18 +1155,50 @@ 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) 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 { - setFilesSaving(false) + endFileSave() + } + } + + /** + * Raises the saving flag for one save. + * + * 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 + * matching [endFileSave], or SaveFileAction stays disabled for the rest of the session. + */ + @VisibleForTesting + internal suspend fun beginFileSave() { + withContext(NonCancellable + Dispatchers.Main.immediate) { + editorViewModel.beginFileSave() } } - private suspend fun setFilesSaving(saving: Boolean) { - withContext(Dispatchers.Main.immediate) { - editorViewModel.areFilesSaving = saving + /** Lowers the saving flag once the last in-flight save finishes. See [beginFileSave]. */ + @VisibleForTesting + internal suspend fun endFileSave() { + withContext(NonCancellable + Dispatchers.Main.immediate) { + editorViewModel.endFileSave() } } 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 4fc16ff0fc..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 @@ -16,6 +17,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 @@ -254,6 +256,43 @@ class EditorProviderImpl( return true } + /** + * 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. 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 + * `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 + 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 ------------------------------------------------------- override fun insertTextAtCursor(text: String): Boolean = @@ -491,6 +530,11 @@ class EditorProviderImpl( companion object { private const val MAIN_EDIT_TIMEOUT_SECONDS = 5L + + // 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/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 new file mode 100644 index 0000000000..95ee134af3 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/OverlappingSaveFlagTest.kt @@ -0,0 +1,164 @@ +/* + * 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.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 +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 java.util.concurrent.atomic.AtomicReference +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()) + + // 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() + + // 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(isDaemon = true) { + runBlocking(Dispatchers.IO) { activity.endFileSave() } + ended.countDown() + } + + try { + 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(TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue() + assertThat(activity.editorViewModel.areFilesSaving).isTrue() + + // 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) + } + } + + /** + * `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. + * + * 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) { + Thread.sleep(1) + } + assertThat(mainLooper.isIdle).isFalse() + } + + private companion object { + const val TIMEOUT_MS = 10_000L + } +} 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..ee8dca44b6 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/SaveFileResultTest.kt @@ -0,0 +1,101 @@ +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 { + // 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 { + 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..b5502dfe7e --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/viewmodel/EditorViewModelSaveFlagTest.kt @@ -0,0 +1,53 @@ +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..f35d285135 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -35,6 +35,29 @@ 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 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 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]** Any `.cgp` can add tools to the AI agent, whose tool set was previously fixed at 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..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 @@ -172,10 +172,40 @@ 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. + * + * 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 - 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 + * [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. + * + * 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 + 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..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 @@ -98,6 +100,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 +321,17 @@ class IdeEditorServiceImpl( return editorProvider.saveCurrentFile() } + override suspend fun saveFile(file: File): Boolean { + requireWrite() + // 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 { if (!writableCurrentFile()) return false return editorProvider.insertTextAtCursor(text) @@ -473,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 } @@ -485,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 } @@ -516,6 +536,6 @@ class IdeEditorServiceImpl( } companion object { - private const val TAG = "IdeEditorService" + private val log = LoggerFactory.getLogger(IdeEditorServiceImpl::class.java) } } 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 + } + } +}