Integrate Compose Helper Plugin #1768

thelumiereguy · 2022-01-30T16:49:43Z

Changes

Added 3 intentions
1. Wrap with Composable
2. Remove a composable
3. Remove the parent composable
Added live templates for the 'Wrap with' intention

Notes -

Image for intention is optional. Can remove it, if you guys say so
I had a bit of trouble writing tests for the intention itself (since we are looking for the FQName of Composable). And I could not emulate that in testData. If you guys could guide me with that, I'd be grateful!
Currently, my plugin is in a private repository. If this is merged, I'm planning to make it public. So, should I stick to the Apache license as well then? (First time contributing to something)

Screenshot

added comments wherever necessary formatted with ktlint

olonho · 2022-02-01T06:56:52Z

Thanks for your contribution, we're working on reviewing it!

olonho · 2022-02-03T08:49:26Z

...n/org/jetbrains/compose/intentions/wrap_with_composable/wrap_with_actions/WrapWithActions.kt

@@ -0,0 +1,70 @@
+package org.jetbrains.compose.intentions.wrap_with_composable.wrap_with_actions


Better use camelCaseNaming.

camelCase for the package names?

and renamed package names

thelumiereguy · 2022-02-04T17:09:45Z

@olonho I've done the necessary changes. Please have a look

AlexeyTsvetkov · 2022-02-09T07:33:53Z

idea-plugin/src/main/resources/META-INF/plugin.xml

+        <intentionAction id="wrap_group" order="first">
+            <className>org.jetbrains.compose.intentions.WrapWithComposableIntentionGroup
+            </className>
+            <category>Composable intentions</category>


Usually, intentions category don't have the word "intentions" (you can check that by looking at the "Preferences | Editor | Intentions" in Intellij).
Android Compose uses Android Compose category for that, so I suggest changing the value to Compose Multiplatform.

Understood! I'll change that. Thanks!

AlexeyTsvetkov · 2022-02-09T07:37:24Z

idea-plugin/src/main/resources/META-INF/plugin.xml

+
+        <defaultLiveTemplates file="templates/ComposeMultiplatformTemplates.xml"/>
+
+        <intentionAction id="wrap_group" order="first">


Do id actually necessary at any of those?
Generally I'm OK with having them, but not unqualified (i.e. I suggest adding a package prefix, so ids don't accidentally clash with another plugin).

It's really not necessary. I was trying out if we could change the order of intentions using the "order" attribute. I'll remove both of them

AlexeyTsvetkov · 2022-02-10T04:46:52Z

idea-plugin/src/main/resources/templates/ComposeMultiplatformTemplates.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<templateSet group="ComposeMultiplatformTemplates">
+
+    <template name="WwB" description="Wrap with Box"


I feel like there is a discrepancy between the name of the template and what it does, when used as a template.
Wrap-* actions as intentions work as expected.
However, templates can be typed in the editor by the name, e.g. here Wrap with Box can be typed as WwB.
But the result of choosing the template in the completion is not, what I would expect from the word wrap.
Consider the following example:

Column { Row { // the editor caret is here } }

If I type WwB at the caret location and press enter, I get the following code:

Column { Row { Box(modifier = Modifier) { } } }

In my opinion, that's not wrapping, this is an insertion of Box.
What do you think?

You're right. I had not thought about Live templates from that angle. My Intention does the wrapping. Live template inserts the respective Composable, like you said. Shall I just rename them?

AlexeyTsvetkov · 2022-02-10T04:49:28Z

idea-plugin/src/main/resources/templates/ComposeMultiplatformTemplates.xml

+<templateSet group="ComposeMultiplatformTemplates">
+
+    <template name="WwB" description="Wrap with Box"
+              value="$COMPOSABLE$(modifier = androidx.compose.ui.Modifier) {&#10; $SELECTION$ &#10; }"


I'm not sure if adding modifier argument by default is better, than not adding it. Probably add/remove modifier could be another intention, but I don't have a strong opinion.

Understood! I asked around a few people regarding this. Even though some of them said they needed the default modifier, most of them did not. So I'll remove it. :)

AlexeyTsvetkov · 2022-02-10T04:50:28Z

idea-plugin/src/main/resources/intentionDescriptions/RemoveComposableIntention/description.html

+<html lang="en">
+<body>
+<p>
+    A simple intention to remove the composable altogether.


Should it be "a Composable" in this context? I also think it should be capitalized.

yes! That sounds better. Will do that

AlexeyTsvetkov · 2022-02-10T04:58:29Z

idea-plugin/src/main/resources/META-INF/plugin.xml

+        </intentionAction>
+
+        <intentionAction id="remove_parent_composable" order="last">
+            <className>org.jetbrains.compose.intentions.RemoveParentComposableIntention


I don't think this intention works as intended.
In the following example I see a few issues:

@Composable fun ExampleComposable() { Column { Row { Box { } } } }

The intention is available on Column, but unavailable on Box. I don't understand, what parent Composable can be removed on Column in this example.

Using intention on Row or Column (in the example) does the same thing as remove Composable.

Maybe I misunderstood the meaning behind the intention?

Oh. I'm sorry, but I think you misunderstood the feature.

Remove Composable removes everything - Selected and its children.
Remove Parent Composable just removes the Selected, but not its children.

In the above example, it'll only be available in case of Column, Row, and not on Box (since it doesnt have any children)

AlexeyTsvetkov · 2022-02-10T05:41:43Z

...n/kotlin/org/jetbrains/compose/intentions/utils/isIntentionAvailable/IsIntentionAvailable.kt

+import org.jetbrains.compose.intentions.utils.composableFinder.ComposableFunctionFinder
+import org.jetbrains.kotlin.idea.KotlinLanguage
+
+interface IsIntentionAvailable {


I don't see any reason to introduce a package and an interface just to namespace a single function.
Why not just put all these little functions inside one or few files?

In other words, how current implementation is better than the following?

package org.jetbrains.compose.intentions.utils fun PsiElement.isComposableIntentionAvailable( composableFunctionFinder: ComposableFunctionFinder ): Boolean { if (language == KotlinLanguage.INSTANCE && isWritable) { val parent = parent ?: return false return composableFunctionFinder.isFunctionComposable(parent) } return false }

Understood! That can be done, yes!
I just personally dont like extension functions being available throughout the project.

Since it's a single function, I'll do as you say

AlexeyTsvetkov · 2022-02-10T05:50:08Z

...rc/main/kotlin/org/jetbrains/compose/intentions/utils/getRootPsiElement/GetRootPsiElement.kt

+     * 4. KtValueArgumentList - ()
+     */
+    tailrec operator fun invoke(element: PsiElement, iteration: Int = 0): PsiElement? {
+        // To avoid infinite loops


I don't think an infinite loop is possible, since you only move up on a syntax tree.

I'm sorry, there wont be an 'infinite' loop per se. I just added it to cover all edge cases. I shall remove it

AlexeyTsvetkov · 2022-02-10T05:52:34Z

...-plugin/src/main/kotlin/org/jetbrains/compose/intentions/WrapWithComposableIntentionGroup.kt

+        return "Wrap with Composable"
+    }
+
+    override fun getIcon(flags: Int): Icon = PreviewIcons.COMPOSE


I suggest to avoid overriding an icon for any intention

Yess! I shall remove it. That was one of the questions I had asked in the original post.

AlexeyTsvetkov · 2022-02-10T06:39:12Z

...tlin/org/jetbrains/compose/intentions/utils/composableFinder/ComposableFunctionFinderImpl.kt

+
+    override fun isFunctionComposable(psiElement: PsiElement): Boolean {
+        return when (psiElement) {
+            is KtNameReferenceExpression -> psiElement.isComposable()


What do you think about limiting the scope of intentions to just KtNameReferenceExpression?
I.e. dropping KtCallExpression, KtProperty and KtValueArgumentList.
I'm just not sure that the current implementation is intuitive and coherent.

For example:

Composable1(modifier = Modifier) {}

In the snippet above, the intentions are available at Composable1 (which makes intuitive sense), at ( and ), but not at modifier = Modifier and not at {}.

There are lots of types of elements in Kotlin PSI, I'm not sure that the current implementation will behave consistently. At the same time it complicates the behavior of the intention as well. For example, I can't clearly articulate, what GetRootPsiElement does, and why it is written in the way it is without taking into account this function. Narrowing the scope of intentions can make it easier to both use intentions and reason about code

@AlexeyTsvetkov
We can remove the KtCallExpression check. As per your example, it currently works on 'Composable' and '('. We would need 'KtValueArgumentList' for detecting on () like you said. {} is not included, of course.

KtProperty can be skipped, but you'll lose the ability to use it with properties. Currently, you can use it on 'val/var', 'property name', and the composable function like 'remember/animateAsFloat'. If you wish to take that call, I can remove it.

So, what GetRootPsiElement does is, if you invoke it on a PsiElement, it gives you the parent PsiElement.

For a composable function, it can be the KtCallExpression.
For a delegatedProperty, it will be KtProperty.

The root psi element is them wrapped with / deleted respectively. You can check out the GetRootPsiElementTest for more info

renamed live template names cleaned up interfaces

thelumiereguy · 2022-02-26T16:54:13Z

@AlexeyTsvetkov I've made the necessary changes and answered some of your questions above. Please have a look at them when you have time. Thank you!

thelumiereguy added 5 commits January 26, 2022 18:32

integrated plugin and all its files

e80fdb3

ktlint fixes

e64e68b

renamed classes

05f2cf6

added comments wherever necessary formatted with ktlint

renamed live template file

718d0a0

added tests for dot qualified expressions

66a8a05

olonho reviewed Feb 3, 2022

View reviewed changes

flattened packages

b79e270

and renamed package names

AlexeyTsvetkov self-requested a review February 8, 2022 07:46

AlexeyTsvetkov reviewed Feb 10, 2022

View reviewed changes

removed icon from intentions

a72c049

renamed live template names cleaned up interfaces

thelumiereguy requested a review from AlexeyTsvetkov January 5, 2023 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Compose Helper Plugin #1768

Integrate Compose Helper Plugin #1768

thelumiereguy commented Jan 30, 2022

olonho commented Feb 1, 2022

olonho Feb 3, 2022

thelumiereguy Feb 3, 2022

thelumiereguy commented Feb 4, 2022

AlexeyTsvetkov Feb 9, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 9, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

AlexeyTsvetkov Feb 10, 2022

thelumiereguy Feb 11, 2022

thelumiereguy commented Feb 26, 2022

		@@ -0,0 +1,70 @@
		package org.jetbrains.compose.intentions.wrap_with_composable.wrap_with_actions


		<defaultLiveTemplates file="templates/ComposeMultiplatformTemplates.xml"/>

		<intentionAction id="wrap_group" order="first">

Integrate Compose Helper Plugin #1768

Are you sure you want to change the base?

Integrate Compose Helper Plugin #1768

Conversation

thelumiereguy commented Jan 30, 2022

Changes

Notes -

Screenshot

olonho commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelumiereguy commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelumiereguy commented Feb 26, 2022