diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt index aabd53f6e4..bb2f65644a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt @@ -9,10 +9,13 @@ import com.itsaky.androidide.actions.requireFile import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractMethodChoice -import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractMethodSheet +import com.itsaky.androidide.lsp.kotlin.refactor.KOTLIN_NAME_MESSAGES +import com.itsaky.androidide.lsp.kotlin.refactor.candidateFor +import com.itsaky.androidide.lsp.kotlin.refactor.toMethodCandidateViews +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionRefusal +import com.itsaky.androidide.lsp.kotlin.utils.refactor.HARD_KEYWORDS import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodPlan import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodRewrites import com.itsaky.androidide.lsp.models.CodeActionItem @@ -20,6 +23,8 @@ 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 @@ -99,7 +104,13 @@ class ExtractMethodAction : BaseKotlinCodeAction() { return } - val shown = ExtractMethodSheet.show(activity, result) { choice -> applyChoice(data, result, choice) } + val shown = + ExtractMethodSheet.show( + activity, + result.toMethodCandidateViews(), + HARD_KEYWORDS, + KOTLIN_NAME_MESSAGES, + ) { selection -> applySelection(data, result, selection) } if (!shown) { logger.warn("Fragment manager unavailable. Cannot show the extract sheet.") } @@ -115,21 +126,21 @@ class ExtractMethodAction : BaseKotlinCodeAction() { * Runs from the sheet's click handler, outside `execAction` and so outside every guard the action * framework provides -- nothing here may throw (R16), hence the [runCatching]. */ - private fun applyChoice( + private fun applySelection( data: ActionData, plan: ExtractMethodPlan, - choice: ExtractMethodChoice, + selection: ExtractMethodSelection, ) { - runCatching { performChoice(data, plan, choice) }.onFailure { error -> - logger.error("Failed to apply the extract-method choice '{}'", choice.name, error) + runCatching { performSelection(data, plan, selection) }.onFailure { error -> + logger.error("Failed to apply the extract-method selection '{}'", selection.name, error) flashError(R.string.msg_cannot_perform_fix) } } - private fun performChoice( + private fun performSelection( data: ActionData, plan: ExtractMethodPlan, - choice: ExtractMethodChoice, + selection: ExtractMethodSelection, ) { val file = data.requireFile() val nioPath = file.toPath() @@ -138,9 +149,16 @@ class ExtractMethodAction : BaseKotlinCodeAction() { return } + val candidate: ExtractMethodCandidate = + plan.candidateFor(selection) ?: run { + logger.warn("Selection {} does not address the plan it came from.", selection) + flashError(R.string.msg_cannot_perform_fix) + return + } + val rewrites = - buildExtractMethodRewrites(plan.fileText, choice.candidate, choice.name) ?: run { - logger.warn("Could not build an extract-method rewrite for '{}'", choice.candidate.label) + buildExtractMethodRewrites(plan.fileText, candidate, selection.name) ?: run { + logger.warn("Could not build an extract-method rewrite for '{}'", candidate.label) flashError(R.string.msg_cannot_perform_fix) return } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/KotlinExtractMethodUi.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/KotlinExtractMethodUi.kt new file mode 100644 index 0000000000..a2ad8b1723 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/KotlinExtractMethodUi.kt @@ -0,0 +1,35 @@ +package com.itsaky.androidide.lsp.kotlin.refactor + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.signaturePrefix +import com.itsaky.androidide.lsp.kotlin.utils.refactor.signatureSuffix +import com.itsaky.androidide.lsp.ui.ExtractMethodSelection +import com.itsaky.androidide.lsp.ui.MethodCandidateView + +/** + * The plan as the shared sheet sees it: labels, names and the two halves of the signature, no PSI and + * no offsets. + * + * Offsets stay on this side deliberately -- the sheet is a chooser, and resolving a selection back into + * a candidate is [candidateFor]'s job. + */ +fun ExtractMethodPlan.toMethodCandidateViews(): List = + candidates.map { candidate -> + MethodCandidateView( + label = candidate.label, + suggestedName = candidate.suggestedName, + takenNames = candidate.takenNames, + signaturePrefix = candidate.signaturePrefix, + signatureSuffix = candidate.signatureSuffix, + ) + } + +/** + * Resolves a selection's index back to the plan it came from, or null when it does not address it. + * + * A null is a wiring bug rather than a user path -- the sheet only ever reports an index it was given -- + * so the caller reports it as a failed quick fix rather than guessing at a candidate. + */ +fun ExtractMethodPlan.candidateFor(selection: ExtractMethodSelection): ExtractMethodCandidate? = + candidates.getOrNull(selection.candidateIndex) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt deleted file mode 100644 index ef72fe6a78..0000000000 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt +++ /dev/null @@ -1,96 +0,0 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui - -import android.os.Bundle -import android.view.LayoutInflater -import android.view.View -import android.view.ViewGroup -import androidx.compose.runtime.getValue -import androidx.compose.ui.platform.ComposeView -import androidx.compose.ui.platform.ViewCompositionStrategy -import androidx.fragment.app.FragmentActivity -import androidx.fragment.app.viewModels -import androidx.lifecycle.compose.collectAsStateWithLifecycle -import com.google.android.material.bottomsheet.BottomSheetDialogFragment -import com.itsaky.androidide.common.compose.IdeTheme -import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan - -/** - * Hosts [ExtractMethodSheetContent]. - * - * The plan is handed in directly rather than through fragment arguments: it carries the file's text - * and offset spans, which is neither `Parcelable` nor meaningful to restore -- after process death - * the document may be entirely different. So [plan] is null on a recreated instance and the sheet - * dismisses itself, the same outcome the action's document-version guard would reach anyway. - */ -class ExtractMethodSheet : BottomSheetDialogFragment() { - private var plan: ExtractMethodPlan? = null - private var onChoice: ((ExtractMethodChoice) -> Unit)? = null - - private val viewModel: ExtractMethodViewModel by viewModels { - ExtractMethodViewModel.factory(requireNotNull(plan) { "sheet shown without a plan" }) - } - - override fun onCreateView( - inflater: LayoutInflater, - container: ViewGroup?, - savedInstanceState: Bundle?, - ): View? { - if (plan == null) { - dismissAllowingStateLoss() - return null - } - - return ComposeView(requireContext()).apply { - setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) - setContent { - IdeTheme { - val state by viewModel.uiState.collectAsStateWithLifecycle() - ExtractMethodSheetContent( - state = state, - onEvent = ::handleEvent, - ) - } - } - } - } - - private fun handleEvent(event: ExtractMethodUiEvent) { - when (event) { - ExtractMethodUiEvent.Confirmed -> { - viewModel.choice()?.let { choice -> onChoice?.invoke(choice) } - dismiss() - } - - ExtractMethodUiEvent.Dismissed -> { - dismiss() - } - - else -> { - viewModel.onEvent(event) - } - } - } - - companion object { - private const val TAG = "extract_method_sheet" - - /** - * Shows the sheet on [activity], calling [onChoice] once if the user confirms. Returns false - * when it could not be shown, so the caller can report a failure rather than doing nothing. - */ - fun show( - activity: FragmentActivity, - plan: ExtractMethodPlan, - onChoice: (ExtractMethodChoice) -> Unit, - ): Boolean { - val manager = activity.supportFragmentManager - if (manager.isStateSaved || manager.isDestroyed) return false - ExtractMethodSheet() - .apply { - this.plan = plan - this.onChoice = onChoice - }.show(manager, TAG) - return true - } - } -} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index 2be457b1ce..c97877555c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -183,18 +183,31 @@ data class ExtractMethodPlan( } /** - * The signature exactly as [buildExtractMethodRewrites] emits it. The sheet's preview calls this, so - * there is one derivation and the preview cannot drift from the declaration (R11). + * Everything the signature says before the method's name. + * + * Split from [signatureSuffix] rather than rendered whole because the sheet's preview follows what the + * user types, and [MethodCandidateView] carries the two halves. Both this and + * [buildExtractMethodRewrites] compose them through [signatureText], so there is one derivation and + * the preview cannot drift from the declaration (R11). */ -fun ExtractMethodCandidate.signatureText(name: String): String = - buildString { - annotations.forEach { append(it).append(' ') } - modifiers.forEach { append(it).append(' ') } - append("fun ") - receiverTypeText?.let { append(it).append('.') } - append(name) - append('(') - append(parameters.joinToString(", ") { "${it.name}: ${it.typeText}" }) - append(')') - returnTypeText?.let { append(": ").append(it) } - } +val ExtractMethodCandidate.signaturePrefix: String + get() = + buildString { + annotations.forEach { append(it).append(' ') } + modifiers.forEach { append(it).append(' ') } + append("fun ") + receiverTypeText?.let { append(it).append('.') } + } + +/** Everything the signature says after the method's name: parameters and return type. */ +val ExtractMethodCandidate.signatureSuffix: String + get() = + buildString { + append('(') + append(parameters.joinToString(", ") { "${it.name}: ${it.typeText}" }) + append(')') + returnTypeText?.let { append(": ").append(it) } + } + +/** The signature exactly as [buildExtractMethodRewrites] emits it. */ +fun ExtractMethodCandidate.signatureText(name: String): String = signaturePrefix + name + signatureSuffix diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt deleted file mode 100644 index fa0de3d7b6..0000000000 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt +++ /dev/null @@ -1,165 +0,0 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui - -import com.itsaky.androidide.lsp.kotlin.utils.refactor.CallSiteForm -import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate -import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan -import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractedBody -import com.itsaky.androidide.lsp.kotlin.utils.refactor.MethodParameter -import com.itsaky.androidide.lsp.refactor.TextSpan -import com.itsaky.androidide.lsp.ui.NameProblem -import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse -import org.junit.Assert.assertNotNull -import org.junit.Assert.assertNull -import org.junit.Assert.assertTrue -import org.junit.Test - -/** The sheet's derivation logic, tested without Compose, a fragment or an activity. */ -class ExtractMethodViewModelTest { - private fun candidate( - label: String, - suggestedName: String, - parameters: List = listOf(MethodParameter("a", "Int")), - returnTypeText: String? = "Int", - modifiers: List = listOf("private"), - takenNames: Set = emptySet(), - ) = ExtractMethodCandidate( - label = label, - span = TextSpan(0, 5), - suggestedName = suggestedName, - takenNames = takenNames, - annotations = emptyList(), - modifiers = modifiers, - receiverTypeText = null, - parameters = parameters, - returnTypeText = returnTypeText, - body = ExtractedBody.ExpressionBody(needsReturn = true), - callSite = CallSiteForm.Call, - insertOffset = 100, - insertIndent = "\t", - rawStringSpans = emptyList(), - ) - - private fun plan(candidates: List) = - ExtractMethodPlan( - fileText = "unused", - documentVersion = 1, - candidates = candidates, - refusal = null, - ) - - @Test - fun `the initial state takes the first candidate's suggestion`() { - val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) - - assertEquals("total", model.uiState.value.name) - assertEquals(0, model.uiState.value.selectedCandidate) - assertNull(model.uiState.value.nameProblem) - } - - @Test - fun `the chooser is hidden for one candidate and shown for more`() { - val single = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) - assertFalse(single.uiState.value.showCandidatePicker) - - val many = listOf(candidate("a + b", "total"), candidate("a + b + c", "total1")) - assertTrue(ExtractMethodViewModel(plan(many)).uiState.value.showCandidatePicker) - } - - @Test - fun `the preview is the signature as it will be emitted`() { - val model = - ExtractMethodViewModel( - plan( - listOf( - candidate( - "load() + 1", - "total", - parameters = listOf(MethodParameter("id", "String")), - returnTypeText = "User", - modifiers = listOf("private", "suspend"), - ), - ), - ), - ) - - assertEquals("private suspend fun total(id: String): User", model.uiState.value.signaturePreview) - - model.onEvent(ExtractMethodUiEvent.NameChanged("loadUser")) - - assertEquals("private suspend fun loadUser(id: String): User", model.uiState.value.signaturePreview) - } - - @Test - fun `a name matching an inherited member is rejected`() { - val model = - ExtractMethodViewModel(plan(listOf(candidate("a + b", "total", takenNames = setOf("helper"))))) - - model.onEvent(ExtractMethodUiEvent.NameChanged("helper")) - - assertEquals(NameProblem.AlreadyTaken, model.uiState.value.nameProblem) - assertFalse(model.uiState.value.canConfirm) - assertNull(model.choice()) - } - - @Test - fun `switching candidate re-suggests the name`() { - val model = - ExtractMethodViewModel( - plan(listOf(candidate("a + b", "total"), candidate("a + b + c", "sum"))), - ) - model.onEvent(ExtractMethodUiEvent.NameChanged("mine")) - - model.onEvent(ExtractMethodUiEvent.CandidateSelected(1)) - - assertEquals("sum", model.uiState.value.name) - assertEquals(1, model.uiState.value.selectedCandidate) - } - - @Test - fun `the choice carries the selected candidate and the typed name`() { - val model = - ExtractMethodViewModel( - plan(listOf(candidate("a + b", "total"), candidate("a + b + c", "sum"))), - ) - model.onEvent(ExtractMethodUiEvent.CandidateSelected(1)) - model.onEvent(ExtractMethodUiEvent.NameChanged("combined")) - - val choice = model.choice() - - assertNotNull(choice) - assertEquals("a + b + c", choice!!.candidate.label) - assertEquals("combined", choice.name) - } - - @Test - fun `a blank name blocks confirmation`() { - val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) - - model.onEvent(ExtractMethodUiEvent.NameChanged("")) - - assertEquals(NameProblem.Blank, model.uiState.value.nameProblem) - assertNull(model.choice()) - } - - @Test - fun `a hard keyword blocks confirmation`() { - val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) - - model.onEvent(ExtractMethodUiEvent.NameChanged("when")) - - assertEquals(NameProblem.Keyword, model.uiState.value.nameProblem) - assertFalse(model.uiState.value.canConfirm) - assertNull(model.choice()) - } - - @Test - fun `a name that only looks like a keyword is accepted`() { - val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) - - model.onEvent(ExtractMethodUiEvent.NameChanged("whenever")) - - assertNull(model.uiState.value.nameProblem) - assertTrue(model.uiState.value.canConfirm) - } -} diff --git a/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodContract.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodContract.kt new file mode 100644 index 0000000000..87a6137c54 --- /dev/null +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodContract.kt @@ -0,0 +1,40 @@ +package com.itsaky.androidide.lsp.ui + +/** + * What the extract-method sheet needs to know about one extractable region. + * + * Deliberately a *view* of a language's plan rather than the plan itself: strings, name sets and index + * positions only. That is what lets one sheet serve both language servers without either depending on + * the other, and without this module knowing what a `KtExpression` or a `Tree` is. Each caller maps its + * own plan into these and maps an [ExtractMethodSelection] back out. + * + * The signature arrives **split around the name** rather than pre-rendered, because the preview has to + * follow what the user types. Java composes `private static int ` + name + `(int a, int b) throws + * IOException`; Kotlin composes `private suspend fun ` + name + `(id: String): User`. Each language + * keeps one derivation, shared by its edit builder and this preview, so the two cannot drift. + * + * [takenNames] is what a method declared at the insertion point would collide with, used to reject a + * typed name. + */ +data class MethodCandidateView( + val label: String, + val suggestedName: String, + val takenNames: Set, + val signaturePrefix: String, + val signatureSuffix: String, +) { + /** The signature exactly as the emitted declaration will read, for [name]. */ + fun signatureFor(name: String): String = signaturePrefix + name + signatureSuffix +} + +/** + * The user's finished decision. + * + * Positional rather than resolved: the caller knows which candidate the index names, and turning it + * back into offsets and edits is its job. Keeping the sheet free of offsets is what makes it a pure + * chooser. + */ +data class ExtractMethodSelection( + val candidateIndex: Int, + val name: String, +) diff --git a/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodSheet.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodSheet.kt new file mode 100644 index 0000000000..c6f3435ef9 --- /dev/null +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodSheet.kt @@ -0,0 +1,111 @@ +package com.itsaky.androidide.lsp.ui + +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.compose.runtime.getValue +import androidx.compose.ui.platform.ComposeView +import androidx.compose.ui.platform.ViewCompositionStrategy +import androidx.fragment.app.FragmentActivity +import androidx.fragment.app.viewModels +import androidx.lifecycle.compose.collectAsStateWithLifecycle +import com.google.android.material.bottomsheet.BottomSheetDialogFragment +import com.itsaky.androidide.common.compose.IdeTheme + +/** + * Hosts [ExtractMethodSheetContent], for whichever language server showed it. + * + * The candidates are handed in directly rather than through fragment arguments: they are a view of an + * analysis result whose offsets refer to one snapshot of one document, which is neither `Parcelable` + * nor meaningful to restore -- after process death the document may be entirely different. So + * [candidates] is null on a recreated instance and the sheet dismisses itself, which is the same + * outcome the caller's document-version guard would reach anyway. + */ +class ExtractMethodSheet : BottomSheetDialogFragment() { + private var candidates: List? = null + private var keywords: Set = emptySet() + private var nameMessages: NameMessages? = null + private var onSelected: ((ExtractMethodSelection) -> Unit)? = null + + private val viewModel: ExtractMethodViewModel by viewModels { + ExtractMethodViewModel.factory( + requireNotNull(candidates) { "sheet shown without candidates" }, + keywords, + ) + } + + override fun onCreateView( + inflater: LayoutInflater, + container: ViewGroup?, + savedInstanceState: Bundle?, + ): View? { + val messages = nameMessages + if (candidates == null || messages == null) { + dismissAllowingStateLoss() + return null + } + + return ComposeView(requireContext()).apply { + // The sheet's window is torn down with the fragment's view, so dispose with it. + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) + setContent { + IdeTheme { + val state by viewModel.uiState.collectAsStateWithLifecycle() + ExtractMethodSheetContent( + state = state, + nameMessages = messages, + onEvent = ::handleEvent, + ) + } + } + } + } + + private fun handleEvent(event: ExtractMethodUiEvent) { + when (event) { + ExtractMethodUiEvent.Confirmed -> { + viewModel.selection()?.let { selection -> onSelected?.invoke(selection) } + dismiss() + } + + ExtractMethodUiEvent.Dismissed -> { + dismiss() + } + + else -> { + viewModel.onEvent(event) + } + } + } + + companion object { + private const val TAG = "extract_method_sheet" + + /** + * Shows the sheet on [activity], calling [onSelected] once if the user confirms. + * + * [keywords] is the language's reserved-word set and [nameMessages] its name-problem strings, so + * a Java user is never shown Kotlin's wording. Returns false when the sheet could not be shown, + * so the caller can report a failure rather than silently doing nothing. + */ + fun show( + activity: FragmentActivity, + candidates: List, + keywords: Set, + nameMessages: NameMessages, + onSelected: (ExtractMethodSelection) -> Unit, + ): Boolean { + val manager = activity.supportFragmentManager + if (manager.isStateSaved || manager.isDestroyed) return false + ExtractMethodSheet() + .apply { + this.candidates = candidates + this.keywords = keywords + this.nameMessages = nameMessages + this.onSelected = onSelected + }.show(manager, TAG) + return true + } + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodSheetContent.kt similarity index 86% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodSheetContent.kt index 485e9cdaef..5a226b3084 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodSheetContent.kt @@ -1,4 +1,4 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui +package com.itsaky.androidide.lsp.ui import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column @@ -18,23 +18,23 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.unit.dp -import com.itsaky.androidide.lsp.kotlin.refactor.KOTLIN_NAME_MESSAGES -import com.itsaky.androidide.lsp.ui.LabelledSection -import com.itsaky.androidide.lsp.ui.OptionList import com.itsaky.androidide.resources.R /** - * The extract-method sheet: the expression chooser (when there is a choice), the name, and the - * signature exactly as it will be emitted. + * The extract-method sheet: the region chooser (when there is a choice), the name, and the signature + * exactly as it will be emitted. * * A sibling of the extract-variable sheet rather than a generalisation of it: a single shared sheet - * would need a state class where half the fields are meaningless to either caller (ADR 0013). + * would need a state class where half the fields are meaningless to either caller. * * Stateless: all state arrives in [state] and every interaction leaves as an [ExtractMethodUiEvent]. + * [nameMessages] is the calling language's wording, since two of the four name problems name the + * language. */ @Composable fun ExtractMethodSheetContent( state: ExtractMethodUiState, + nameMessages: NameMessages, onEvent: (ExtractMethodUiEvent) -> Unit, modifier: Modifier = Modifier, ) { @@ -71,7 +71,7 @@ fun ExtractMethodSheetContent( singleLine = true, supportingText = state.nameProblem?.let { problem -> - { Text(stringResource(KOTLIN_NAME_MESSAGES.resFor(problem))) } + { Text(stringResource(nameMessages.resFor(problem))) } }, modifier = Modifier.fillMaxWidth(), ) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodUiState.kt similarity index 67% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodUiState.kt index 80f4c916d4..9f39e77088 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodUiState.kt @@ -1,12 +1,9 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui - -import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate -import com.itsaky.androidide.lsp.ui.NameProblem +package com.itsaky.androidide.lsp.ui /** * Everything the extract-method sheet renders. * - * There is no scope chooser (the new function is always a sibling of the enclosing declaration) and + * There is no scope chooser (the new method is always a sibling of the region's own declaration) and * no replace-all checkbox (the region is the only site rewritten), so the sheet is a chooser, a name * field and a preview. * @@ -39,12 +36,3 @@ sealed interface ExtractMethodUiEvent { data object Dismissed : ExtractMethodUiEvent } - -/** - * The user's finished decision, handed to the action to turn into edits. Free of offsets and text so - * the sheet stays a pure chooser. - */ -data class ExtractMethodChoice( - val candidate: ExtractMethodCandidate, - val name: String, -) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodViewModel.kt similarity index 54% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodViewModel.kt index b0969c46d8..feed4ec55d 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractMethodViewModel.kt @@ -1,24 +1,22 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui +package com.itsaky.androidide.lsp.ui import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelProvider -import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan -import com.itsaky.androidide.lsp.kotlin.utils.refactor.HARD_KEYWORDS -import com.itsaky.androidide.lsp.kotlin.utils.refactor.signatureText -import com.itsaky.androidide.lsp.ui.validateVariableName import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow /** - * Derives the sheet's state from an [ExtractMethodPlan] and nothing else -- no analysis, no PSI, no - * I/O -- which is what lets it hold all the sheet's logic and still be a plain unit test. + * Derives the sheet's state from the [MethodCandidateView]s it was given and nothing else -- no + * analysis, no syntax tree, no I/O -- which is what lets it hold all the sheet's logic and still be a + * plain unit test, and what lets both language servers share it without either depending on the other. * * A plain [ViewModelProvider.Factory] rather than a Koin definition, for the same reason as - * `ExtractVariableViewModel`: sheet-scoped, injects nothing, takes the plan as a runtime argument. + * [ExtractVariableViewModel]: sheet-scoped, injects nothing, takes its inputs as runtime arguments. */ class ExtractMethodViewModel( - private val plan: ExtractMethodPlan, + private val candidates: List, + private val keywords: Set, ) : ViewModel() { private val _uiState = MutableStateFlow(stateFor(candidateIndex = 0, name = null)) val uiState: StateFlow = _uiState.asStateFlow() @@ -28,8 +26,8 @@ class ExtractMethodViewModel( when (event) { is ExtractMethodUiEvent.CandidateSelected -> { if (event.index == current.selectedCandidate) return - // A different expression means a different signature and suggested name, so the name is - // re-suggested rather than carried over -- the old one described the old expression. + // A different region means a different signature and suggested name, so the name is + // re-suggested rather than carried over -- the old one described the old region. _uiState.value = stateFor(event.index, name = null) } @@ -44,38 +42,42 @@ class ExtractMethodViewModel( } /** The user's decision, or null when the name is unusable. */ - fun choice(): ExtractMethodChoice? { + fun selection(): ExtractMethodSelection? { val state = _uiState.value if (!state.canConfirm) return null - return ExtractMethodChoice(candidate(state.selectedCandidate), state.name) + return ExtractMethodSelection(state.selectedCandidate, state.name) } - private fun candidate(index: Int) = plan.candidates[index.coerceIn(plan.candidates.indices)] + private fun candidate(index: Int) = candidates[index.coerceIn(candidates.indices)] private fun stateFor( candidateIndex: Int, name: String?, ): ExtractMethodUiState { - val bounded = candidateIndex.coerceIn(plan.candidates.indices) + val bounded = candidateIndex.coerceIn(candidates.indices) val candidate = candidate(bounded) val resolvedName = name ?: candidate.suggestedName return ExtractMethodUiState( - candidateLabels = plan.candidates.map { it.label }, + candidateLabels = candidates.map { it.label }, selectedCandidate = bounded, - showCandidatePicker = plan.candidates.size > 1, + showCandidatePicker = candidates.size > 1, name = resolvedName, - nameProblem = validateVariableName(resolvedName, candidate.takenNames, HARD_KEYWORDS), - // The same call the edit builder makes, so the preview cannot drift from the declaration. - signaturePreview = candidate.signatureText(resolvedName), + nameProblem = validateVariableName(resolvedName, candidate.takenNames, keywords), + // The same composition the edit builder makes, so the preview cannot drift from the + // declaration. + signaturePreview = candidate.signatureFor(resolvedName), ) } companion object { - fun factory(plan: ExtractMethodPlan): ViewModelProvider.Factory = + fun factory( + candidates: List, + keywords: Set, + ): ViewModelProvider.Factory = object : ViewModelProvider.Factory { @Suppress("UNCHECKED_CAST") - override fun create(modelClass: Class): T = ExtractMethodViewModel(plan) as T + override fun create(modelClass: Class): T = ExtractMethodViewModel(candidates, keywords) as T } } } diff --git a/lsp/ui/src/test/java/com/itsaky/androidide/lsp/ui/ExtractMethodViewModelTest.kt b/lsp/ui/src/test/java/com/itsaky/androidide/lsp/ui/ExtractMethodViewModelTest.kt new file mode 100644 index 0000000000..332cad7c01 --- /dev/null +++ b/lsp/ui/src/test/java/com/itsaky/androidide/lsp/ui/ExtractMethodViewModelTest.kt @@ -0,0 +1,170 @@ +package com.itsaky.androidide.lsp.ui + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The sheet's derivation logic, tested without Compose, a fragment or an activity. + * + * The keyword set is passed in rather than looked up, so both languages' shapes are exercised here: + * Kotlin's `fun name(a: Int): Int` and Java's `int name(int a) throws IOException`. + */ +class ExtractMethodViewModelTest { + private val kotlinKeywords = setOf("when", "val", "fun") + private val javaKeywords = setOf("int", "static", "class") + + private fun candidate( + label: String, + suggestedName: String, + signaturePrefix: String = "private fun ", + signatureSuffix: String = "(a: Int): Int", + takenNames: Set = emptySet(), + ) = MethodCandidateView( + label = label, + suggestedName = suggestedName, + takenNames = takenNames, + signaturePrefix = signaturePrefix, + signatureSuffix = signatureSuffix, + ) + + private fun model( + candidates: List, + keywords: Set = kotlinKeywords, + ) = ExtractMethodViewModel(candidates, keywords) + + @Test + fun `the initial state takes the first candidate's suggestion`() { + val model = model(listOf(candidate("a + b", "total"))) + + assertEquals("total", model.uiState.value.name) + assertEquals(0, model.uiState.value.selectedCandidate) + assertNull(model.uiState.value.nameProblem) + } + + @Test + fun `the chooser is hidden for one candidate and shown for more`() { + val single = model(listOf(candidate("a + b", "total"))) + assertFalse(single.uiState.value.showCandidatePicker) + + val many = listOf(candidate("a + b", "total"), candidate("a + b + c", "total1")) + assertTrue(model(many).uiState.value.showCandidatePicker) + } + + @Test + fun `the preview is the Kotlin signature as it will be emitted`() { + val model = + model( + listOf( + candidate( + "load() + 1", + "total", + signaturePrefix = "private suspend fun ", + signatureSuffix = "(id: String): User", + ), + ), + ) + + assertEquals("private suspend fun total(id: String): User", model.uiState.value.signaturePreview) + + model.onEvent(ExtractMethodUiEvent.NameChanged("loadUser")) + + assertEquals("private suspend fun loadUser(id: String): User", model.uiState.value.signaturePreview) + } + + @Test + fun `the preview is the Java signature as it will be emitted`() { + val model = + model( + listOf( + candidate( + "read(path)", + "read", + signaturePrefix = "private static int ", + signatureSuffix = "(int a, int b) throws IOException", + ), + ), + javaKeywords, + ) + + assertEquals("private static int read(int a, int b) throws IOException", model.uiState.value.signaturePreview) + + model.onEvent(ExtractMethodUiEvent.NameChanged("readTotal")) + + assertEquals( + "private static int readTotal(int a, int b) throws IOException", + model.uiState.value.signaturePreview, + ) + } + + @Test + fun `a name matching an inherited member is rejected`() { + val model = model(listOf(candidate("a + b", "total", takenNames = setOf("helper")))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("helper")) + + assertEquals(NameProblem.AlreadyTaken, model.uiState.value.nameProblem) + assertFalse(model.uiState.value.canConfirm) + assertNull(model.selection()) + } + + @Test + fun `switching candidate re-suggests the name`() { + val model = model(listOf(candidate("a + b", "total"), candidate("a + b + c", "sum"))) + model.onEvent(ExtractMethodUiEvent.NameChanged("mine")) + + model.onEvent(ExtractMethodUiEvent.CandidateSelected(1)) + + assertEquals("sum", model.uiState.value.name) + assertEquals(1, model.uiState.value.selectedCandidate) + } + + @Test + fun `the selection carries the selected candidate and the typed name`() { + val model = model(listOf(candidate("a + b", "total"), candidate("a + b + c", "sum"))) + model.onEvent(ExtractMethodUiEvent.CandidateSelected(1)) + model.onEvent(ExtractMethodUiEvent.NameChanged("combined")) + + val selection = model.selection() + + assertNotNull(selection) + assertEquals(1, selection!!.candidateIndex) + assertEquals("combined", selection.name) + } + + @Test + fun `a blank name blocks confirmation`() { + val model = model(listOf(candidate("a + b", "total"))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("")) + + assertEquals(NameProblem.Blank, model.uiState.value.nameProblem) + assertNull(model.selection()) + } + + @Test + fun `a keyword blocks confirmation in either language`() { + val kotlin = model(listOf(candidate("a + b", "total"))) + kotlin.onEvent(ExtractMethodUiEvent.NameChanged("when")) + assertEquals(NameProblem.Keyword, kotlin.uiState.value.nameProblem) + assertNull(kotlin.selection()) + + val java = model(listOf(candidate("a + b", "total")), javaKeywords) + java.onEvent(ExtractMethodUiEvent.NameChanged("static")) + assertEquals(NameProblem.Keyword, java.uiState.value.nameProblem) + assertNull(java.selection()) + } + + @Test + fun `a name that only looks like a keyword is accepted`() { + val model = model(listOf(candidate("a + b", "total"))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("whenever")) + + assertNull(model.uiState.value.nameProblem) + assertTrue(model.uiState.value.canConfirm) + } +}