-
-
Notifications
You must be signed in to change notification settings - Fork 55
ADFA-5259 | Add file-targeted IdeEditorService.saveFile(File) API #1750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stage
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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.AtomicInteger | ||
| import java.util.function.Consumer | ||
|
|
||
| /** | ||
|
|
@@ -136,6 +138,9 @@ open class EditorHandlerActivity : | |
|
|
||
| private val fileTimestamps = ConcurrentHashMap<String, Long>() | ||
|
|
||
| /** Number of saves in flight. Main-thread confined; see [beginFileSave]. */ | ||
| private val activeSaveCount = AtomicInteger(0) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F13 - The doc says "Main-thread confined; see [beginFileSave]", and both mutations now happen inside A plain (If F01 is addressed by moving the counter into the ViewModel, decide the threading story there and document it once.) |
||
|
|
||
| private val pluginTabIndices = mutableMapOf<String, Int>() | ||
| private val tabIndexToPluginId = mutableMapOf<Int, String>() | ||
| private var lastAppliedPluginFontScale = EditorPreferences.editorFontScale | ||
|
|
@@ -938,6 +943,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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F07 - the atomicity claim does not survive the write "Resolution and the write share one main-thread continuation, so a tab close cannot shift the index out from under the save" holds only up to the
Close a tab to the left of the saved file during a large write and the asterisk is stripped from the wrong tab, so a genuinely dirty buffer renders as clean. Re-resolve the tab from the file (or from |
||
| * the index out from under the save. | ||
| */ | ||
| suspend fun saveFileResult(file: File): Boolean = | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F11 - the headline feature has no test
Untested and non-obvious behaviors that would silently regress:
That last one is the highest-value gap: the plugin-facing KDoc explicitly advertises it ("Authorization failures throw SecurityException rather than returning false"), and a plugin-visible security guarantee with zero coverage is worth a test on its own. A plain JUnit test over |
||
| try { | ||
| performFileSave { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F12 - the flag is raised before there is any work to do
Resolve the editor first and enter |
||
| withContext(Dispatchers.Main.immediate) { | ||
| val view = getEditorForFile(file) ?: return@withContext false | ||
| if (!view.isModified && file.exists()) return@withContext true | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F03 - main-thread disk IO trips StrictMode
Both It also cuts against the established pattern in this same file - |
||
| val index = findIndexOfEditorByFile(file) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F05 - the file is resolved twice through two different indexing schemes, unchecked
Nothing asserts that the resolved Add |
||
| index >= 0 && saveResultInternal(index, SaveResult()) && file.exists() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F02 - the
So a plugin that edits Hoist the |
||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| } 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,18 +1045,49 @@ 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 <T : Any?> performFileSave(crossinline action: suspend () -> T): T { | ||
| setFilesSaving(true) | ||
| beginFileSave() | ||
| try { | ||
| return action() | ||
| } finally { | ||
| setFilesSaving(false) | ||
| endFileSave() | ||
| } | ||
| } | ||
|
|
||
| private suspend fun setFilesSaving(saving: Boolean) { | ||
| withContext(Dispatchers.Main.immediate) { | ||
| editorViewModel.areFilesSaving = saving | ||
| /** | ||
| * 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 | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** Lowers the saving flag once the last in-flight save finishes. See [beginFileSave]. */ | ||
| @VisibleForTesting | ||
| internal suspend fun endFileSave() { | ||
| withContext(NonCancellable + Dispatchers.Main.immediate) { | ||
| if (activeSaveCount.decrementAndGet() == 0) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F04 - the counter has no floor, so one unbalanced decrement latches the flag on forever
This is newly reachable: The removed |
||
| editorViewModel.areFilesSaving = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F08 - this KDoc is factually wrong about where the write runs "the write itself runs there [on the main thread], so a blocking bridge would deadlock against it" - it does not. The deadlock is real, but it comes from the Note also that IdeServices.kt:185 makes the opposite claim ("the write is marshalled to the editor thread"). The two should agree; that one is the accurate half. |
||
| */ | ||
| override suspend fun saveFile(file: File): Boolean { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F06 - the only Every other method in this class goes through If a plugin calls The KDoc's suggested Either enforce the bound host-side ( |
||
| val activity = activity() ?: return false | ||
| return activity.saveFileResult(file) | ||
| } | ||
|
|
||
| // --- Buffer edits ------------------------------------------------------- | ||
|
|
||
| override fun insertTextAtCursor(text: String): Boolean = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /* | ||
| * 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 <https://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
||
| 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(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) | ||
| } | ||
| } | ||
|
|
||
| /** Blocks until the worker's main-thread hop is sitting in the paused looper's queue. */ | ||
| private fun awaitPostToMain(mainLooper: ShadowLooper) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F10 - It only asserts that The test then calls It passes today only because the queue happens to be empty. Synchronize on the queue actually growing - snapshot |
||
| 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 | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,10 +172,30 @@ 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 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. | ||
| */ | ||
| suspend fun saveFile(file: File): Boolean = false | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F09 - new public plugin API with no
Without a YY.WW row for |
||
|
|
||
| fun insertTextAtCursor(text: String): Boolean | ||
|
|
||
| fun replaceSelection(text: String): Boolean | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F01 -
activeSaveCountoutlives the flag it guardsThe counter is per-activity-instance, but the flag it guards (
editorViewModel.areFilesSaving) lives in the retainedEditorViewModel.EditorActivityKt's manifestconfigChanges(AndroidManifest.xml:101) covers onlyorientation|screenSize|screenLayout|smallestScreenSize|fontScale, so a dark-mode / locale / display-size change destroys and recreates the activity.saveAllAsyncruns its save underwithContext(NonCancellable), so save A keeps running against the old instance (its counter is 1). The new instance starts save B: its fresh counter goes 0 -> 1 and setsareFilesSaving = true. Save A then finishes on the old instance, its counter goes 1 -> 0, and it writesareFilesSaving = falseon the shared retained ViewModel while B is still writing.SaveFileActionre-enables mid-write - exactly the failure this PR was written to fix.The counter needs to live in
EditorViewModel, next to the flag it is paired with.