Skip to content

Commit

Permalink
Apply reviewer comments
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
kiritsuku committed Oct 9, 2014
1 parent 3fa5598 commit b9e88e3
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 91 deletions.
@@ -1,9 +1,11 @@
package org.scalaide.core.text

import org.eclipse.jface.text.BadLocationException
import org.eclipse.jface.text.{Document => EDocument}
import org.junit.ComparisonFailure
import org.junit.Test
import org.eclipse.jface.text.{Document => EDocument}
import org.scalaide.core.internal.text.TextDocument
import org.scalaide.util.eclipse.RegionUtils._

class DocumentTest {

Expand Down Expand Up @@ -45,21 +47,21 @@ class DocumentTest {
def textRange_returns_range() =
"hello world" becomes "lo wo" after (_.textRange(3, 8))

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def textRange_throws_when_start_lower_than_zero() = {
val d = document("hello world")
d.textRange(-1, 8)
()
}

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def textRange_throws_when_end_lower_than_start() = {
val d = document("hello world")
d.textRange(5, 4)
()
}

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def textRange_throws_when_end_greater_than_length() = {
val d = document("hello world")
d.textRange(5, 20)
Expand Down Expand Up @@ -97,12 +99,12 @@ class DocumentTest {
|def
|
|gh""".stripMargin)
d.lines === Seq(
d.Range(0,1),
d.Range(2,4),
d.Range(5,8),
d.Range(9,9),
d.Range(10,12))
d.lines.map(r => (r.start, r.end)) === Seq(
(0,1),
(2,4),
(5,8),
(9,9),
(10,12))
}

@Test
Expand All @@ -117,37 +119,37 @@ class DocumentTest {

@Test
def trim_trims_whitespace() =
" \t hello \t " becomes "hello" after (_.lineInformation(0).trim.text)
" \t hello \t " becomes "hello" after (d => d.lineInformation(0).trim(d).text(d))

@Test
def trim_trims_nothing_when_there_is_no_whitespace() =
"hello" becomes "hello" after (_.lineInformation(0).trim.text)
"hello" becomes "hello" after (d => d.lineInformation(0).trim(d).text(d))

@Test
def trimLeft_trims_left_whitespace() =
" \t hello \t " becomes "hello \t " after (_.lineInformation(0).trimLeft.text)
" \t hello \t " becomes "hello \t " after (d => d.lineInformation(0).trimLeft(d).text(d))

@Test
def trimLeft_trims_nothing_when_there_is_no_whitespace() =
"hello \t " becomes "hello \t " after (_.lineInformation(0).trimLeft.text)
"hello \t " becomes "hello \t " after (d => d.lineInformation(0).trimLeft(d).text(d))

@Test
def trimRight_trims_right_whitespace() =
" \t hello \t " becomes " \t hello" after (_.lineInformation(0).trimRight.text)
" \t hello \t " becomes " \t hello" after (d => d.lineInformation(0).trimRight(d).text(d))

@Test
def trimRight_trims_nothing_when_there_is_no_whitespace() =
" \t hello" becomes " \t hello" after (_.lineInformation(0).trimRight.text)
" \t hello" becomes " \t hello" after (d => d.lineInformation(0).trimRight(d).text(d))

@Test
def apply_on_non_empty_file_succeeds() =
document("some text").apply(3) === 'e'

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def apply_thrown_when_index_lower_than_zero(): Unit =
document("some text").apply(-1)

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def apply_thrown_when_index_greater_equal_than_length(): Unit =
document("some text").apply(9)

Expand Down Expand Up @@ -183,35 +185,35 @@ class DocumentTest {
def lastOpt_on_non_empty_file_succeeds() =
document("some text").lastOpt === Some('t')

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def head_on_empty_file_throws(): Unit =
document("").head

@Test
def headOpt_on_empty_file_returns_none() =
document("").headOpt === None

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def tail_on_empty_file_throws(): Unit =
document("").tail

@Test
def tailOpt_on_empty_file_returns_none() =
document("").tailOpt === None

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def init_on_empty_file_throws(): Unit =
document("").init

@Test
def initOpt_on_empty_file_returns_none() =
document("").initOpt === None

@Test(expected = classOf[IndexOutOfBoundsException])
@Test(expected = classOf[BadLocationException])
def last_on_empty_file_throws(): Unit =
document("").last

@Test
def lastOpt_on_empty_file_returns_none() =
document("").lastOpt === None
}
}
@@ -1,16 +1,15 @@
package org.scalaide.core.internal.text

import org.eclipse.jface.text.IDocument
import org.eclipse.jface.text.IRegion
import org.eclipse.jface.text.Region
import org.scalaide.core.text.Document
import org.scalaide.core.text.InternalDocument

class TextDocument(private val doc: IDocument) extends Document with InternalDocument {

override def apply(i: Int): Char =
if (i >= 0 && i < length)
doc.getChar(i)
else
throw new IndexOutOfBoundsException
doc.getChar(i)

override def length: Int =
doc.getLength()
Expand All @@ -19,36 +18,28 @@ class TextDocument(private val doc: IDocument) extends Document with InternalDoc
doc.get()

override def textRange(start: Int, end: Int): String =
if (isValidRange(start, end))
doc.get(start, end-start)
else
throw new IndexOutOfBoundsException
doc.get(start, end-start)

override def textRangeOpt(start: Int, end: Int): Option[String] =
if (isValidRange(start, end))
Some(doc.get(start, end-start))
else
None

override def lines: Seq[Range] =
override def lines: Seq[IRegion] =
0 until lineCount map lineInformation

override def lineCount: Int =
doc.getNumberOfLines()

override def lineInformation(lineNumber: Int): Range = {
val l = doc.getLineInformation(lineNumber)
Range(l.getOffset(), l.getOffset()+l.getLength())
}
override def lineInformation(lineNumber: Int): IRegion =
doc.getLineInformation(lineNumber)

override def replace(start: Int, end: Int, text: String): Unit =
doc.replace(start, end-start, text)

override def head: Char =
if (!isEmpty)
doc.getChar(0)
else
throw new IndexOutOfBoundsException
doc.getChar(0)

override def headOpt: Option[Char] =
if (!isEmpty)
Expand All @@ -57,10 +48,7 @@ class TextDocument(private val doc: IDocument) extends Document with InternalDoc
None

override def tail: String =
if (!isEmpty)
doc.get(1, length-1)
else
throw new IndexOutOfBoundsException
doc.get(1, length-1)

override def tailOpt: Option[String] =
if (!isEmpty)
Expand All @@ -69,10 +57,7 @@ class TextDocument(private val doc: IDocument) extends Document with InternalDoc
None

override def init: String =
if (!isEmpty)
doc.get(0, length-1)
else
throw new IndexOutOfBoundsException
doc.get(0, length-1)

override def initOpt: Option[String] =
if (!isEmpty)
Expand All @@ -81,10 +66,7 @@ class TextDocument(private val doc: IDocument) extends Document with InternalDoc
None

override def last: Char =
if (!isEmpty)
doc.getChar(length-1)
else
throw new IndexOutOfBoundsException
doc.getChar(length-1)

override def lastOpt: Option[Char] =
if (!isEmpty)
Expand All @@ -97,4 +79,4 @@ class TextDocument(private val doc: IDocument) extends Document with InternalDoc

private def isValidRange(start: Int, end: Int): Boolean =
start >= 0 && start <= end && end <= length
}
}
36 changes: 4 additions & 32 deletions org.scala-ide.sdt.core/src/org/scalaide/core/text/Document.scala
@@ -1,5 +1,7 @@
package org.scalaide.core.text

import org.eclipse.jface.text.IRegion

trait Document {

doc =>
Expand All @@ -14,11 +16,11 @@ trait Document {

def textRangeOpt(start: Int, end: Int): Option[String]

def lines: Seq[Range]
def lines: Seq[IRegion]

def lineCount: Int

def lineInformation(lineNumber: Int): Range
def lineInformation(lineNumber: Int): IRegion

def head: Char

Expand All @@ -35,36 +37,6 @@ trait Document {
def last: Char

def lastOpt: Option[Char]

case class Range(start: Int, end: Int) {
def length: Int = end-start
def text: String = doc.textRange(start, end)

def trim: Range =
trimLeft.trimRight

def trimLeft: Range = {
val s = text
val len = length

var i = 0
while (i < len && Character.isWhitespace(s.charAt(i)))
i += 1

Range(start+i, end)
}

def trimRight: Range = {
val s = text
val len = length

var i = len-1
while (i >= 0 && Character.isWhitespace(s.charAt(i)))
i -= 1

Range(start, start+i+1)
}
}
}

private[core] trait InternalDocument extends Document {
Expand Down
Expand Up @@ -2,6 +2,7 @@ package org.scalaide.extensions
package saveactions

import org.scalaide.core.text.Remove
import org.scalaide.util.eclipse.RegionUtils._

object RemoveDuplicatedEmptyLinesSetting extends SaveActionSetting(
id = ExtensionSetting.fullyQualifiedName[RemoveDuplicatedEmptyLines],
Expand All @@ -22,7 +23,7 @@ trait RemoveDuplicatedEmptyLines extends SaveAction with DocumentSupport {

override def perform() = {
val emptyLines = document.lines.zipWithIndex.filter {
case (l, _) => l.trimRight.length == 0
case (r, _) => r.trimRight(document).length == 0
}

val removedLines = emptyLines.sliding(2) flatMap {
Expand Down
Expand Up @@ -2,6 +2,7 @@ package org.scalaide.extensions
package saveactions

import org.scalaide.core.text.Remove
import org.scalaide.util.eclipse.RegionUtils._

object RemoveTrailingWhitespaceSetting extends SaveActionSetting(
id = ExtensionSetting.fullyQualifiedName[RemoveTrailingWhitespace],
Expand All @@ -22,7 +23,7 @@ trait RemoveTrailingWhitespace extends SaveAction with DocumentSupport {

override def perform() = {
document.lines flatMap { line =>
val trimmed = line.trimRight
val trimmed = line.trimRight(document)

if (trimmed.length != line.length)
Seq(Remove(trimmed.end, line.end))
Expand Down
Expand Up @@ -47,9 +47,7 @@ object SaveActionExtensions {
RemoveTrailingWhitespaceSetting,
AddNewLineAtEndOfFileSetting,
AutoFormattingSetting,
AddMissingOverrideSetting,
RemoveDuplicatedEmptyLinesSetting,
AddReturnTypeToPublicSymbolsSetting
RemoveDuplicatedEmptyLinesSetting
)

/**
Expand Down Expand Up @@ -103,7 +101,6 @@ trait SaveActionExtensions extends HasLogger {
*/
private def applySaveActions(udoc: IDocument): Unit = {
applyDocumentExtensions(udoc)
applyCompilerExtensions(udoc)
}

/**
Expand Down

0 comments on commit b9e88e3

Please sign in to comment.