Skip to content

Commit 98096ca

Browse files
itsaky-adfaitsaky-adfa
authored
ADFA-4827: Kotlin inline variable code action (K2 LSP) (#1706)
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
1 parent b21d93d commit 98096ca

28 files changed

Lines changed: 3763 additions & 91 deletions

‎app/src/main/java/com/itsaky/androidide/fragments/sheets/ProgressSheet.java‎

Lines changed: 85 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -30,68 +30,89 @@
3030

3131
public class ProgressSheet extends BaseBottomSheetFragment {
3232

33-
private LayoutProgressSheetBinding binding;
34-
private String message = "";
35-
private String subMessage = "";
36-
private boolean subMessageEnabled = false;
37-
38-
@Override
39-
public void onViewCreated(@NonNull View view, Bundle savedInstanceState) {
40-
super.onViewCreated(view, savedInstanceState);
41-
42-
binding.message.setText(message);
43-
44-
final var params = (ConstraintLayout.LayoutParams) binding.message.getLayoutParams();
45-
if (subMessageEnabled) {
46-
binding.subMessage.setText(subMessage);
47-
binding.subMessage.setVisibility(View.VISIBLE);
48-
params.bottomToBottom = View.NO_ID;
49-
} else {
50-
binding.subMessage.setVisibility(View.GONE);
51-
params.bottomToBottom = LayoutParams.PARENT_ID;
52-
}
53-
}
54-
55-
@Nullable
56-
@Override
57-
public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup container,
58-
@Nullable Bundle savedInstanceState
59-
) {
60-
binding = LayoutProgressSheetBinding.inflate(LayoutInflater.from(getContext()));
61-
return binding.getRoot();
62-
}
63-
64-
public void setSubMessageEnabled(boolean enabled) {
65-
this.subMessageEnabled = enabled;
66-
}
67-
68-
public void setSubMessage(String msg) {
69-
this.subMessage = msg;
70-
if (isShowing()) {
71-
binding.subMessage.setText(msg);
72-
}
73-
}
74-
75-
public ProgressSheet setMessage(String message) {
76-
this.message = message;
77-
if (isShowing()) {
78-
binding.message.setText(message);
79-
}
80-
81-
return this;
82-
}
83-
84-
public ProgressSheet setProgressDrawable(Drawable drawable) {
85-
if (isShowing()) {
86-
binding.progress.setIndeterminateDrawable(drawable);
87-
}
88-
return this;
89-
}
90-
91-
@Override
92-
public void dismiss() {
93-
if (isShowing()) {
94-
super.dismiss();
95-
}
96-
}
33+
private LayoutProgressSheetBinding binding;
34+
private String message = "";
35+
private String subMessage = "";
36+
private boolean subMessageEnabled = false;
37+
38+
/* A dismiss that arrived before this fragment was attached, replayed in onStart. */
39+
private boolean dismissPending = false;
40+
41+
/**
42+
* {@inheritDoc}
43+
*
44+
* <p>
45+
* A dismiss that arrives before the enqueued {@code show()} transaction has run is remembered rather than dropped: the fragment is not attached to a fragment manager yet, so dismissing now would throw, but doing nothing would leave the sheet on screen with nothing left to close it.
46+
*/
47+
@Override
48+
public void dismiss() {
49+
if (!isAdded()) {
50+
dismissPending = true;
51+
return;
52+
}
53+
54+
dismissPending = false;
55+
super.dismiss();
56+
}
57+
58+
@Nullable
59+
@Override
60+
public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup container,
61+
@Nullable Bundle savedInstanceState) {
62+
binding = LayoutProgressSheetBinding.inflate(LayoutInflater.from(getContext()));
63+
return binding.getRoot();
64+
}
65+
66+
@Override
67+
public void onStart() {
68+
super.onStart();
69+
if (dismissPending) {
70+
dismissPending = false;
71+
dismissAllowingStateLoss();
72+
}
73+
}
74+
75+
@Override
76+
public void onViewCreated(@NonNull View view, Bundle savedInstanceState) {
77+
super.onViewCreated(view, savedInstanceState);
78+
79+
binding.message.setText(message);
80+
81+
final var params = (ConstraintLayout.LayoutParams) binding.message.getLayoutParams();
82+
if (subMessageEnabled) {
83+
binding.subMessage.setText(subMessage);
84+
binding.subMessage.setVisibility(View.VISIBLE);
85+
params.bottomToBottom = View.NO_ID;
86+
} else {
87+
binding.subMessage.setVisibility(View.GONE);
88+
params.bottomToBottom = LayoutParams.PARENT_ID;
89+
}
90+
}
91+
92+
public ProgressSheet setMessage(String message) {
93+
this.message = message;
94+
if (isShowing()) {
95+
binding.message.setText(message);
96+
}
97+
98+
return this;
99+
}
100+
101+
public ProgressSheet setProgressDrawable(Drawable drawable) {
102+
if (isShowing()) {
103+
binding.progress.setIndeterminateDrawable(drawable);
104+
}
105+
return this;
106+
}
107+
108+
public void setSubMessage(String msg) {
109+
this.subMessage = msg;
110+
if (isShowing()) {
111+
binding.subMessage.setText(msg);
112+
}
113+
}
114+
115+
public void setSubMessageEnabled(boolean enabled) {
116+
this.subMessageEnabled = enabled;
117+
}
97118
}

‎app/src/test/java/com/itsaky/androidide/fragments/sheets/ProgressSheetDismissTest.kt‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/*
2+
* This file is part of AndroidIDE.
3+
*
4+
* AndroidIDE is free software: you can redistribute it and/or modify
5+
* it under the terms of the GNU General Public License as published by
6+
* the Free Software Foundation, either version 3 of the License, or
7+
* (at your option) any later version.
8+
*
9+
* AndroidIDE is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
* GNU General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU General Public License
15+
* along with AndroidIDE. If not, see <https://www.gnu.org/licenses/>.
16+
*/
17+
18+
package com.itsaky.androidide.fragments.sheets
19+
20+
import android.os.Looper
21+
import androidx.appcompat.app.AppCompatActivity
22+
import com.google.common.truth.Truth.assertThat
23+
import com.itsaky.androidide.app.BaseApplication
24+
import org.junit.Test
25+
import org.junit.runner.RunWith
26+
import org.robolectric.Robolectric
27+
import org.robolectric.RobolectricTestRunner
28+
import org.robolectric.Shadows.shadowOf
29+
import org.robolectric.annotation.Config
30+
31+
/**
32+
* `DialogFragment.show` only enqueues the add transaction, so a dismiss issued in the same
33+
* main-thread pass lands before the sheet exists. Callers that show a progress sheet around work
34+
* that can finish synchronously - `IDELanguageClientImpl.performCodeAction` - rely on that dismiss
35+
* being honoured; dropping it strands the sheet on screen with nothing left to close it.
36+
*/
37+
@RunWith(RobolectricTestRunner::class)
38+
@Config(application = ProgressSheetDismissTest.TestApp::class)
39+
class ProgressSheetDismissTest {
40+
open class TestApp : BaseApplication()
41+
42+
@Test
43+
fun `dismiss issued before the show transaction runs still closes the sheet`() {
44+
val activity = Robolectric.buildActivity(AppCompatActivity::class.java).setup().get()
45+
val manager = activity.supportFragmentManager
46+
47+
val sheet = ProgressSheet()
48+
sheet.isCancelable = false
49+
sheet.show(manager, TAG)
50+
sheet.dismiss()
51+
52+
shadowOf(Looper.getMainLooper()).idle()
53+
manager.executePendingTransactions()
54+
55+
assertThat(manager.findFragmentByTag(TAG)).isNull()
56+
assertThat(sheet.isShowing).isFalse()
57+
}
58+
59+
@Test
60+
fun `dismiss issued once the sheet is on screen closes it`() {
61+
val activity = Robolectric.buildActivity(AppCompatActivity::class.java).setup().get()
62+
val manager = activity.supportFragmentManager
63+
64+
val sheet = ProgressSheet()
65+
sheet.isCancelable = false
66+
sheet.show(manager, TAG)
67+
68+
shadowOf(Looper.getMainLooper()).idle()
69+
manager.executePendingTransactions()
70+
assertThat(sheet.isShowing).isTrue()
71+
72+
sheet.dismiss()
73+
74+
shadowOf(Looper.getMainLooper()).idle()
75+
manager.executePendingTransactions()
76+
77+
assertThat(manager.findFragmentByTag(TAG)).isNull()
78+
assertThat(sheet.isShowing).isFalse()
79+
}
80+
81+
private companion object {
82+
const val TAG = "progress_sheet_test"
83+
}
84+
}

‎docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# 0012. Refactoring UI lives in the owning LSP module
1+
# 0013. Refactoring UI lives in the owning LSP module
22

33
- **Status:** Proposed
44
- **Date:** 2026-08-03

‎docs/adr/0014-refactorings-decline-rather-than-rewrite.md‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
# 0013. Interactive refactorings decline rather than rewrite unselected code
1+
# 0014. Interactive refactorings decline rather than rewrite unselected code
22

33
- **Status:** Proposed
44
- **Date:** 2026-08-10
55
- **Deciders:** Code On The Go team
66

77
## Context
88

9-
The K2 Kotlin LSP is growing a family of interactive refactorings: extract variable (ADFA-4826), extract method (ADFA-5080), inline variable (ADFA-4827), semantic rename (ADFA-4825). [ADR 0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) settles where their UI lives and that analysis produces plain data. It says nothing about how capable they should be.
9+
The K2 Kotlin LSP is growing a family of interactive refactorings: extract variable (ADFA-4826), extract method (ADFA-5080), inline variable (ADFA-4827), semantic rename (ADFA-4825). [ADR 0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) settles where their UI lives and that analysis produces plain data. It says nothing about how capable they should be.
1010

1111
That question turns out to dominate the requirements. Designing extract method surfaced a run of cases where the transformation the user asked for cannot be performed by *moving* their code - it also needs the moved code's interior edited, or a guess about intent:
1212

@@ -29,8 +29,11 @@ Concretely:
2929
- **Prefer a stricter rule to a cleverer one** when strictness costs capability and cleverness costs certainty. Extract method refuses a reassigned outer `var` even when the write is provably dead, because proving it needs liveness analysis.
3030
- **Never emit code that does not compile, and avoid emitting code that warns.** The two modifiers extract method *does* add - `suspend` and `@Composable` - are required precisely because omitting them breaks compilation.
3131
- **A refusal is a backlog item, not a dead end.** Where the refused case is common, file it: ADFA-5082 tracks the reassigned-`var` output.
32+
- **Where part of the request is sound, apply that part and say so.** A refactoring has three outcomes, not two: apply, refuse, or **apply partially**. Inline variable (ADFA-4827) is the case that needs the third - a variable whose value is reassigned partway through can be inlined at the references before the write and nowhere after it, so refusing the whole thing would discard a sound transformation of the earlier half. A partial application must report both counts and what it left behind, and it must leave the file compiling on its own, exactly as a full application does. It is not a licence to apply the doubtful part and hope.
3233

33-
This applies to the whole refactoring family, not just extract method. Inline variable and rename inherit it.
34+
This applies to the whole refactoring family, not just extract method. Inline variable and rename inherit it - inline variable adding the third outcome above, rename presumed to need only the first two until its design says otherwise.
35+
36+
**Out of scope of this decision.** Whether a refactoring may duplicate an expression that is evaluated more than once. Inline variable does, without checking for side effects (see [kotlin-inline-variable.md](../features/kotlin-inline-variable.md)): the emitted code compiles, carries no warning, and is the user's own expression unedited, so nothing above forbids it. Kotlin offers no way to prove purity, so a check would be a heuristic rather than a stricter rule, and this ADR prefers strictness to cleverness in both directions.
3437

3538
## Consequences
3639

@@ -40,12 +43,14 @@ This applies to the whole refactoring family, not just extract method. Inline va
4043
- Refusal reasons are cheap to specify, cheap to test (one case each) and cheap to QA, where a clever transformation needs its own test matrix and its own failure modes.
4144
- The rules are stateable in a sentence each, which is what makes the feature docs reviewable by someone who has not read the implementation.
4245
- Excluding cases by construction keeps the analysis pass small, which matters when it runs on a phone.
46+
- Partial application recovers capability that an all-or-nothing rule would throw away, without weakening the compile-and-do-not-warn guarantee: the sound part is applied and the doubtful part is simply not touched.
4347

4448
**Negative / costs**
4549

4650
- The refactorings are visibly less capable than a desktop IDE's. Two of extract method's refusals - a reassigned outer `var` (the accumulator loop) and an enclosing `with`/`apply` receiver (pervasive in Android code) - will be hit routinely.
4751
- The quality of the *messages* becomes load-bearing. A generic refusal reads as a broken feature, so this decision spends translated strings: roughly seven for extract method alone.
4852
- Users arriving from IntelliJ will read some refusals as regressions rather than as design.
53+
- A partial application is harder to *report* than either other outcome, and harder to QA: the message has to convey two counts and a surviving declaration in one flash, and every "how many were left behind" case is its own test. A partial result the user misreads as a complete one is the failure mode to watch.
4954
- The line is a judgement, not a formalism. "Editing the interior of the moved code" is clear in the cases above but will need re-application, case by case, in each future refactoring.
5055

5156
## Alternatives considered
@@ -57,7 +62,8 @@ This applies to the whole refactoring family, not just extract method. Inline va
5762

5863
## Related
5964

60-
- [ADR 0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - where refactoring UI lives; this ADR answers *how capable it is*
65+
- [ADR 0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) - where refactoring UI lives; this ADR answers *how capable it is*
6166
- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) - the K2 Analysis API as the Kotlin semantic source of truth
6267
- [kotlin-extract-method.md](../features/kotlin-extract-method.md) - R7 to R10 and R14 are this decision applied case by case
6368
- [kotlin-extract-variable.md](../features/kotlin-extract-variable.md) - the shared vocabulary and primitives
69+
- [kotlin-inline-variable.md](../features/kotlin-inline-variable.md) - R6 to R9 are this decision applied to a subtractive refactoring, and the origin of the third outcome

‎docs/adr/README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,5 +26,5 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences
2626
| [0010](0010-navigation-resolves-via-analysis-api.md) | Kotlin navigation resolves via the Analysis API, not the symbol index | Proposed |
2727
| [0011](0011-command-analysis-priority.md) | User-invoked commands get their own analysis priority | Proposed |
2828
| [0012](0012-volatile-build-metadata-out-of-abis.md) | Keep volatile build metadata out of module ABIs | Proposed |
29-
| [0013](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed |
30-
| [0014](0013-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed |
29+
| [0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed |
30+
| [0014](0014-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed |

0 commit comments

Comments
 (0)