Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature: move members #735

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Feature: move members #735

wants to merge 6 commits into from

Conversation

ishmum123
Copy link
Contributor

This is draft PR which aims to close #16 (eventually). The initial PR is just to ensure that this is going towards the correct direction.

Move.Members.mov

@ishmum123
Copy link
Contributor Author

@klazuka can you please provide some comments on this?

@pravdomil
Copy link
Contributor

pravdomil commented Feb 12, 2021 •
edited

Hi @ishmum123, I have tested the feature and I got following exception, when I invoke Rename dialog:

java.lang.NoSuchFieldError: disableMagic
	at org.elm.ide.refactoring.move.RsMoveTopLevelItemsDialog.createCenterPanel(ElmRefactoringDialog.kt:80)
	at com.intellij.openapi.ui.DialogWrapper.init(DialogWrapper.java:1331)
	at org.elm.ide.refactoring.move.RsMoveTopLevelItemsDialog.<init>(ElmRefactoringDialog.kt:29)
	at org.elm.ide.refactoring.move.ElmMoveTopLevelItemHandler.doMove(ElmMoveTopLevelItemHandler.kt:64)
	at org.elm.ide.refactoring.move.ElmMoveTopLevelItemHandler.tryToMove(ElmMoveTopLevelItemHandler.kt:31)
	at com.intellij.refactoring.move.MoveHandler.invoke(MoveHandler.java:71)
	at com.intellij.refactoring.actions.BaseRefactoringAction.performRefactoringAction(BaseRefactoringAction.java:156)
	at com.intellij.refactoring.actions.BaseRefactoringAction.actionPerformed(BaseRefactoringAction.java:108)
	at com.intellij.openapi.actionSystem.ex.ActionUtil.performActionDumbAware(ActionUtil.java:281)
	at com.intellij.openapi.actionSystem.ex.ActionUtil.performActionDumbAwareWithCallbacks(ActionUtil.java:275)
	at com.intellij.ui.popup.ActionPopupStep.performAction(ActionPopupStep.java:223)
	at com.intellij.ui.popup.ActionPopupStep.performAction(ActionPopupStep.java:213)
	at com.intellij.ui.popup.ActionPopupStep.lambda$onChosen$2(ActionPopupStep.java:207)
	at com.intellij.openapi.application.TransactionGuardImpl.performUserActivity(TransactionGuardImpl.java:95)
	at com.intellij.ui.popup.AbstractPopup.lambda$dispose$16(AbstractPopup.java:1467)
	at com.intellij.util.ui.EdtInvocationManager.invokeLaterIfNeeded(EdtInvocationManager.java:88)
	at com.intellij.util.ui.UIUtil.invokeLaterIfNeeded(UIUtil.java:2194)
	at com.intellij.ide.IdeEventQueue.ifFocusEventsInTheQueue(IdeEventQueue.java:188)
	at com.intellij.ide.IdeEventQueue.executeWhenAllFocusEventsLeftTheQueue(IdeEventQueue.java:140)
	at com.intellij.openapi.wm.impl.FocusManagerImpl.doWhenFocusSettlesDown(FocusManagerImpl.java:173)
	at com.intellij.openapi.wm.impl.IdeFocusManagerImpl.doWhenFocusSettlesDown(IdeFocusManagerImpl.java:36)
	at com.intellij.ui.popup.AbstractPopup.dispose(AbstractPopup.java:1463)
	at com.intellij.ui.popup.WizardPopup.dispose(WizardPopup.java:158)
	at com.intellij.ui.popup.list.ListPopupImpl.dispose(ListPopupImpl.java:329)
	at com.intellij.ui.popup.PopupFactoryImpl$ActionGroupPopup.dispose(PopupFactoryImpl.java:304)
	at com.intellij.openapi.util.ObjectTree.runWithTrace(ObjectTree.java:138)
	at com.intellij.openapi.util.ObjectTree.executeAll(ObjectTree.java:168)
	at com.intellij.openapi.util.Disposer.dispose(Disposer.java:142)
	at com.intellij.openapi.util.Disposer.dispose(Disposer.java:130)
	at com.intellij.ui.popup.WizardPopup.disposeAllParents(WizardPopup.java:261)
	at com.intellij.ui.popup.list.ListPopupImpl.handleNextStep(ListPopupImpl.java:446)
	at com.intellij.ui.popup.list.ListPopupImpl._handleSelect(ListPopupImpl.java:418)
	at com.intellij.ui.popup.list.ListPopupImpl.handleSelect(ListPopupImpl.java:359)
	at com.intellij.ui.popup.list.ListPopupImpl$1.actionPerformed(ListPopupImpl.java:272)
	at com.intellij.ui.popup.WizardPopup.proceedKeyEvent(WizardPopup.java:376)
	at com.intellij.ui.popup.WizardPopup.dispatch(WizardPopup.java:356)
	at com.intellij.ui.popup.PopupDispatcher.dispatchKeyEvent(PopupDispatcher.java:112)
	at com.intellij.ui.popup.PopupDispatcher.dispatch(PopupDispatcher.java:148)
	at com.intellij.ide.IdePopupManager.dispatch(IdePopupManager.java:93)
	at com.intellij.ide.IdeEventQueue._dispatchEvent(IdeEventQueue.java:810)
	at com.intellij.ide.IdeEventQueue.lambda$dispatchEvent$8(IdeEventQueue.java:454)
	at com.intellij.openapi.progress.impl.CoreProgressManager.computePrioritized(CoreProgressManager.java:773)
	at com.intellij.ide.IdeEventQueue.lambda$dispatchEvent$9(IdeEventQueue.java:453)
	at com.intellij.openapi.application.impl.ApplicationImpl.runIntendedWriteActionOnCurrentThread(ApplicationImpl.java:822)
	at com.intellij.ide.IdeEventQueue.dispatchEvent(IdeEventQueue.java:507)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

@ishmum123 ishmum123 marked this pull request as draft February 12, 2021 19:13
@ishmum123
Copy link
Contributor Author

Hi @pravdomil, let me get back to you after writing some tests for this...

@ishmum123
Copy link
Contributor Author

Hi @pravdomil, I checked locally and the dialog doesn't seem to throw an error. Would be really grateful if you could give me the re-generation steps. Also, do you have any tips on testing it?

klazuka
klazuka reviewed Feb 13, 2021
View reviewed changes
Copy link
Collaborator

@klazuka klazuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It looks like you're going in the right direction. And I completely agree that basing it off of intellij-rust seems totally reasonable given undocumented APIs.

My main requirement would be that it generates code that compiles. So all imports and exposings should be fixed-up (which it looks like you do for many cases).

In terms of formatting, it's not as important to me that it looks perfect, although it would be nice if it preserved a sensible amount of blank lines between top-level declarations. The user can always invoke elm-format in the currently-editing file, but that would miss changes in the file that you moved the declarations to. So I would pay special attention to how it inserts into the other file to make sure that it's reasonably clean.

src/main/kotlin/org/elm/ide/refactoring/move/ElmRefactoringDialog.kt Outdated
import java.io.File
import javax.swing.JComponent

class RsMoveTopLevelItemsDialog(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix should be changed to Elm

src/main/kotlin/org/elm/ide/refactoring/move/RsMoveTopLevelItemsProcessor.kt Outdated
import org.elm.lang.core.psi.elements.*
import org.elm.lang.core.psi.moduleName

class RsMoveTopLevelItemsProcessor(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix and filename need to be changed to Elm

@pravdomil
Copy link
Contributor

pravdomil commented Feb 15, 2021 •
edited

@ishmum123

hmmm.mov

@ishmum123 ishmum123 marked this pull request as ready for review February 23, 2021 01:57
@ishmum123
Copy link
Contributor Author

@pravdomil sorry for the long pause. I was worried about the tests for this. Thanks for the video and I would definitely try to look into it, God willing.

@ishmum123 ishmum123 changed the title Feature: move members (draft) Feature: move members Feb 23, 2021
@ishmum123
Copy link
Contributor Author

@klazuka this issue seems to be spawning cases like a hydra. I am not sure how much more time I can allocate for this. Would be really grateful if someone could take over from here

@ishmum123
Copy link
Contributor Author

@pravdomil seems to work on my machine now. Would be grateful if you could try again...

@pravdomil
Copy link
Contributor

pravdomil commented Feb 25, 2021 •
edited

at commit acc249c

hmmm.mov

@ishmum123
Copy link
Contributor Author

I am very sorry... I keep getting this

Screen.Recording.2021-02-26.at.6.47.48.AM.mov

@pravdomil
Copy link
Contributor

@ishmum123 intellij-rust/intellij-rust#4411 (comment)

@cies
Copy link
Contributor

cies commented Apr 3, 2024

Can I close this issue, or is there still a chance the PR get shaped up for merge?

@Birowsky
Copy link

Birowsky commented Apr 3, 2024

I don't remember this ever being completely functional.

@cies
Copy link
Contributor

cies commented Apr 3, 2024

I don't remember this ever being completely functional.

thanks for chiming in!

@cies cies added the feature label Apr 3, 2024
@ishmum123
Copy link
Contributor Author

#739 (comment)

the same comment applies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klazuka klazuka klazuka left review comments

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add "Move module members" refactoring
5 participants
@ishmum123 @pravdomil @cies @Birowsky @klazuka