Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# 0013. Refactoring UI lives in the owning LSP module

- **Status:** Proposed
- **Status:** Accepted, revised 2026-09-05 (see Revision below)
- **Date:** 2026-08-03
- **Deciders:** Code On The Go team

Expand Down Expand Up @@ -44,6 +44,22 @@ So a refactoring in `lsp/kotlin` either renders its own UI, or a new inversion m
- **Render in `app`.** `app` is the integration point and already hosts `BottomSheetDialogFragment`s and `ILanguageClient`. Rejected: same inversion problem, and it puts Kotlin-specific refactoring UI in the module where nothing else language-specific lives.
- **A new `lsp/kotlin-ui` module.** Keeps Compose out of `lsp/kotlin` without inverting. Rejected for now: a new Gradle module in a ~80-module build is disproportionate for one sheet. Reconsider once extract-method and inline-variable have landed and the UI surface is known.

## Revision: a shared `:lsp:ui` for UI that serves two languages

- **Date:** 2026-09-05
- **Tickets:** ADFA-5047 (extract variable), ADFA-5048 (extract method)

The decision above anticipated its own revisit: *"If three or more `lsp/*` modules end up with Compose UI, extracting a shared UI module becomes worthwhile"*, and *"Reconsider once extract-method and inline-variable have landed and the UI surface is known"*. Both refactorings have now landed in both languages, and the trigger turned out to be duplication between two modules rather than a third one appearing.

**The rule is now:** a refactoring sheet used by **more than one** language server lives in **`:lsp:ui`**, behind a plain-data contract of strings, name sets and index positions. A sheet used by exactly one stays in that language's own module.

- `:lsp:ui` holds `ExtractVariableSheet` and `ExtractMethodSheet` with their `ViewModel`s, states, events and the shared `LabelledSection`/`OptionList`. Neither knows what a `KtExpression` or a javac `Tree` is: each caller maps its own plan into `CandidateView`/`MethodCandidateView` and maps a selection back out.
- Language-specific wording stays with the language. `NameMessages` and the keyword set are passed in, because two of the four name-problem strings name the language ("Not a valid Java name") and a shared lookup would show a Java user Kotlin's wording.
- The extract-method signature preview crosses the boundary as a **prefix and a suffix around the name**, not a rendered string: the preview follows what the user types, and each language keeps one derivation shared with its own edit builder.
- `lsp/kotlin` keeps `InlineVariableSheet`, which has no Java counterpart, under the original decision.

Everything else in this ADR stands unchanged: the plain-data plan boundary, the `BottomSheetDialogFragment` hosting a `ComposeView`, the `ContextWrapper` walk for a `FragmentActivity`, and ADR 0009's UDF shape. The costs listed above are unchanged too, except that Compose now sits in one shared module rather than being added to each language server in turn.

## Related

- [ADR 0009](0009-jetpack-compose-for-new-ui.md) — Compose for new UI; this ADR answers *where*, not *what*.
Expand Down
324 changes: 324 additions & 0 deletions docs/features/java-extract-method.md

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ object TooltipTag {
const val EDITOR_CODE_ACTIONS_ORGANIZE_IMPORTS = "editor.codeactions.organizeimports"
const val EDITOR_CODE_ACTIONS_TRY_CATCH = "editor.codeactions.trycatch"
const val EDITOR_CODE_ACTIONS_EXTRACT_VARIABLE = "editor.codeactions.extractvariable"
const val EDITOR_CODE_ACTIONS_EXTRACT_METHOD = "editor.codeactions.extractmethod"

// Kotlin code actions. Tags are per-language even where the action exists in both languages,
// so the tooltip can describe the Kotlin behaviour (see ADFA-4730).
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
package com.itsaky.androidide.lsp.java.actions

import android.content.Context
import com.itsaky.androidide.actions.ActionData
import com.itsaky.androidide.actions.requireContext
import com.itsaky.androidide.actions.requireEditor
import com.itsaky.androidide.actions.requireFile
import com.itsaky.androidide.idetooltips.TooltipTag
import com.itsaky.androidide.lsp.java.refactor.ExtractMethodPlan
import com.itsaky.androidide.lsp.java.refactor.ExtractionRefusal
import com.itsaky.androidide.lsp.java.refactor.JAVA_KEYWORDS
import com.itsaky.androidide.lsp.java.refactor.JAVA_NAME_MESSAGES
import com.itsaky.androidide.lsp.java.refactor.buildExtractMethodPlan
import com.itsaky.androidide.lsp.java.refactor.buildExtractMethodRewrites
import com.itsaky.androidide.lsp.java.refactor.candidateFor
import com.itsaky.androidide.lsp.java.refactor.toMethodCandidateViews
import com.itsaky.androidide.lsp.models.CodeActionItem
import com.itsaky.androidide.lsp.models.CodeActionKind
import com.itsaky.androidide.lsp.models.Command
import com.itsaky.androidide.lsp.models.DocumentChange
import com.itsaky.androidide.lsp.refactor.toTextEdit
import com.itsaky.androidide.lsp.ui.ExtractMethodSelection
import com.itsaky.androidide.lsp.ui.ExtractMethodSheet
import com.itsaky.androidide.lsp.ui.findFragmentActivity
import com.itsaky.androidide.projects.FileManager
import com.itsaky.androidide.resources.R
import com.itsaky.androidide.utils.flashError
import com.itsaky.androidide.utils.flashInfo
import org.slf4j.LoggerFactory
import java.nio.file.Path
import kotlin.coroutines.cancellation.CancellationException

/**
* Moves the expression at the cursor, or a selected range of statements, into a new `private` method.
*
* [execAction] runs one attributed compile and returns a plain-data [ExtractMethodPlan]; [postExec]
* shows the shared sheet and turns the user's selection into two text edits with pure offset
* arithmetic. Where the region cannot be moved faithfully the plan carries a typed refusal, which
* postExec renders as a specific message rather than a generic failure (ADR 0014).
*/
class ExtractMethodAction : BaseJavaCodeAction() {
companion object {
const val ID = "ide.editor.lsp.java.extractMethod"

private val log = LoggerFactory.getLogger(ExtractMethodAction::class.java)
}

override val titleTextRes: Int = R.string.action_extract_method
override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_EXTRACT_METHOD

override val id: String = ID
override var label: String = ""

// Deciding whether anything is extractable needs an attributed compile, far too costly for
// prepare() on the UI thread. BaseJavaCodeAction's file-type and module gate is all that applies;
// the action stays visible on any Java file and reports a refusal instead.
override var requiresUIThread: Boolean = false

override suspend fun execAction(data: ActionData): ExtractMethodPlan {
val file = data.requireFile().toPath()
val cursor = data.requireEditor().cursor
val selectionStart = minOf(cursor.left, cursor.right)
val selectionEnd = maxOf(cursor.left, cursor.right)
val version = documentVersionOf(file)

// Resolving the compiler and taking its lock can both throw, and neither is inside the planner's
// own guard. DefaultActionsRegistry catches only IllegalArgumentException and this runs on a scope
// with no exception handler, so anything else would crash the app rather than fail the action.
return runCatching {
data.requireCompiler().compile(file).get { task ->
buildExtractMethodPlan(task, file, selectionStart, selectionEnd, version)
}
}.getOrElse { error ->
if (error is CancellationException) throw error
log.warn("Could not analyse {} for extract method.", file, error)
ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse, documentVersion = version)
}
}

override fun postExec(
data: ActionData,
result: Any,
) {
super.postExec(data, result)
if (result !is ExtractMethodPlan) return

val context = data.requireContext()
if (result.isEmpty) {
flashInfo(refusalMessage(context, result.refusal ?: ExtractionRefusal.CouldNotAnalyse))
return
}

val activity =
context.findFragmentActivity()
?: run {
// A wiring problem rather than a user path: the editor is always hosted by one.
log.warn("No FragmentActivity for the editor context. Cannot show the extract sheet.")
flashError(R.string.msg_cannot_perform_fix)
return
}

val shown =
ExtractMethodSheet.show(
activity,
result.toMethodCandidateViews(),
JAVA_KEYWORDS,
JAVA_NAME_MESSAGES,
) { selection -> applySelection(data, result, selection) }
if (!shown) {
log.warn("Fragment manager unavailable. Cannot show the extract sheet.")
}
}

/**
* Turns the user's selection into the two edits and hands them to the language client.
*
* Runs from the sheet's click handler, outside `execAction` and so outside every guard the action
* framework provides -- nothing here may throw, hence the [runCatching].
*/
private fun applySelection(
data: ActionData,
plan: ExtractMethodPlan,
selection: ExtractMethodSelection,
) {
runCatching { performSelection(data, plan, selection) }.onFailure { error ->
log.error("Failed to apply the extract-method selection '{}'", selection.name, error)
flashError(R.string.msg_cannot_perform_fix)
}
}

/**
* The document version is re-read here rather than trusted from the plan: the editor stays reachable
* while the sheet is open, and applying spans computed against older text would corrupt the file.
* Refusing is always safe; the user can invoke the action again.
*/
private fun performSelection(
data: ActionData,
plan: ExtractMethodPlan,
selection: ExtractMethodSelection,
) {
val file = data.requireFile().toPath()
// A plan built while the document was closed carries no version to compare, so there is nothing
// to prove the text still matches: refuse rather than apply spans on trust.
if (plan.documentVersion == null || documentVersionOf(file) != plan.documentVersion) {
flashInfo(R.string.msg_extract_method_file_changed)
return
}

val candidate =
plan.candidateFor(selection) ?: run {
log.warn("Selection {} does not address the plan it came from.", selection)
flashError(R.string.msg_cannot_perform_fix)
return
}

val rewrites =
buildExtractMethodRewrites(plan.fileText, candidate, selection.name) ?: run {
log.warn("Could not build an extract-method rewrite for '{}'", candidate.label)
flashError(R.string.msg_cannot_perform_fix)
return
}

val client =
data.getLanguageClient() ?: run {
log.warn("No language client set. Cannot extract method.")
return
}

client.performCodeAction(
CodeActionItem(
title = label,
changes =
listOf(
DocumentChange(
file = file,
// Already in descending document order: applyActionEdits applies these in list
// order with line/column ranges, so the call site must not shift the insertion point.
edits = rewrites.map { it.toTextEdit(plan.fileText) },
),
),
kind = CodeActionKind.QuickFix,
// The rewrites are emitted fully indented. Running google-java-format here would reformat
// the whole file into the same undo step as the extraction.
command = Command("", ""),
),
)
}

/**
* Each refusal names the construct in the way; a generic message reads as a broken feature.
*
* Exhaustive with no `else`: a future variant added without a message here is a compile error rather
* than a silent gap.
*/
private fun refusalMessage(
context: Context,
refusal: ExtractionRefusal,
): String =
when (refusal) {
ExtractionRefusal.NotASingleRegion -> {
context.getString(R.string.msg_extract_method_not_single_region)
}

ExtractionRefusal.CouldNotAnalyse -> {
context.getString(R.string.msg_extract_method_could_not_analyse)
}

is ExtractionRefusal.MultipleOutputs -> {
context.getString(R.string.msg_extract_method_multiple_outputs, refusal.names.joinToString(", "))
}

is ExtractionRefusal.ReassignsOuterVar -> {
context.getString(R.string.msg_extract_method_reassigns_outer_var, refusal.name)
}

ExtractionRefusal.ExitsRegion -> {
context.getString(R.string.msg_extract_method_exits_region)
}

is ExtractionRefusal.UsesTypeParameter -> {
context.getString(R.string.msg_extract_method_uses_type_parameter, refusal.name)
}

ExtractionRefusal.UnrenderableType -> {
context.getString(R.string.msg_extract_method_unrenderable_type)
}

is ExtractionRefusal.CapturedLocalDeclaration -> {
context.getString(R.string.msg_extract_method_captured_local_declaration, refusal.name)
}
}

/** Null when the document is not open, which the confirm guard treats as unverifiable and refuses. */
private fun documentVersionOf(path: Path): Int? = FileManager.getActiveDocument(path)?.version
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,6 @@ object JavaCodeActionsMenu : IActionsMenuProvider {
TooltipTag.EDITOR_CODE_ACTIONS_TRY_CATCH,
),
ExtractVariableAction(),
ExtractMethodAction(),
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package com.itsaky.androidide.lsp.java.refactor

import com.itsaky.androidide.lsp.refactor.RewriteSpan
import com.itsaky.androidide.lsp.refactor.TextSpan
import com.itsaky.androidide.lsp.refactor.detectIndentUnit
import com.itsaky.androidide.lsp.refactor.detectNewline
import com.itsaky.androidide.lsp.refactor.leadingIndentAt

/**
* The two replacements an extraction performs: the new method, and the call that replaces the region.
*
* **Descending document order is mandatory, not stylistic.** `IDELanguageClientImpl.applyActionEdits`
* iterates the list and applies each edit with line/column ranges against whatever the text is at that
* moment, so an earlier edit must never shift a later one. In Java the new method always leads: the
* anchor member *contains* the region, so its end is always past the region's.
*
* Nothing on that path calls `beginBatchEdit`, so this costs the user **two** undo steps and the
* intermediate state does not compile. ADFA-5081 fixes that by batching the edit loop; until it lands
* the two-step undo is a stated limitation.
*
* Returns null when the offsets cannot be honoured, which the caller reports rather than applying.
*/
fun buildExtractMethodRewrites(
fileText: String,
candidate: ExtractMethodCandidate,
name: String,
): List<RewriteSpan>? {
val span = candidate.span
if (span.end > fileText.length) return null
if (candidate.insertOffset > fileText.length) return null
// The anchor member contains the region, so anything else means the plan and the text disagree.
if (candidate.insertOffset < span.end) return null

val newline = detectNewline(fileText)
val indent = candidate.insertIndent
val bodyIndent = indent + detectIndentUnit(fileText)
val regionText = fileText.substring(span.start, span.end)
val baseIndent = leadingIndentAt(fileText, span.start)

val lines = indentedBodyLines(regionText, span.start, baseIndent, bodyIndent, newline, candidate.textBlockSpans)
val bodyLines =
when (val body = candidate.body) {
is ExtractedBody.ExpressionBody -> {
// An expression carries no `;` of its own -- the source one sits outside its span -- so the
// statement it becomes gets one here.
val returned =
if (body.needsReturn) {
// The first line is never inside a text block's interior -- the region starts at the
// code itself -- so it always carries bodyIndent and `return ` goes straight after it.
listOf(bodyIndent + "return " + lines.first().substring(bodyIndent.length)) + lines.drop(1)
} else {
lines
}
returned.dropLast(1) + (returned.last() + ";")
}

is ExtractedBody.StatementBody -> {
lines + listOfNotNull(body.trailingReturn?.let { bodyIndent + it })
}
}

val declaration =
buildString {
append(indent).append(candidate.signatureText(name)).append(" {").append(newline)
bodyLines.forEach { append(it).append(newline) }
append(indent).append('}')
}

val call = "$name(${candidate.parameters.joinToString(", ") { it.name }})"
val callText =
when (val form = candidate.callSite) {
CallSiteForm.Call -> call
CallSiteForm.CallStatement -> "$call;"
is CallSiteForm.AssignOutput -> "${form.typeText} ${form.name} = $call;"
CallSiteForm.Return -> "return $call;"
}

return listOf(
RewriteSpan(TextSpan(candidate.insertOffset, candidate.insertOffset), newline + newline + declaration),
RewriteSpan(span, callText),
).sortedByDescending { it.span.start }
}

/**
* The region's lines at the new method's body indentation: the original base indentation removed and
* [bodyIndent] put in its place. Lines nested deeper than the base keep the extra depth; the first line
* only gains the indent, since the span starts at the code itself.
*
* A line inside one of [protectedSpans] is emitted byte-for-byte. Those are text block literals, whose
* interior whitespace is part of their value and whose closing delimiter sets the incidental-whitespace
* margin, so moving either edits the interior of the moved code (ADR 0014).
*/
private fun indentedBodyLines(
regionText: String,
regionStart: Int,
baseIndent: String,
bodyIndent: String,
newline: String,
protectedSpans: List<TextSpan>,
): List<String> {
var offset = regionStart
return regionText.split(newline).mapIndexed { index, line ->
val lineStart = offset
offset += line.length + newline.length
when {
index == 0 -> bodyIndent + line
protectedSpans.any { lineStart > it.start && lineStart < it.end } -> line
else -> bodyIndent + line.removePrefix(baseIndent)
}
}
}
Loading
Loading