Save actions #761
Save actions #761
Conversation
|
Test PASSed. |
|
Test FAILed. |
|
It fails with |
|
Test FAILed. |
| /** | ||
| * Applies a transformation to the tree of the saved document. | ||
| */ | ||
| final def transformFile(trans: Transformation[Tree, Tree]): Seq[Change] = |
There was a problem hiding this comment.
The code looks very familiar to me, could we change scala refactoring so you don't have to copy paste as much code?
There was a problem hiding this comment.
I thought about it but didn't come up with a good solution. I have my own Change objects, that is why I couldn't reuse the code from scala-refactoring. I want to play around with some cancellation conditions anyway, where own code is useful, but I can't say at the moment to what extend it makes sense to move the changes to scala-refactoring.
|
I took a look at the code, but I'm not sure what causes the UI dependency and the tests to fail. Could it be the ScalaDocumentProvider in the ScalaPlugin? But that's just a guess.. |
|
That could be it, the |
|
Now the error message makes totally sense to me, |
|
Yes, it is a problem when trying to load 'ScalaDocumentProvider' to use in 'ScalaPlugin'. I can give you the full stack traces if you want. |
|
After making the |
| with SaveActionExtensions { | ||
|
|
||
| /** We need to access private values of the super class */ | ||
| private val ra = ReflectAccess[CompilationUnitDocumentProvider](this) |
There was a problem hiding this comment.
I don't think reflexion is require. It is used for 2 attributes (fIsAboutToSave and fSavePolicy) which are only used in method you already override, or which would be easy to override (setSavePolicy and saveDocumentContent).
You could have local copies of this values.
|
I don't quite get why For me, it would make more sense to have an API like : trait SaveAction extends ScalaIdeExtension {
}
trait SaveActionWithDocument extends SaveAction {
def perform(document: Document)
}
trait SaveActionWithCompiler extends SaveAction {
def perform(document: Document, c: ScalaPresentationCompiler, t: ScalaPresentationCompiler#Tree, sf: SourceFile,)
}Then, it is only a matter of pattern matching to know which |
| ext | ||
| }.getOrElse(Seq()) | ||
| } | ||
| Await.ready(f, Duration.Inf).value.get match { |
There was a problem hiding this comment.
Using the TimeoutFuture is redundant of using the Await.ready with the right timeout.
You can have a simple Future, and the result of ready would be None in case of timeout, Some(Success(..)) on success, Some(Failure(e)) on execption e during the withSafeRunner.
There was a problem hiding this comment.
I already use Await.ready internally in TimeoutFuture, but I need to wait on two different futures and take the first one that completes, therefore I don't see how to simplify it.
|
Why do we need a wrapper for |
|
An explanation about the design of the API: The current design originated from my extensions project, it is an API meant to be used by end-users that don't have a lot of knowledge about Eclipse (and are not meant to to have). Beside from that my goal is to provide an API that does not only work for save actions, but also for other features. We only have save actions at the moment and the extensions that can be created by users do not yet work. That is why the design looks a bit heavy at the moment, but I believe that it is better to look ahead and implement the API accordingly - otherwise we would have a lot of changes to the API afterwards once my other ideas are implemented. My thoughts about specific points:
|
kiritsuku
force-pushed
the
save-actions
branch
from
d772a27 to
aeda050
Compare
|
Test PASSed. |
| @AfterClass | ||
| final def deleteProject(): Unit = { | ||
| EclipseUtils.workspaceRunnableIn(ScalaPlugin.plugin.workspaceRoot.getWorkspace()) { _ => | ||
| project.underlying.delete(/* force */ true, new NullProgressMonitor) |
There was a problem hiding this comment.
This is already implemented in SDTTestUtils.deleteProject().
|
The feature looks really good. I still have a few manual test I want to do. My main problem is with the side stuff.
a new extension system - I see a lot of difficult problems to solve to be able to create something different next to the Eclipse extension system. I think it has to be down at the osgi services level to be able to pick up configuration from other plugins and to link everything together, and correctly with the classloaders and other fun things. |
|
About I also think the new design is more extensible in the long term, for example it allows us to have support for linked models, which is not possible with |
|
I have a test case where the caret position is not where I would have expected it after the save actions are run. I have all of them enabled. Before package test
class ScalaClass {
/**
*/ ^$
$
}After package test
class ScalaClass {
/**
^*/
}I would expected the caret to be after the closing comment tag |
|
The 'undo' is not right. It should work as one big change to revert to the content just before the save actions are run. Right now, I have to hit undo for each change made to the file. |
kiritsuku
force-pushed
the
save-actions
branch
from
aeda050 to
cfb4522
Compare
|
Test FAILed. |
|
I'd like to give it a go locally, could you please rebase? It doesn't merge cleanly anymore... |
|
This is a great feature, and I love the attention to detail and new features you managed to implement so quickly! However, in my opinion the main appeal of this feature is the ability for users to write their own save actions "on the fly", in an editor or edit box, and have them running without restarting the IDE. I see that as the main goal of this project. The value comes from user-configurability. Even if there are still certain limitations (maybe tree-based actions won't be possible), this is the holy grail. What do you think? |
|
Awesome!! |
Conflicts: org.scala-ide.sdt.core.tests/src/org/scalaide/TestsSuite.scala
- Refactoring based save actions are disabled because they are too unstable - `Document.Range` is removed because it is unnecessary - Bounds checking is removed from `Document` With the introduction of save actions some open tickets that document problems in combination with the JDT save action feature can be marked as fixed because JDT is no longer involved in the Scala editor with its save actions. Re #1000499 Fixes #1000900 Fixes #1000887 Fixes #1001138
|
I solved most points (see last commit), but not yet the Undo behavior. It turned out to be more difficult to do this in general. Also, I found a NPE that sometimes occurs but I couldn't find out so far why it happens. It somehow occurs when switching between Scala editors. Open points:
|
|
@sschaef Am I correct that you wish this PR to be merged before quick fixes? |
|
No afterwards. Quick fixes don't work anymore once this PR is merged and fixing the quick fix PR first means that we can verify that it is really possible to fix them... |
|
Test FAILed. |
|
Actually, if the number of undo actions that one needs to completely revert save actions is equal to the number of enabled save actions, I don't think it's a huge deal. It would be 1, 2 or maximum three, but I don't see why would anyone enable all three of them ( |
|
(ignore the test failure, that's what I was trying to fix in #828) |
|
Actually it is worse than that. I found out that the merged text changes is not a real merged text change, it is just a wrapper for multiple changes. Therefore we still have as many undos as text changes. |
|
I just peeked at how the Scala formatter does it:
|
|
Ok, I'll have a look. |
Each changed region creates its own undo entry in the undo queue. This means that one has to undo each text change separately after save actions are invoked instead of only a single undo. Fortunately it is possible to tell the undo queue that multiple undos belong together.
Conflicts: org.scala-ide.sdt.core/plugin.xml org.scala-ide.sdt.core/src/org/scalaide/util/eclipse/EclipseUtils.scala
It is no longer possible to retrieve compilation units from JDT classes, we have to use the definitions in `IScalaPlugin` and `ScalaPlugin`.
|
Good news: It's done! The Undo thing and quick assists work fine. There is just the NPE left but I have still no clue on how to reproduce it. How awesome that this is just RC1... |
I'm not sure why this exception occurs but I guess that whenever an editor is closed, its compilation unit is removed but the job for the update occurrences feature may still be running. We just need to check if null is returned and do nothing in this case.
|
NPE fixed. The only important thing that is missing now is documentation. |
|
What do you think about removing the refactoring-based tests until we iron out the details? |
They are unstable
|
Yeah, seems to be better. I make a party the day when I find out why exactly all these compiler tests fail. |
|
I'll buy the drinks for that party ;-) Great work Simon! |
|
Test PASSed. See Console Output in the link below for an update site containing this PR binary artefacts. Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-ide-validator/1227/ |
|
It looks good! Great work, Simon! It's still a bit slow, but I don't know if we can fix that... I timed different parts of the implementation and almost all the time is spent in I don't think that's a blocker for RC1, though! I'd just like to keep this in mind, and try to fix it for the final release. Again, fantastic work and thanks for the dedication to pull this through! |
|
One idea. Documents should have a 'working copy' system. It might speed thing up to do it on a working copy, and to apply it to the real document. |
|
Yes, updating the UI shouldn't happen when it is not necessary. There is still some time before the final release, I'll find a way to make it more performant. |
This is not yet completely finished but because I already used it in production and it worked great so far I want to give you the possibility to play with it around as early as possible. In the following a short overview about what is included in this PR.
Features included:
Scala → Editor → Save actionswhere the save actions can be enabled/disabled:Design overview:
org.scalaide.extensionscontains new API+save actionsorg.scalaide.core.textalso contains new APIorg.scalaide.core.internal.{text,extensions}contains implementations of the APIorg.scalaide.ui.internal.editorcontains additions needed for save actions.trait X extends SaveAction with {CompilerSupport,DocumentSupport}SaveActionExtensions.saveActionSettingsand in one ofSaveActionExtensions#{applyDocumentExtensions,applyCompilerExtensions}Drawbacks/TODOs:
ScalaDocumentProviderbroke several thingsSemanticHighlightingReconciliation)ScalaToggleBreakpointAdapter)BaseSemanticAction.refreshJavaPlugin.getDefault.getWorkingCopyManager.getWorkingCopyEditorUtility.openInEditorcreatesInternalClassFileEditorInput, whosegetAdaptermethod is broken → it needs to aScalaClassFile.AddMissingOverride:AddReturnTypeToPublicSymbols: