Scala Installations support stage II : choose your own installation #731

huitseeker · 2014-07-03T08:33:25Z

This is here to help advance review for the second phase of the Scala Installations support

The basic functionality is here, meaning you can choose a scala installation instead of a source level, and add your own scala installation as just a bunch of jars. A few missing items that will be added quickly:

ghprb-bot · 2014-07-03T12:26:32Z

Test PASSed.
Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-ide-validator/847/

dragos · 2014-07-03T12:40:29Z

org.scala-ide.sdt.core/src/org/scalaide/core/internal/containers/ScalaClasspathContainers.scala

+    }
+  }
+
+  class LabelProvider extends org.eclipse.jface.viewers.LabelProvider {


Since I have to nitpick, it would nice to have a pretty icon to go with that text ;-)

dragos · 2014-07-03T13:43:08Z

I didn't look too closely at the preferences code (and the publish-subscribe mechanism), but in my simple tests it worked well.

I noticed that sources are not found for custom installations (not a big problem).

I also noticed that the "Remove" button is not disabled for bundled/default (but it correctly doesn't remove them)

huitseeker · 2014-07-03T15:50:30Z

@dragos The ListViewer under the Scala Installations listing is multi-select by construction. What should the behavior be in case the user tries to press Remove on a multiple selection, which would include both bundled and custom installations ?

dragos · 2014-07-03T15:53:32Z

Do what your conscience dictates :)

huitseeker · 2014-07-03T18:23:52Z

@dragos : what's your test case for sources jars not being found for custom installs ? in particular, what are the source jar file names ?
EDIT: We determined not finding the source jars was because they weren't in the same directory, which is ok because as of now this is unsupported behavior.

ghprb-bot · 2014-07-04T15:48:01Z

Test PASSed.
Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-ide-validator/862/

skyluc · 2014-07-07T13:57:06Z

org.scala-ide.sdt.core/src/org/scalaide/core/internal/containers/ScalaClasspathContainers.scala

+import org.eclipse.jface.viewers.ISelection
+import org.eclipse.jface.viewers.StructuredSelection
+import org.scalaide.core.internal.project.ScalaProject
+import org.scalaide.ui.internal.project.ScalaInstallationChoiceUIProviders


There are a lot of unused imports in this list

huitseeker@f2ad3c7#diff-1

ghprb-bot · 2014-07-08T14:54:29Z

Test FAILed.
Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-ide-validator/869/

dragos · 2014-07-08T15:47:06Z

I'll have to look through the code. In my tests so far, it worked well. I would like to suggest a simpler description string for Scala installations. I think it's very confusing, and I couldn't really parse it easily (see screenshot). I'd like the string in there to be really clear:

Scala 2.11.2-....
Scala 2.10.5.---
...

I think that currently it shows multi-bundle/bundle/legacy and some hash number. Users won't have a clue as to what that is, nor why should they care.

For the dropdown (in preferences and in the classpath container) I'd also like to remove clutter (it's all valuable debugging info, but I don't think hey belong in user-facing strings). They should hopefully be very similar to what there's in Installed Scalas, namely

Scala 2.11 (dynamic)
Scala 2.10 (dynamic)
Scala 2.10.5.-..
Scala 2.11.2.-...

dragos · 2014-07-08T16:01:30Z

org.scala-ide.sdt.core/src/org/scalaide/core/internal/project/ScalaInstallation.scala

+        getName().fold(jarSeq)(str => str +: jarSeq).mkString
+      }
+
+      override def hashCode() = getHashString().hashCode()


You should override equals as well, otherwise this only solves the first step in a hashmap lookup. Equality is still checked when hashcodes match.

huitseeker@1fd190c#diff-b1f294b34b69fd8b9d1fcf63e3812607R126

huitseeker · 2014-07-09T09:54:00Z

@dragos, as of now (huitseeker@6a83af6) the Scala Installations list looks like this :

… Dropdown

Some cleaning: - Project has simpler getDesiredScalaInstallationChoice. - actually it's easy to have Scala installation saving log (harder for reading)

- factory method for directoryScalaInstallations - more logging on install serialization - cleaner Classpath test project - cleaned imports

Tweaks the source jar resolution to be more strict

…ts to preferences.

dragos · 2014-07-11T11:47:51Z

Seems to be working fine at this point, except that I get a dialog for every library that is cross-compiled with a different version. My project has 4 dependencies with _2.10. If I select a 2.11 installation, I get the warning, and suggestion to choose a different scala installation.

I do it once, but then the remaining 3 dependencies (each) trigger a dialog that is out dated (I already changed the Scala installation to a compatible one). I think the dialog should be shown only once, at the end of the classpath check.

dragos · 2014-07-11T12:02:06Z

I'll try to fix it myself, let's see... :)

huitseeker · 2014-07-11T12:31:35Z

@dragos huitseeker@d764529

dragos · 2014-07-11T12:32:41Z

@huitseeker Instead of more mutable state and flags, could we pull that code out of the loop?

dragos · 2014-07-11T12:39:47Z

I can regularly get into the following bad state just by following the dialogs:

Scala Installation: Latest 2.10
Scala classpath container: 2.11
Error markers for libraries cross-compiled with 2.10

I start with a 2.10 install, I switch to 2.11 and hit apply. Then I get the dialog asking me to change the Scala installation (twice). I do it. Everything closes down, but the containers are still on 2.11

dragos · 2014-07-11T13:15:01Z

Here's pulling out the dialog from the loop. I still might get two dialogs, but only because of the explicit call to classpathHasChanged. Turning off listeners might not be enough to ensure you don't get a certain event. I think they might be broadcasted on a different thread, so by the time they fire, you already added back the listener.

Should we just drop all this crazy UI logic that seems to never work well, and do less for the user? Just do whatever he tells us, and don't offer any quick fix dialogs. We could offer smarter quickfixes at the level of error markers, maybe?

--- a/org.scala-ide.sdt.core/src/org/scalaide/core/internal/project/ClasspathManagement.scala
+++ b/org.scala-ide.sdt.core/src/org/scalaide/core/internal/project/ClasspathManagement.scala
@@ -438,36 +438,36 @@ trait ClasspathManagement extends HasLogger { self: ScalaProject =>
     }
   }

-  private var crossCompiledMessageWasShown = new java.util.concurrent.atomic.AtomicBoolean(false)
   private def validateBinaryVersionsOnClasspath(): Seq[(Int, String)] = {
     import scala.concurrent.ExecutionContext.Implicits.global
     import org.scalaide.ui.internal.handlers.BadScalaInstallationPromptStatusHandler

     val entries = scalaClasspath.userCp
     val errors = mutable.ListBuffer[(Int, String)]()
+    var status: IStatus = Status.OK_STATUS

     for (entry <- entries if entry ne null) {
       entry.lastSegment() match {
         case VersionInFile(version) =>
           if (!plugin.isCompatibleVersion(version, this)) {
             val msg = s"${entry.lastSegment()} is cross-compiled with an incompatible version of Scala (${version.unparse}). In case this report is mistaken, this check can be disabled
-            if (!crossCompiledMessageWasShown.getAndSet(true)) {
-             val handlerSuffix = "Configure a Scala Installation for this specific project ?"
-               val status = new Status(IStatus.ERROR, ScalaPlugin.plugin.pluginId, BadScalaInstallationPromptStatusHandler.STATUS_CODE_PREV_CLASSPATH, msg + handlerSuffix, null)
-             try{
-              val handler = DebugPlugin.getDefault().getStatusHandler(status)
-                if (!classpathContinuation.isCompleted) handler.handleStatus(status, (this, classpathContinuation))
-                classpathContinuation.future onSuccess {
-                case f => f()
-                }
-             } finally { classpathContinuation = Promise[() => Unit]; crossCompiledMessageWasShown.set(false)}
-            }
+            val handlerSuffix = "\nConfigure a Scala Installation for this specific project ?"
+            status = new Status(IStatus.ERROR, ScalaPlugin.plugin.pluginId, BadScalaInstallationPromptStatusHandler.STATUS_CODE_PREV_CLASSPATH, msg + handlerSuffix, null)
             errors += ((IMarker.SEVERITY_ERROR, msg))
           }
         case _ =>
           // ignore libraries that aren't cross compiled/are compatible
       }
     }
+
+    if (!status.isOK()) try {
+      val handler = DebugPlugin.getDefault().getStatusHandler(status)
+      if (!classpathContinuation.isCompleted) handler.handleStatus(status, (this, classpathContinuation))
+      classpathContinuation.future onSuccess {
+        case f => f()
+      }
+    } finally { classpathContinuation = Promise[() => Unit]}
+
     errors.toSeq
   }

huitseeker · 2014-07-11T13:28:45Z

@dragos huitseeker@ef2320e

- better name for scala installs - equals for ScalaInstallation - formatting, error messages typos - no Default & apply for Scala Installations - legacy bundles have a better tag - sparser labels for Scala isntallations - cleaner, unified names for installs in dialogs - don't modify the existing install in case of refused solution

dragos · 2014-07-14T13:00:52Z

org.scala-ide.sdt.core/src/org/scalaide/ui/internal/preferences/CompilerSettings.scala

@@ -144,22 +166,20 @@ class CompilerSettings extends PropertyPage with IWorkbenchPreferencePage with E

  var useProjectSettingsWidget: Option[UseProjectSettingsWidget] = None
  var additionalParamsWidget: AdditionalParametersWidget = _
-  var dslWidget: Option[DesiredSourceLevelWidget] = None
+  var dslWidget: Option[DesiredInstallationWidget] = None

  def save(): Unit = {
    val project = getConcernedProject()
    val scalaProject = project flatMap (ScalaPlugin.plugin.asScalaProject(_))
    scalaProject foreach (p => preferenceStore0.removePropertyChangeListener(p.compilerSettingsListener))
    var wasClasspathChanged = new AtomicIntegerArray(3)


I didn't notice this in the first part of this PR, but

this var could be a val, right?

is there no other way than to have an array of bits to encode what happened? I'd prefer to have at least symbolic constants for what each bit stands for.

I don't believe it could : https://github.com/scala-ide/scala-ide/pull/731/files#diff-48db6f6626423cb0c8a6a4f96a5aaf3fR250
Incidentally, you'll notice useProjectSettingWidget has been functioning the same way so far.

I can replace this with three AtomicBooleans, but is there a fundamental reason for this or is it mostly cosmetic ?

I was referring to var wasClasspathChanged. I believe that one is never reassigned.

My main issue is with using 1 and 2 to refer to flags. I'm surprised I need to justify this, but here it goes: I can easily see myself messing this up next time I update this code. Having these constants have a name would be way more robust, and I believe, standard practice.. :)

Also corrects initialize semantics change in compiler settings upon background change

Also, more informative error markers.

Error markers do not create threading issues, especially since the Property Dialog they create is modal. The Dialogs go away.

ghprb-bot · 2014-07-14T19:47:27Z

Test PASSed.
Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-ide-validator/904/

dragos · 2014-07-15T14:50:45Z

@huitseeker, do we need to update the docs/blog?

huitseeker · 2014-07-15T14:52:55Z

@dragos Done since yesterday huitseeker/scala-ide.github.com@6cef1bc
scala-ide/scala-ide.github.com#74
(I linked to the update on Flowdock)

Scala Installations support stage II : choose your own installation

dragos reviewed Jul 3, 2014
View reviewed changes

skyluc reviewed Jul 7, 2014
View reviewed changes

dragos reviewed Jul 8, 2014
View reviewed changes

François Garillot and others added 9 commits July 11, 2014 13:30

Classpath Error (previous version) now offers a generic Scala Install…

c83cb30

… Dropdown

Cross-versioned error now proposes a Scala Installation Choice

651295f

Project now prompts for a Scala Installation on resolution error.

189797b

Some cleaning: - Project has simpler getDesiredScalaInstallationChoice. - actually it's easy to have Scala installation saving log (harder for reading)

More linting

6661cea

- factory method for directoryScalaInstallations - more logging on install serialization - cleaner Classpath test project - cleaned imports

Adds tests for DirectoryScalaInstallation

1059dec

Tweaks the source jar resolution to be more strict

Un-break tests

0a7d2e2

Edition of build path for projects w/ container & library now redirec…

710928f

…ts to preferences.

Stub for Scala Desired Installation testing

b519661

Tests classpath modifications

29aab22

dragos reviewed Jul 14, 2014
View reviewed changes

François Garillot added 5 commits July 14, 2014 16:03

Put notifier updates on the Display Thread

800f19d

Also corrects initialize semantics change in compiler settings upon background change

Only correct a cross-compiled classpath Once

7e1d26f

Better error messages w/projcet name

00d6b1e

Also, more informative error markers.

Use error markers instead of Dialogs in case of wrong Scala version.

af4e1a9

Error markers do not create threading issues, especially since the Property Dialog they create is modal. The Dialogs go away.

refactor countingListener

49e7582

skyluc added a commit that referenced this pull request Jul 15, 2014

Merge pull request #731 from huitseeker/issue/scala-installations

73a785d

Scala Installations support stage II : choose your own installation

skyluc merged commit 73a785d into scala-ide:master Jul 15, 2014

skyluc mentioned this pull request Jul 15, 2014

rebased pr/735 #737

Merged

scabug mentioned this pull request Apr 7, 2017

Scala version's unparse is unsightly scala/bug#8711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala Installations support stage II : choose your own installation #731

Scala Installations support stage II : choose your own installation #731

huitseeker commented Jul 3, 2014

ghprb-bot commented Jul 3, 2014

dragos Jul 3, 2014

dragos commented Jul 3, 2014

huitseeker commented Jul 3, 2014

dragos commented Jul 3, 2014

huitseeker commented Jul 3, 2014

ghprb-bot commented Jul 4, 2014

skyluc Jul 7, 2014

huitseeker Jul 8, 2014

ghprb-bot commented Jul 8, 2014

dragos commented Jul 8, 2014

dragos Jul 8, 2014

huitseeker Jul 9, 2014

huitseeker commented Jul 9, 2014

dragos commented Jul 11, 2014

dragos commented Jul 11, 2014

huitseeker commented Jul 11, 2014

dragos commented Jul 11, 2014

dragos commented Jul 11, 2014

dragos commented Jul 11, 2014

huitseeker commented Jul 11, 2014

dragos Jul 14, 2014

huitseeker Jul 14, 2014

dragos Jul 14, 2014

ghprb-bot commented Jul 14, 2014

dragos commented Jul 15, 2014

huitseeker commented Jul 15, 2014

Scala Installations support stage II : choose your own installation #731

Scala Installations support stage II : choose your own installation #731

Conversation

huitseeker commented Jul 3, 2014

ghprb-bot commented Jul 3, 2014

Choose a reason for hiding this comment

dragos commented Jul 3, 2014

huitseeker commented Jul 3, 2014

dragos commented Jul 3, 2014

huitseeker commented Jul 3, 2014

ghprb-bot commented Jul 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghprb-bot commented Jul 8, 2014

dragos commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker commented Jul 9, 2014

dragos commented Jul 11, 2014

dragos commented Jul 11, 2014

huitseeker commented Jul 11, 2014

dragos commented Jul 11, 2014

dragos commented Jul 11, 2014

dragos commented Jul 11, 2014

huitseeker commented Jul 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghprb-bot commented Jul 14, 2014

dragos commented Jul 15, 2014

huitseeker commented Jul 15, 2014