Adds a preference and an project preference page #43

Merged
merged 1 commit into from Apr 4, 2013

Projects

None yet

5 participants

@skyluc
Eclipse Scala IDE member

The preference is initialized with framework defaults (play.api.templates., play.api.templates.PlayMagic.).
It can be overriden at the project level, in the project properties.
Changes are not applied directly, as we are currently lacking a real builder.

@skyluc
Eclipse Scala IDE member

@dotta, @dragos, @huitseeker, review?

@aldenml, any chance that you could review my pull request, at least to check if that works for you?

@skyluc
Eclipse Scala IDE member

I push a new version, I forgot to include the link to #35.

@dragos
Eclipse Scala IDE member

I'll have a look ASAP.

@aldenml

I didn't have time to review in deep the commit, but I did test it in my environment and it works! I need to edit the file to trigger the template's compilation though.

@dotta dotta and 3 others commented on an outdated diff Mar 5, 2013
...c/org/scalaide/play2/properties/PlayPreferences.scala
@@ -16,6 +16,34 @@ import org.eclipse.ui.IWorkbenchPreferencePage
import org.eclipse.ui.dialogs.PropertyPage
import org.scalaide.play2.PlayPlugin
+object PlayPreferences {
+ final val TemplatesImport = "templateImport"
+
+ val importsRegex = "import ([^\n]+)\n".r
@dotta
dotta Mar 5, 2013

Can this be private?

@skyluc
skyluc Mar 5, 2013

Yes, it should.

@dragos
dragos Mar 6, 2013

This won't find multi-line imports (but I think we can live with this limitation).

import scala.{
  collection,
  util,
  concurrent }
@huitseeker
huitseeker Mar 6, 2013

You may want to try "(?sm)import\s([^\n]+$|\{.+?\}$)" (untested). And then, you either filter the (single) capturing group based on whether it starts with braces, or you go for adding capturing subgroups in the expression above, and find the non-empty one.

@skyluc
skyluc Mar 6, 2013

The regex is not suppose to find multiline imports. The value of the preference is not directly accessible by the user. In code, serializeImports and deserializeImports should be used to got between Array[String] and String.
The preference is stored as valid import code, so it doesn't have to be recreated every time it is used (and it needs to be serialized to String anyway).

I'll re-organize the scaladoc to make it more clear.

@dotta dotta and 1 other commented on an outdated diff Mar 5, 2013
...g/scalaide/play2/properties/ProjectPropertyPage.scala
+ def isValid(text: String) = null
+ });
+
+ if (dlg.open() == Window.OK) {
+ dlg.getValue()
+ } else {
+ null
+ }
+ }
+
+ }
+
+ // store information about the element being configured. The data is provided by the workbench during the
+ // page lifecycle.
+ var element: IAdaptable = _
+ var playProject: PlayProject = _
@dotta
dotta Mar 5, 2013
  • Can the above fields be private?
  • Can getElement/setElement be called by different threads? (if yes, then this class has to be thread-safe)
@skyluc
skyluc Mar 5, 2013

The fields shoud be private.
I'll double check for the get/set, but they should be called in UI thread.

@dotta
dotta Mar 5, 2013

Does this mean/imply that instances of this class are confined to the UI Thread? If yes, could you add a note in the class' ScalaDoc.

@dotta dotta and 1 other commented on an outdated diff Mar 5, 2013
...g/scalaide/play2/properties/ProjectPropertyPage.scala
+ override def getElement(): IAdaptable = element
+
+ override def setElement(e: IAdaptable) {
+ element = e
+ playProject = element match {
+ case project: IProject =>
+ PlayPlugin.plugin.asPlayProject(project).get
+ case project: IJavaProject =>
+ PlayPlugin.plugin.asPlayProject(project.getProject()).get
+ }
+ }
+
+ // ----
+
+ override def doGetPreferenceStore(): IPreferenceStore = {
+ playProject.preferenceStore
@dotta
dotta Mar 5, 2013

Just wondering if ScopedPreferenceStore is thread-safe.

@skyluc
skyluc Mar 6, 2013

Yes it is thread safe, and funny fact: it is using an immutable map (check ImmutableMap in the equinox bundle).

@dotta dotta commented on an outdated diff Mar 5, 2013
...la-ide.play2/src/org/scalaide/play2/PlayProject.scala
class PlayProject private (val scalaProject: ScalaProject) {
private val presentationCompiler = new TemplatePresentationCompiler(this)
+
+ lazy val preferenceStore = new ScopedPreferenceStore(new ProjectScope(scalaProject.underlying), PlayPlugin.PluginId)
@dragos
Eclipse Scala IDE member

👍

@skyluc
Eclipse Scala IDE member

I updated the documentation and cleaned up a bit the code.

@dotta
Eclipse Scala IDE member

The below comment is a follow-up on this discussion

Mmmm, I had a quick look at at ScopedPreferenceStore and it doesn't look thread-safe. It contains a lot of mutable and non-final fields.

It's also fishy that the JavaDoc of ScopedPreferenceStore doesn't mention thread-safety (this is not enough to prove that the class isn't thread-safe, but given Eclipse documentation is usually good, I think it might be an indication). Indeed, if you look at the EventManager JavaDoc ( ScopedPreferenceStore is a subtype of EventManager), it is clearly stated that All the methods on this class are guaranteed to be thread-safe..

@skyluc skyluc Adds a preference and an project preference page
The preference is initialized with framework defaults (play.api.templates._, play.api.templates.PlayMagic._).
It can be overriden at the project level, in the project properties.
Changes are not applied directly, as we are currently lacking a real builder.

Fix #35.
c97147b
@skyluc
Eclipse Scala IDE member

I made the cached preference store thread safe. An instance of the preference store is created on the fly for the property page.

@dotta
Eclipse Scala IDE member

LGTM

@skyluc skyluc was assigned Apr 4, 2013
@skyluc skyluc merged commit d4588a4 into scala-ide:master Apr 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment