From b0cc1c7573544e5c8861dab6044fc1b65a5b31f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Sch=C3=A4fer?= Date: Mon, 29 Aug 2016 18:32:49 +0200 Subject: [PATCH 1/4] Remove annotations that belong to a removed range of an editor When text is removed from an editor the annotations that belong to the removed range need to be removed too because they are no longer valid. This is especially useful functionality when the user fixes an error. In such a case, the builder did already create an annotation previously that needs to be removed. With this change, it can happen that errors are removed which were not yet fixed. In most cases, the PC will find them once again and show them to users. In rare cases, the builder needs to be run to find all problems. This is not optimal behavior but probably the best what we can get in Scala because we can't run the builder on every source change, it is simly to slow. Fix #1002726 --- .../editor/DecoratedInteractiveEditor.scala | 25 ++++++++++++++----- .../ScalaSourceViewerConfiguration.scala | 2 +- .../ScalaReconcilingStrategy.scala | 19 ++++++++++---- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala index 95481e6bb4..e50426dfd5 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala @@ -1,14 +1,16 @@ package org.scalaide.ui.editor -import org.eclipse.jface.text.source.Annotation -import org.eclipse.jface.text.source.IAnnotationModel -import org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.ProblemAnnotation +import scala.collection.JavaConverters._ import scala.collection.breakOut + +import org.eclipse.jdt.core.ICompilationUnit import org.eclipse.jdt.core.compiler.IProblem -import org.scalaide.util.internal.eclipse.AnnotationUtils.RichModel -import org.eclipse.jface.text.Position +import org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.ProblemAnnotation import org.eclipse.jface.text.ITextViewerExtension2 -import org.eclipse.jdt.core.ICompilationUnit +import org.eclipse.jface.text.Position +import org.eclipse.jface.text.source.Annotation +import org.eclipse.jface.text.source.IAnnotationModel +import org.scalaide.util.internal.eclipse.AnnotationUtils._ import org.scalaide.util.ui.DisplayThread trait DecoratedInteractiveEditor extends ISourceViewerEditor { @@ -18,6 +20,17 @@ trait DecoratedInteractiveEditor extends ISourceViewerEditor { private var previousAnnotations = List[Annotation]() + /** + * This removes all annotations in the region between `start` and `end`. + */ + def removeAnnotationsInRegion(start: Int, end: Int): Unit = annotationModel foreach { model ⇒ + val annsToRemove = model.getAnnotationIterator.asScala.filter { ann ⇒ + val pos = model.getPosition(ann) + pos.offset >= start && pos.offset+pos.length <= end + } + model.deleteAnnotations(annsToRemove.toSeq) + } + /** * Update annotations on the editor from a list of IProblems */ diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/ScalaSourceViewerConfiguration.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/ScalaSourceViewerConfiguration.scala index 915057186a..bc50ffdbad 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/ScalaSourceViewerConfiguration.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/ScalaSourceViewerConfiguration.scala @@ -254,7 +254,7 @@ class ScalaSourceViewerConfiguration( else baseReconcilers) - val reconciler = new ScalaReconciler(editor, s, isIncremental = false) + val reconciler = new ScalaReconciler(editor, s, isIncremental = true) reconciler.setDelay(IScalaPlugin().getPreferenceStore.getInt(ScalaPreferences.ReconcilerDelayId)) reconciler.setProgressMonitor(new NullProgressMonitor()) reconciler diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala index a15c852e88..d2d9589146 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala @@ -2,13 +2,13 @@ package org.scalaide.ui.internal.reconciliation import org.eclipse.core.runtime.IProgressMonitor import org.eclipse.core.runtime.NullProgressMonitor -import org.scalaide.logging.HasLogger +import org.eclipse.jdt.core.ICompilationUnit +import org.eclipse.jdt.internal.ui.text.java.IJavaReconcilingListener import org.eclipse.jface.text._ +import org.eclipse.jface.text.reconciler.DirtyRegion import org.eclipse.jface.text.reconciler.IReconcilingStrategy import org.eclipse.jface.text.reconciler.IReconcilingStrategyExtension -import org.eclipse.jface.text.reconciler.DirtyRegion -import org.eclipse.jdt.internal.ui.text.java.IJavaReconcilingListener -import org.eclipse.jdt.core.ICompilationUnit +import org.scalaide.logging.HasLogger import org.scalaide.ui.editor.InteractiveCompilationUnitEditor import org.scalaide.util.Utils._ @@ -35,7 +35,16 @@ class ScalaReconcilingStrategy(icuEditor: InteractiveCompilationUnitEditor) exte override def setProgressMonitor(pMonitor: IProgressMonitor): Unit = {} override def reconcile(dirtyRegion: DirtyRegion, subRegion: IRegion): Unit = { - logger.debug("Incremental reconciliation not implemented.") + dirtyRegion.getType match { + case DirtyRegion.INSERT ⇒ + case DirtyRegion.REMOVE ⇒ + import org.scalaide.util.eclipse.RegionUtils._ + // when Eclipse gains focus, two regions are created. The first one + // removes the entire file content, the second one adds it. We don't + // want to catch this remove region here. + if (!(dirtyRegion.start == 0 && dirtyRegion.length == icUnit.getContents().length)) + icuEditor.removeAnnotationsInRegion(dirtyRegion.start, dirtyRegion.end) + } } override def reconcile(partition: IRegion): Unit = { From 81d447bd5c81c245e925ab4c3134d292fb0e2366 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Sch=C3=A4fer?= Date: Mon, 29 Aug 2016 19:09:35 +0200 Subject: [PATCH 2/4] Cleanups in DecoratedInteractiveEditor --- .../editor/DecoratedInteractiveEditor.scala | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala index e50426dfd5..9b4db4ae1f 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala @@ -26,7 +26,7 @@ trait DecoratedInteractiveEditor extends ISourceViewerEditor { def removeAnnotationsInRegion(start: Int, end: Int): Unit = annotationModel foreach { model ⇒ val annsToRemove = model.getAnnotationIterator.asScala.filter { ann ⇒ val pos = model.getPosition(ann) - pos.offset >= start && pos.offset+pos.length <= end + pos.offset >= start && pos.offset + pos.length <= end } model.deleteAnnotations(annsToRemove.toSeq) } @@ -35,7 +35,7 @@ trait DecoratedInteractiveEditor extends ISourceViewerEditor { * Update annotations on the editor from a list of IProblems */ def updateErrorAnnotations(errors: List[IProblem], cu: ICompilationUnit): Unit = annotationModel foreach { model ⇒ - val newAnnotations: Map[Annotation, Position] = (for (e <- errors) yield { + val newAnnotations: Map[Annotation, Position] = (for (e ← errors) yield { val annotation = new ProblemAnnotation(e, cu) // no compilation unit val position = new Position(e.getSourceStart, e.getSourceEnd - e.getSourceStart + 1) (annotation, position) @@ -48,15 +48,19 @@ trait DecoratedInteractiveEditor extends ISourceViewerEditor { // http://stackoverflow.com/questions/12507620/race-conditions-in-annotationmodel-error-annotations-lost-in-reconciler val presViewer = getViewer if (presViewer.isInstanceOf[ITextViewerExtension2]) { - // TODO: This should be replaced by a better modularization of semantic highlighting PositionsChange - val newPositions = newAnnotations.values - def end (x:Position) = x.offset + x.length - 1 - val taintedBounds : (Int, Int) = ((Int.MaxValue, 0) /: newPositions) {(acc, p1) => (Math.min(acc._1, p1.offset), Math.max(acc._2, end(p1)))} - val taintedLength = (taintedBounds._2 - taintedBounds._1 +1) + // TODO: This should be replaced by a better modularization of semantic highlighting PositionsChange + val newPositions = newAnnotations.values + def end(x: Position) = x.offset + x.length - 1 + val taintedBounds = (newPositions foldLeft (Int.MaxValue, 0)) { (acc, p1) ⇒ (Math.min(acc._1, p1.offset), Math.max(acc._2, end(p1))) } + val taintedLength = (taintedBounds._2 - taintedBounds._1 + 1) - DisplayThread.asyncExec { presViewer.asInstanceOf[ITextViewerExtension2].invalidateTextPresentation(taintedBounds._1, taintedLength) } + DisplayThread.asyncExec { + presViewer.asInstanceOf[ITextViewerExtension2].invalidateTextPresentation(taintedBounds._1, taintedLength) + } } else { - DisplayThread.asyncExec { getViewer.invalidateTextPresentation() } + DisplayThread.asyncExec { + getViewer.invalidateTextPresentation() + } } } From 3f3cf526d86c69fd9164fb036ad3648fab6f1b4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Sch=C3=A4fer?= Date: Tue, 30 Aug 2016 15:49:39 +0200 Subject: [PATCH 3/4] Apply reviewer comments about changes in DecoratedInteractiveEditor --- .../editor/DecoratedInteractiveEditor.scala | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala index 9b4db4ae1f..e876bb56db 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/editor/DecoratedInteractiveEditor.scala @@ -9,14 +9,14 @@ import org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.Pr import org.eclipse.jface.text.ITextViewerExtension2 import org.eclipse.jface.text.Position import org.eclipse.jface.text.source.Annotation -import org.eclipse.jface.text.source.IAnnotationModel +import org.eclipse.jface.text.source.IAnnotationModelExtension2 import org.scalaide.util.internal.eclipse.AnnotationUtils._ import org.scalaide.util.ui.DisplayThread trait DecoratedInteractiveEditor extends ISourceViewerEditor { /** Return the annotation model associated with the current document. */ - private def annotationModel = Option(getDocumentProvider).map(_.getAnnotationModel(getEditorInput).asInstanceOf[IAnnotationModel]) + private def annotationModel = Option(getDocumentProvider).map(_.getAnnotationModel(getEditorInput)) private var previousAnnotations = List[Annotation]() @@ -24,9 +24,14 @@ trait DecoratedInteractiveEditor extends ISourceViewerEditor { * This removes all annotations in the region between `start` and `end`. */ def removeAnnotationsInRegion(start: Int, end: Int): Unit = annotationModel foreach { model ⇒ - val annsToRemove = model.getAnnotationIterator.asScala.filter { ann ⇒ - val pos = model.getPosition(ann) - pos.offset >= start && pos.offset + pos.length <= end + val annsToRemove = model match { + case model: IAnnotationModelExtension2 ⇒ + model.getAnnotationIterator(start, end - start, /*canStartBefore*/ false, /*canEndAfter*/ false).asScala + case _ ⇒ + model.getAnnotationIterator.asScala.filter { ann ⇒ + val pos = model.getPosition(ann) + pos.offset >= start && pos.offset + pos.length <= end + } } model.deleteAnnotations(annsToRemove.toSeq) } @@ -46,21 +51,21 @@ trait DecoratedInteractiveEditor extends ISourceViewerEditor { // This shouldn't be necessary in @dragos' opinion. But see #84 and // http://stackoverflow.com/questions/12507620/race-conditions-in-annotationmodel-error-annotations-lost-in-reconciler - val presViewer = getViewer - if (presViewer.isInstanceOf[ITextViewerExtension2]) { - // TODO: This should be replaced by a better modularization of semantic highlighting PositionsChange - val newPositions = newAnnotations.values - def end(x: Position) = x.offset + x.length - 1 - val taintedBounds = (newPositions foldLeft (Int.MaxValue, 0)) { (acc, p1) ⇒ (Math.min(acc._1, p1.offset), Math.max(acc._2, end(p1))) } - val taintedLength = (taintedBounds._2 - taintedBounds._1 + 1) + getViewer match { + case viewer: ITextViewerExtension2 ⇒ + // TODO: This should be replaced by a better modularization of semantic highlighting PositionsChange + val newPositions = newAnnotations.values + def end(x: Position) = x.offset + x.length - 1 + val taintedBounds = (newPositions foldLeft (Int.MaxValue, 0)) { (acc, p1) ⇒ (Math.min(acc._1, p1.offset), Math.max(acc._2, end(p1))) } + val taintedLength = (taintedBounds._2 - taintedBounds._1 + 1) - DisplayThread.asyncExec { - presViewer.asInstanceOf[ITextViewerExtension2].invalidateTextPresentation(taintedBounds._1, taintedLength) - } - } else { - DisplayThread.asyncExec { - getViewer.invalidateTextPresentation() - } + DisplayThread.asyncExec { + viewer.invalidateTextPresentation(taintedBounds._1, taintedLength) + } + case viewer ⇒ + DisplayThread.asyncExec { + viewer.invalidateTextPresentation() + } } } From b25774f3b6d4771c59481e921c46bd2bab2b7bcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Sch=C3=A4fer?= Date: Tue, 30 Aug 2016 15:51:53 +0200 Subject: [PATCH 4/4] Move reconciliation tasks to new incremental reconciliation method It is not possible to call both the incremental and the non-incremental method, therefore we had to move the implementation of the non-incremental method to the incremental one. --- .../ScalaOutlineReconcilingStrategy.scala | 19 +++++---- .../spelling/SpellingReconcileStrategy.scala | 11 +++--- .../ScalaReconcilingStrategy.scala | 39 ++++++++++++------- 3 files changed, 41 insertions(+), 28 deletions(-) diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/outline/ScalaOutlineReconcilingStrategy.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/outline/ScalaOutlineReconcilingStrategy.scala index c868c1cb4c..2895f8680f 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/outline/ScalaOutlineReconcilingStrategy.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/outline/ScalaOutlineReconcilingStrategy.scala @@ -16,11 +16,19 @@ class ScalaOutlineReconcilingStrategy(icuEditor: OutlinePageEditorExtension) ext override def setProgressMonitor(pMonitor: IProgressMonitor): Unit = {} override def reconcile(dirtyRegion: DirtyRegion, subRegion: IRegion): Unit = { - logger.debug("Incremental reconciliation not implemented.") + updateOutlineView() } override def reconcile(partition: IRegion): Unit = { - val sop = Option(icuEditor.getOutlinePage) + logger.debug("Non incremental reconciliation not implemented.") + } + + override def initialReconcile(): Unit = { + updateOutlineView() + } + + private def updateOutlineView(): Unit = { + val sop = Option(icuEditor.getOutlinePage) if (!sop.isEmpty) { val oldRoot = sop.get.getInput icUnit.scalaProject.presentationCompiler.apply(comp => { @@ -34,9 +42,4 @@ class ScalaOutlineReconcilingStrategy(icuEditor: OutlinePageEditorExtension) ext }) } } - - override def initialReconcile(): Unit = { - reconcile(null) - } - -} \ No newline at end of file +} diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/spelling/SpellingReconcileStrategy.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/spelling/SpellingReconcileStrategy.scala index 354818f625..3502c3a7a0 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/spelling/SpellingReconcileStrategy.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/editor/spelling/SpellingReconcileStrategy.scala @@ -7,13 +7,14 @@ import org.eclipse.jdt.internal.ui.text.spelling.JavaSpellingProblem import org.eclipse.jface.preference.IPreferenceStore import org.eclipse.jface.text.IDocument import org.eclipse.jface.text.IRegion +import org.eclipse.jface.text.reconciler.DirtyRegion import org.eclipse.jface.text.source.IAnnotationModel import org.eclipse.jface.text.source.ISourceViewer import org.eclipse.ui.texteditor.ITextEditor import org.eclipse.ui.texteditor.spelling.ISpellingProblemCollector import org.eclipse.ui.texteditor.spelling.SpellingProblem -import org.eclipse.ui.texteditor.spelling.{SpellingReconcileStrategy => ESpellingReconcileStrategy} -import org.eclipse.ui.texteditor.spelling.{SpellingService => ESpellingService} +import org.eclipse.ui.texteditor.spelling.{ SpellingReconcileStrategy => ESpellingReconcileStrategy } +import org.eclipse.ui.texteditor.spelling.{ SpellingService => ESpellingService } /** * Reconcile strategy for spell checking. It checks whether spell checking is @@ -22,7 +23,7 @@ import org.eclipse.ui.texteditor.spelling.{SpellingService => ESpellingService} * The implementation of this class is adopted from * [[JavaSpellingReconcileStrategy]], which couldn't be used because it sets the * spelling service in its constructor. The spelling service is the only thing - * we had to modify to enable a Scala spelling engine, for a more detailful + * we had to modify to enable a Scala spelling engine, for a more detailed * description see [[ScalaSpellingService]]. * * @param editor @@ -55,10 +56,10 @@ final class SpellingReconcileStrategy( super.setDocument(document) } - override def reconcile(region: IRegion): Unit = { + override def reconcile(dirtyRegion: DirtyRegion, subRegion: IRegion): Unit = { val isSpellingEnabled = store.getBoolean(ESpellingService.PREFERENCE_SPELLING_ENABLED) if (requestor.isDefined && isSpellingEnabled) - super.reconcile(region) + super.reconcile(dirtyRegion, subRegion) } override def getAnnotationModel(): IAnnotationModel = diff --git a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala index d2d9589146..17bd520aae 100644 --- a/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala +++ b/org.scala-ide.sdt.core/src/org/scalaide/ui/internal/reconciliation/ScalaReconcilingStrategy.scala @@ -35,21 +35,24 @@ class ScalaReconcilingStrategy(icuEditor: InteractiveCompilationUnitEditor) exte override def setProgressMonitor(pMonitor: IProgressMonitor): Unit = {} override def reconcile(dirtyRegion: DirtyRegion, subRegion: IRegion): Unit = { - dirtyRegion.getType match { - case DirtyRegion.INSERT ⇒ - case DirtyRegion.REMOVE ⇒ - import org.scalaide.util.eclipse.RegionUtils._ - // when Eclipse gains focus, two regions are created. The first one - // removes the entire file content, the second one adds it. We don't - // want to catch this remove region here. - if (!(dirtyRegion.start == 0 && dirtyRegion.length == icUnit.getContents().length)) - icuEditor.removeAnnotationsInRegion(dirtyRegion.start, dirtyRegion.end) - } + removeInvalidAnnotations(dirtyRegion) + handleReconciliation() } override def reconcile(partition: IRegion): Unit = { + logger.debug("Non incremental reconciliation not implemented.") + } + + override def initialReconcile(): Unit = { + // an askReload there adds the scUnit to the list of managed CUs + icUnit.initialReconcile() + handleReconciliation() + } + + private def handleReconciliation(): Unit = { listeningEditor.foreach(_.aboutToBeReconciled()) val errors = icUnit.forceReconcile() + println(errors) // Some features, such as quick fixes, are dependent upon getting an ICompilationUnit there val cu: Option[ICompilationUnit] = icUnit.asInstanceOfOpt[ICompilationUnit] @@ -62,10 +65,16 @@ class ScalaReconcilingStrategy(icuEditor: InteractiveCompilationUnitEditor) exte listeningEditor.foreach(_.reconciled(null, false, new NullProgressMonitor())) } - override def initialReconcile(): Unit = { - // an askReload there adds the scUnit to the list of managed CUs - icUnit.initialReconcile() - reconcile(null) + private def removeInvalidAnnotations(dirtyRegion: DirtyRegion): Unit = { + dirtyRegion.getType match { + case DirtyRegion.INSERT ⇒ + case DirtyRegion.REMOVE ⇒ + import org.scalaide.util.eclipse.RegionUtils._ + // when Eclipse gains focus, two regions are created. The first one + // removes the entire file content, the second one adds it. We don't + // want to catch this remove region here. + if (!(dirtyRegion.start == 0 && dirtyRegion.length == icUnit.getContents().length)) + icuEditor.removeAnnotationsInRegion(dirtyRegion.start, dirtyRegion.end) + } } - }