Change Signature: Add/remove constructor keyword on primary constructor when changing modifier list

compiler/frontend/src/org/jetbrains/kotlin/psi/JetPrimaryConstructor.kt

@@ -17,9 +17,10 @@
 package org.jetbrains.kotlin.psi
 
 import com.intellij.lang.ASTNode
+import com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.lexer.JetTokens
-import org.jetbrains.kotlin.psi.addRemoveModifier.*
+import org.jetbrains.kotlin.psi.addRemoveModifier.addModifier
 import org.jetbrains.kotlin.psi.stubs.KotlinPlaceHolderStub
 import org.jetbrains.kotlin.psi.stubs.elements.JetStubElementTypes
 
@@ -31,16 +32,29 @@ public class JetPrimaryConstructor : JetConstructor<JetPrimaryConstructor> {
 
     override fun getContainingClassOrObject() = getParent() as JetClassOrObject
 
+    private fun getOrCreateConstructorKeyword(): PsiElement {
+        return getConstructorKeyword() ?: addBefore(JetPsiFactory(this).createConstructorKeyword(), valueParameterList!!)
+    }
+
     override fun addModifier(modifier: JetModifierKeywordToken) {
-        val modifierList = getModifierList()
+        val modifierList = modifierList
         if (modifierList != null) {
             addModifier(modifierList, modifier, JetTokens.PUBLIC_KEYWORD)
+            if (this.modifierList == null) {
+                getConstructorKeyword()?.delete()
+            }
         }
         else {
             if (modifier == JetTokens.PUBLIC_KEYWORD) return
-            val parameterList = getValueParameterList()!!
+            val newModifierList = JetPsiFactory(this).createModifierList(modifier)
-            val newModifierList = JetPsiFactory(getProject()).createModifierList(modifier)
+            addBefore(newModifierList, getOrCreateConstructorKeyword())
-            addBefore(newModifierList, parameterList)
+        }
+    }
+
+    override fun removeModifier(modifier: JetModifierKeywordToken) {
+        super.removeModifier(modifier)
+        if (modifierList == null) {
+            getConstructorKeyword()?.delete()
         }
     }
 

compiler/frontend/src/org/jetbrains/kotlin/psi/addRemoveModifier.kt

@@ -66,6 +66,9 @@ internal fun addModifier(modifierList: JetModifierList, modifier: JetModifierKey
     if (modifier == defaultVisibilityModifier) { // do not insert explicit 'internal' keyword (or 'public' for primary constructor)
         //TODO: code style option
         modifierToReplace?.delete()
+        if (modifierList.firstChild == null) {
+            modifierList.delete()
+        }
         return
     }
 
@@ -98,7 +101,12 @@ internal fun addModifier(modifierList: JetModifierList, modifier: JetModifierKey
 }
 
 internal fun removeModifier(owner: JetModifierListOwner, modifier: JetModifierKeywordToken) {
-    owner.getModifierList()?.getModifier(modifier)?.delete()
+    owner.getModifierList()?.let {
+        it.getModifier(modifier)?.delete()
+        if (it.firstChild == null) {
+            it.delete()
+        }
+    }
 }
 
 private val MODIFIERS_TO_REPLACE = mapOf(

idea/src/org/jetbrains/kotlin/idea/refactoring/changeSignature/JetChangeInfo.kt

@@ -194,7 +194,7 @@ public open class JetChangeInfo(
             buffer.append(name)
 
             if (newVisibility != defaultVisibility) {
-                buffer.append(' ').append(newVisibility).append(' ')
+                buffer.append(' ').append(newVisibility).append(" constructor ")
             }
         }
         else {

idea/src/org/jetbrains/kotlin/idea/refactoring/changeSignature/usages/JetCallableDefinitionUsage.java

@@ -293,18 +293,10 @@ public class JetCallableDefinitionUsage<T extends PsiElement> extends JetUsageIn
         }
         else {
             if (element instanceof JetClass) {
-                PsiElement anchor = ((JetClass) element).getTypeParameterList();
+                JetPrimaryConstructor constructor = ((JetClass) element).createPrimaryConstructorIfAbsent();
-
+                JetParameterList oldParameterList = constructor.getValueParameterList();
-                if (anchor == null) {
+                assert oldParameterList != null : "primary constructor from factory has parameter list";
-                    anchor = ((JetClass) element).getNameIdentifier();
+                newParameterList = (JetParameterList) oldParameterList.replace(newParameterList);
-                }
-                if (anchor != null) {
-                    JetPrimaryConstructor constructor =
-                            (JetPrimaryConstructor) element.addAfter(psiFactory.createPrimaryConstructor(), anchor);
-                    JetParameterList oldParameterList = constructor.getValueParameterList();
-                    assert oldParameterList != null : "primary constructor from factory has parameter list";
-                    newParameterList = (JetParameterList) oldParameterList.replace(newParameterList);
-                }
             }
             else if (isLambda) {
                 //noinspection ConstantConditions
@@ -357,9 +349,7 @@ public class JetCallableDefinitionUsage<T extends PsiElement> extends JetUsageIn
             ((JetCallableDeclaration)element).addModifier(newVisibilityToken);
         }
         else if (element instanceof JetClass) {
-            JetPrimaryConstructor constructor = ((JetClass) element).getPrimaryConstructor();
+            ((JetClass) element).createPrimaryConstructorIfAbsent().addModifier(newVisibilityToken);
-            assert constructor != null : "Primary constructor should be created before changing visibility";
-            constructor.addModifier(newVisibilityToken);
         }
         else throw new AssertionError("Invalid element: " + PsiUtilPackage.getElementTextWithContext(element));
     }

idea/testData/intentions/changeVisibility/private/noModifierListPrimaryConstructor.kt.after

@@ -1,2 +1,2 @@
 // INTENTION_TEXT: Make primary constructor private
-class C<caret> private (val v: Int)
+class C<caret> private constructor(val v: Int)

idea/testData/refactoring/changeSignature/AddConstructorVisibilityAfter.kt

@@ -1,4 +1,4 @@
-class C1 protected (val x: Any) {}
+class C1 protected constructor(val x: Any) {}
 
 fun f() {
   val c = C1(12);

idea/testData/refactoring/changeSignature/ChangeConstructorVisibilityAfter.kt

@@ -1 +1 @@
-class C1 protected (){}
+class C1 protected constructor(){}

idea/testData/refactoring/changeSignature/MakePrimaryConstructorPrivateNoParamsAfter.kt

@@ -0,0 +1 @@
+class X private constructor()

idea/testData/refactoring/changeSignature/MakePrimaryConstructorPrivateNoParamsBefore.kt

@@ -0,0 +1 @@
+class <caret>X

idea/testData/refactoring/changeSignature/MakePrimaryConstructorPublicAfter.kt

@@ -0,0 +1 @@
+class X (n: Int)

idea/testData/refactoring/changeSignature/MakePrimaryConstructorPublicBefore.kt

@@ -0,0 +1 @@
+class <caret>X private constructor(n: Int)

idea/tests/org/jetbrains/kotlin/idea/refactoring/changeSignature/JetChangeSignatureTest.java

@@ -1306,6 +1306,18 @@ public class JetChangeSignatureTest extends KotlinCodeInsightTestCase {
         doTest(changeInfo);
     }
 
+    public void testMakePrimaryConstructorPrivateNoParams() throws Exception {
+        JetChangeInfo changeInfo = getChangeInfo();
+        changeInfo.setNewVisibility(Visibilities.PRIVATE);
+        doTest(changeInfo);
+    }
+
+    public void testMakePrimaryConstructorPublic() throws Exception {
+        JetChangeInfo changeInfo = getChangeInfo();
+        changeInfo.setNewVisibility(Visibilities.PUBLIC);
+        doTest(changeInfo);
+    }
+
     private List<Editor> editors = null;
 
     private static final String[] EXTENSIONS = {".kt", ".java"};