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.

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

Auto edits #886

Merged
merged 136 commits into from
Jun 12, 2015
Merged

Auto edits #886

merged 136 commits into from
Jun 12, 2015

Conversation

kiritsuku
Copy link
Member

Auto edits are the new way to support users while typing (and therefore replace the existing auto edit strategies (subtypes of IAutoEditStrategy) borrowed from Eclipse). This work is mostly finished and I would like to get review for it to see if it works for others and not just for me.

New key features are:

  • Support for Eclipse' linked mode model
  • Auto expand templates by pressing tab (see ApplyTemplate)
  • Auto close curly braces to surround a single expression (see SurroundBlock)
  • Easier way to enable/disable auto edits due to a new configuration page

More in depth explanations about design choices: https://groups.google.com/forum/#!searchin/scala-ide-dev/auto$20edits/scala-ide-dev/HbF1B5GkluA/ZLpu__Rzeg4J

Todos:

  • More Scaladoc for the API needs to be written
  • The table in the preference page doesn't grow correctly (scroll bar missing)
  • Lost functionality: Move braces to correct position. It should be reintroduced.
  • Fix 1002133
  • when multiple linked models are created (which happens when multiple open parens are typed for example) it is not possible to use the tab key to navigate to "outer" linked models
  • Templates with empty patterns (${}) need to be optimized away by the underlying processor, otherwise they can get in the way in special situations
  • Fix 1002379
  • Convert AutoIndentStrategy to an auto edit
  • Convert CommentAutoIndentStrategy to an auto edit
  • Convert LiteralAutoEditStrategy to an auto edit
  • Convert MultiLineStringAutoEditStrategy to an auto edit
  • Convert MultiLineStringAutoIndentStrategy to an auto edit
  • Convert StringAutoEditStrategy to an auto edit
  • Convert SurroundSelectionStrategy to an auto edit
  • Convert TabsToSpacesConverter to an auto edit (or use the save action instead)
  • Fix 1002463

The previous PR with some more info is #770.

We need a way to access existing Scala compilation units in several
parts of the IDE. Therefore the document provider is moved to the Scala
plugin in order to act as a compilation unit cache. The Java plugin
works similar and has with the WorkingCopyManager a class that provides
such a cache, but because this class is statically configured in the
Java plugin we can't override with our own implementation.

Therefore, all calls to

    JavaPlugin.getDefault.getWorkingCopyManager.getWorkingCopyManager

need to be replaced with

    ScalaPlugin.plugin.scalaCompilationUnit

We can do these change in our own codebase, but there is the risk that
code inherited from JDT also calls accesses the working copy manager and
in these cases we have to override such implementations in order to get
the code up and running again.
The apply method now creates the best fit text change object and not
only an `Replace` object.
The copy method considers forwards to this apply method to keep the
behavior.
This adds a new `Change` object to pass an updated cursor position from
an auto edit to the logic that applies it.

This also adds some syntax sugar for auto edits to operate on the input.

The return type of the auto edit has changed to an `Option` because it
is possible that an auto edit can not be applied. Previously the input
change object had to be returned to describe this situation but
returning an `Option` makes more sense.
Auto edits should be able to react on everything that is pasted into
the editor. If for example the "=>" symbol is pasted, the auto edit can
now handle such an input and convert it to its unicode counterpart.
It turned out that this dependency makes it more complicated than it
should be. The text viewer should handle all changes by itself, the auto
edit logic should just tell him what to do.
If no change is returned the already existing text change needs to be
used.
This auto edit always moves the semicolon to the end of the line unless
the cursor is inside a for comprehension - the only place in Scala where
the usage of semicolons is a syntax feature.

The "auto insert braces at corret position" feature of the Java editor
is no longer supported by the Scala editor because there are very few
cases where this feature really makes sense. Most of the time one can
use a template to generate code where the opening brace is at the
correct position.

Fixes #1002056
This auto edit strategy is replaced by the `SmartSemicolonInsertion`
auto edit.
This allows to write tests that check if the removal of text works
correctly.
This auto edit is partly a replacement for `BracketAutoEditStrategy`.
This function may be useful to write the auto edit in a DSL style.
Another step towards replacing `BracketAutoEditStrategy`.
This completes the replacement of `BracketAutoEditStrategy`.
Its functionality was replaced by the auto edits `CloseCurlyBrace`,
`JumpOverClosingCurlyBrace` and `RemoveCurlyBracePair`.
This allows all auto edits to use the linked mode model and to test its
behavior in the test suite.
Moving the content out of this file means that we can use it for other
auto edits.
They need to be available very early in initialization of Scala IDE (due
to the fact that they are accessed in preference initializers). By
moving them to their own object that does not have further dependencies
we ensure that we don't get into any initialization issues. Previously
the logger was not available because its initialization was not yet
completed at the point where save actions or auto edits were
initialized.
As it turned out this code is not needed. The compiler already has
access to the classpath of the entire Eclipse platform, thanks to the
custom class loader that is defined in `ExtensionCompiler`.
@kiritsuku
Copy link
Member Author

I now killed all the *Creator types and instead implemented a mechanism that allows the dynamic loading of the extensions at runtime. The code is more complex now but also more powerful.

Conflicts:
	org.scala-ide.sdt.core.tests/src/org/scalaide/core/ui/UITestSuite.scala
@Kwestor
Copy link
Member

Kwestor commented Jun 1, 2015

It looks ok, but I don't like the idea of merging 128 commits to master for one feature. Do you think it is possible to squash it a bit?

@kiritsuku
Copy link
Member Author

I'm not a fan of squashing, the commit history contains useful information. Also, it is difficult to do here, because this branch contains a lot of merge commits.

This reverts commit 7411d72.

I was wrong, we indeed need this code code.
A different classpath is required when the IDE is used in development
mode.
@kiritsuku
Copy link
Member Author

@wpopielarski @Kwestor would one of you please check if the latest changes of this PR work on Windows? the first time after something is edited in the editor or the editor is saved, you should see log messages, saying that extensions are compiler (+ a little delay in the editor). After that on every further invocation of the extension, it should be loaded from a cache, that is located in ~/.scalaide/classes.

`getActiveWorkbench` can return null.

Fixes #1002485
See d4b8704 for a similar exception.

Fixes #1002484
`getLocation` shouldn't be used, it is not always correct. Furthermore,
it is cumbersome to work with it. `FileLocator.getBundleFile` is used
instead, it returns a `java.io.File` that points to an existing file.

Furthermore, more logging is enabled. This makes it much easier in
future if any errors occur.
@ghprb-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins.scala-ide.org:8496/jenkins//job/ghprb-scala-ide-multi-conf/207/
Test PASSed.

@kiritsuku
Copy link
Member Author

I tested it on Windows by myself and I found some minor issues, which I all solved. Everything works now on both Linux and Windows, Mac should work fine too.

I merge as is without further, more precise code review, otherwise it takes another month until this PR gets through.

kiritsuku added a commit that referenced this pull request Jun 12, 2015
@kiritsuku kiritsuku merged commit de38efc into scala-ide:master Jun 12, 2015
@kiritsuku kiritsuku deleted the auto-edits branch June 12, 2015 18:29
override def performOk(): Boolean = {
changes foreach { autoEdit =>
val previousValue = prefStore.getBoolean(autoEdit.id)
prefStore.setValue(autoEdit.id, !previousValue)
Copy link
Member

Choose a reason for hiding this comment

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

You should always save the state in the control. No need to invert the existing boolean value, it's easy to get out of sync, and even if you think that shouldn't happen, it's still unnecessarily stateful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants