#514 Add Gradle editor #564

cmoine · 2017-09-08T12:26:40Z

I have used the genericeditor. It was almost too simple :)

Appart from the joke, I wanted to use a more sophisticated highlighter rules... For example "task" is defined as a keyword but it is not. It is related to Gradle DSL.... So if you invoke a task like for instance:

myTask task: 'anotherTask'

task will be highlighted :( It seems we need some kind of semantic parser to do that. I tried (https://github.com/danielsun1106/groovy-parser), but I am not sure parsing the document for highlight syntax is good (I think I can imagine the answer :D ).

If you of someone else have experience on that, I am very interested with these problematic :)

I know Gradle DSL is complex, and it is probably the reason why there is not bundled Gradle editor in Eclipse :D Even if Eclipse seem to bet more on Kotlin DSL to provide an editor, I don't think it will be quickly the most used editor...

It is why I kept it simple so far :)

I also took the opportunity to add a EditorContributor for #560 :)

eclipse#560 Add editor contribution Signed-off-by: Christophe Moine <christophe.moine@free.fr>

donat

I wanted to give your PR a go, but it seems to be that the changes don't compile ATM.
Could you please try building your project locally (./gradlew build should do the trick) and update the PR?
Besides, there are a lot of style issues: missing headers, tabs instead of spaces, and so on. The build process will indicate those problems, so fixing should be straightforward.
There is some cleanup needed in the code, but first, let me see it in action :)

cmoine · 2017-09-13T10:44:04Z

Sorry, I setup a new environment properly with Oomph :)

I tried also to rebase on master branch, I hope there will be no trouble with that... :O

The FixedRequestAttributesBuilder class was removed in the 2.1.0 release, even though it's used by existing STS releases. To resolve the situation we re-added and deprecateted this class.

The Spring Tool Suite has a dependency to Guava 15 and to Buildship. Due to how the dependencies are currently set up, the only way to make STS and Buildship work together is to use Guava 15 everywhere. This reverts commit c7f47e5.

This commit fixes a regression introduced in Buildship 2.1. The new preferences API ignored the project location and always executed all tasks on the root project. As a result, task selectors were always executed on all projects, even if they were started from a subproject. The fix changes the RunConfiguration interface such that it references a project config as opposed to a build configuration. This commit fixes eclipse#520

Until 2.1 Buildship accepted absolute root project location in the import preferences.

Signed-off-by: Andy Wu <andy.wu@liferay.com>

donat

We are getting closer! I've tried out the editor and it feels so much nicer than using the simple text editor.

I've added a couple of suggestions how to clean up the code and how to make the new functionality simpler.

There's one thing besides my review comments. Can you please rebase your branch to the latest master so that only your files appear in the review? It's a bit annoying that I have to go through 65 files, even though your change touches ~6 files. Plus, there's already a merge conflict in one of the manifest files.

donat · 2017-09-14T12:26:45Z

org.eclipse.buildship.core/plugin.xml

+    <extension point="org.eclipse.core.contenttype.contentTypes">
+      <content-type
+           file-extensions="gradle"
+           id="org.eclipse.buildship.core.gradleScript"


org.eclipse.buildship.core.files.gradlebuildscript

donat · 2017-09-14T12:28:01Z

org.eclipse.buildship.core/plugin.xml

+      <content-type
+           file-extensions="gradle"
+           id="org.eclipse.buildship.core.gradleScript"
+           name="Gradle Build File"


Gradle Build Script

donat · 2017-09-14T12:29:27Z

...ipse.buildship.ui/src/main/java/org/eclipse/buildship/ui/editor/GradleEditorContributor.java

@@ -0,0 +1,29 @@
+package org.eclipse.buildship.ui.editor;


Missing header.

donat · 2017-09-14T12:29:57Z

...ipse.buildship.ui/src/main/java/org/eclipse/buildship/ui/editor/GradleEditorContributor.java

+import org.eclipse.ui.handlers.IHandlerService;
+import org.eclipse.ui.part.EditorActionBarContributor;
+
+public class GradleEditorContributor extends EditorActionBarContributor {


Missing javadoc. Please run the build and fix all these issues. If you have any problems running the build (should be as easy as executing ./gradlew build) just let me know and I'll help.

donat · 2017-09-14T12:39:15Z

...ipse.buildship.ui/src/main/java/org/eclipse/buildship/ui/editor/GradleEditorContributor.java

+import org.eclipse.ui.part.EditorActionBarContributor;
+
+public class GradleEditorContributor extends EditorActionBarContributor {
+	private static final String REFRESHPROJECT_COMMAND_ID = "org.eclipse.buildship.ui.commands.refreshproject";


Move this ID to the UiPluginConstants class.

donat · 2017-09-14T12:50:38Z

org.eclipse.buildship.ui/plugin.xml

-            class="org.eclipse.ui.editors.text.TextEditor"
-            icon="icons/full/obj16/gradle_file.png"
-            name="Gradle Build Script Editor"
+            class="org.eclipse.ui.internal.genericeditor.ExtensionBasedTextEditor"


Do we really have to use an internal class here?

Hmm, good point, but I don't see other way to define an icon for the editor, and most important: an editorcontributor to add "Refresh Gradle Project" action in the toolbar.

donat · 2017-09-14T12:51:51Z

...buildship.ui/src/main/java/org/eclipse/buildship/ui/editor/GradlePresentationReconciler.java

+
+public class GradlePresentationReconciler extends PresentationReconciler {
+	private static final List<String> GRADLE_IDENTIFIERS=Arrays.asList(
+			// Java keywords


Please clean up the indentation and remove the comments.

donat · 2017-09-14T13:08:47Z

...ldship.ui/src/main/java/org/eclipse/buildship/ui/preferences/GradleEditorPreferencePage.java

+ * @author Benjamin gurok
+ * @author Paul Verest
+ */
+public class GradleEditorPreferencePage extends FieldEditorPreferencePage implements IWorkbenchPreferencePage {


We have a simple editor and thus we should keep it simple. How about we

remove the preference page

reuse the colors from the Java editor (org.eclipse.jdt.ui.PreferenceConstants)

Add a preference listener to update the colors when the Java editor colors changed.
This way we can provide a consistent color scheme without adding more complexity to the UI.

I hesitated reusing Java Colors. I will see if that fits.

cmoine · 2017-09-14T16:19:08Z

Ok, no pb. Do you mind if I create a new PR ? I would like to create a clean new branch on my fork.

donat · 2017-09-14T17:48:34Z

Aure, go ahead. Actually, it makes sense as changing this PR might mess up the review comments.

cmoine · 2017-09-15T07:09:48Z

It seems that colors for Java Syntax depends on the used Theme. With dark theme, keywords are orange instead of violet. Do you know how to take the Theme into account ? I had a look yesterday, but I haven't found in a "reasonable" timing :) If you already know, I would be very interested, thanks.

eclipse#560 Add editor contribution Signed-off-by: Christophe Moine <christophe.moine@free.fr>

donat · 2017-09-15T08:19:38Z

You can attach a listener to the JDT UI plugin preferences. Here's an example:

EclipsePreferencesUtils.getInstanceScope().getNode(JavaUI.ID_PLUGIN)
    .addPreferenceChangeListener(new IPreferenceChangeListener() {

    @Override
    public void preferenceChange(PreferenceChangeEvent event) {
        if (event.getKey().equals(PreferenceConstants.EDITOR_JAVA_KEYWORD_COLOR)) {
           System.out.println(JavaUI.getColorManager()
               .getColor(PreferenceConstants.EDITOR_JAVA_KEYWORD_COLOR));
        }
    }
});

A small note: The EclipsePreferencesUtils is a utility class from Buildship.

donat · 2017-09-20T09:27:08Z

Can we help anything regarding this PR?

cmoine · 2017-09-20T10:01:14Z

Thanks but I haven't much time this week, but don't worry, I'm on it :)

Also the current implementation of the PresentationReconcilier don't support the addPreferenceChangeListener.
I made I short test:

UiPlugin.getInstance().getPreferenceStore().addPropertyChangeListener(new IPropertyChangeListener() {
            @Override
            public void propertyChange(PropertyChangeEvent event) {
                if (event.getProperty().equals(GradleEditorConstants.KEY_COLOR_KEYWORD)) {
                    GradlePresentationReconciler.this.keywordToken.setData(new TextAttribute(Display.getDefault().getSystemColor(SWT.COLOR_DARK_YELLOW), null, UiPlugin.getInstance().getPreferenceStore().getBoolean(GradleEditorConstants.KEY_BOLD_KEYWORD) ? SWT.BOLD : SWT.NORMAL));
                }
            }
        });

But as expected, the setData with a new TextAttribute doesn't refresh the editor...

JDT use setData with string (token ids) instead of TextAttribute, and I need to use something similar I guess.

cmoine · 2017-09-20T17:27:49Z

Ok, I made some progress, but it seems a long way home :(

It seems that we need a "Reconcilier". The PresentationReconcilier is not sufficient. unfortunately, only genericeditor v1.1 let us define a Reconcilier... Either I need to get ride of genericeditor, or we need to migrate to genericeditor 1.1. What do you think ?

cmoine · 2017-09-20T17:48:08Z

Hmm, sorry, but it seems that genericeditor it too far from what you expect: the editor doesn't override affectsTextPresentation which allows colors changing "on the fly"

donat · 2017-09-20T18:02:59Z

I think it would be already fine if the editor colors would be updated after the editor is closed and reopened. Theme updates are not that frequent anyway.

But there's yet another problem with the generic editor. Since the dependency is relatively new, we can only ship with it Buildship only for the latest version. So, if we set the org.eclipse.buildship.ui.genericeditor dependency to 1.1, then the editor support will be available in Eclipse 4.8+, maybe 4.7.1, I'm not 100% sure (I'd adjust the build scripts, you don't have to worry about that).

That being said, I think I'm leaning towards going back to the initial idea of having a discrete editor definition. That way we can support previous Eclipse versions and we have full control over the color preferences. How does that sound to you?

In any case, thanks for sticking with us! We are super grateful that you invest your time into this feature.

cmoine · 2017-09-21T12:35:23Z

I totally agree. My experiments with genericeditor are not conclusive unfortunately :(

cmoine · 2017-09-25T07:52:11Z

I love Eclipse, but... the Syntax Hightlighthing mechanism with Reconcilier/Damager/Partitioner is just a nightmare !!!
Do you mind if I use the internal org.eclipse.jdt.internal.ui.text.AbstractJavaScanner ?

cmoine · 2017-09-25T08:05:19Z

Damned, I found the magic method to make it work with genericeditor:
viewer.invalidateTextPresentation()
Sorry for the n-th change, but I recommend to use generic editor: Defining our own editor/presenter/reconcilier require a LOT OF CODE, and sometimes references to internal Java Plugin if we don't want to copy paste too much :(

What do you think ?

PS: both solution are almost working, we simply need to choose, but I am less confident with the solution without genericeditor, because there is so much bunch of code I took from other that am not in full control :(

donat · 2017-09-25T08:16:52Z

I'd prefer genericeditor if we can use viewer.invalidateTextPresentation() and if we don't need the 1.1 version just to update the colors upon the theme changes.

cmoine · 2017-09-26T12:36:45Z

omg, spent one day figuring out with Generic Editor doesn't work... It definitely doesn't fit. The partitionning/damager/reconciler is definitely complex stuff, and Generic Editor doesn't fit. I will switch back the plain old implementation again :'-( (without generic editor) If somebody interested, the limitation comes from this (in SourceViewerConfiguration), which is not properly extendable within GenericEditor:

public String[] getConfiguredContentTypes(ISourceViewer sourceViewer) {
	return new String[] { IDocument.DEFAULT_CONTENT_TYPE };
}
public String getConfiguredDocumentPartitioning(ISourceViewer sourceViewer) {
	return IDocumentExtension3.DEFAULT_PARTITIONING;
}

Now I think I understand why there is not Gradle editor :D

cmoine · 2017-09-28T07:12:56Z

I am almost ready to push the "non generic editor" solution, is it ok ?

donat · 2017-09-28T07:25:44Z

Sure, please go ahead.

types + Reuse Java syntax color highlighting Signed-off-by: Christophe Moine <christophe.moine@free.fr>

cmoine · 2017-09-28T10:13:52Z

Pushed.

cmoine · 2017-09-28T10:14:20Z

Sorry for my clumpsy usage of Git :(

donat · 2017-09-28T10:21:23Z

Thanks for the change, I'll have a look shortly.

In the meanwhile, here's what I usually do on the command line when I want to produce a PR with a single commit:

# count the commits I have on the local branch; say there were 3
git rebase -i head~3 # interactive rebase for the first 3 commits
# in the git rebase editor I leave the first entry on 'pick' and change the rest to 'squash'
git rebase master
git push -f # replace changes on remote branch

cmoine · 2017-09-29T08:42:48Z

I find out my mistake with Git: I told that I woudl create a new PR :)

cmoine · 2017-09-29T08:44:44Z

New PR: #585

eclipse#514 Add Gradle editor

a000335

eclipse#560 Add editor contribution Signed-off-by: Christophe Moine <christophe.moine@free.fr>

donat self-requested a review September 12, 2017 15:14

donat requested changes Sep 12, 2017

View reviewed changes

donat and others added 20 commits September 13, 2017 14:01

Restore compatibility with Spring Tool Suite

3ab6670

The FixedRequestAttributesBuilder class was removed in the 2.1.0 release, even though it's used by existing STS releases. To resolve the situation we re-added and deprecateted this class.

Revert Guava dependency to version 15.0

202f93b

The Spring Tool Suite has a dependency to Guava 15 and to Buildship. Due to how the dependencies are currently set up, the only way to make STS and Buildship work together is to use Guava 15 everywhere. This reverts commit c7f47e5.

Update e47 SDK version

c2515f4

Project configuration can read absolute root project locations again

8708c24

Until 2.1 Buildship accepted absolute root project location in the import preferences.

Fix typo

d9c3710

Add test coverage for task execution

cd38d67

Ignore invalid project configs upon task view refresh

4a8f398

Update tooling-commons to 0.19.3

77a862d

Ignore invalid project configs upon synchronization

05b14a2

Centralize broken project config handling in ConfigurationManager

bcb72ce

Project task is executed on root project

b1ef9e2

Improve disabled task explanation

d9038b3

Adjust task enablement

166fd90

Increment version from 2.1.2 to 2.1.3

5201a50

double click to expand or collapse the folder node in tasks view

7e465b6

Signed-off-by: Andy Wu <andy.wu@liferay.com>

Simplify task view tests

ed1a41c

Add e47 milestone and release update sites to docs

0069e0d

Update icons for Open build scans action

7d3b01d

EditorColors file was missing and content-type

74a15c8

cmoine force-pushed the master branch from c8486e6 to 74a15c8 Compare September 13, 2017 12:03

donat self-requested a review September 14, 2017 12:21

donat requested changes Sep 14, 2017

View reviewed changes

eclipse#514 Add Gradle editor

d0883d1

eclipse#560 Add editor contribution Signed-off-by: Christophe Moine <christophe.moine@free.fr>

cmoine added 2 commits September 28, 2017 11:41

Get rid of the GenericEditor, because it doesn't allow several partition

b529d38

types + Reuse Java syntax color highlighting Signed-off-by: Christophe Moine <christophe.moine@free.fr>

Get rid of the GenericEditor, because it doesn't allow several partition

b399d19

types + Reuse Java syntax color highlighting Signed-off-by: Christophe Moine <christophe.moine@free.fr>

cmoine closed this Sep 29, 2017

cmoine mentioned this pull request Sep 29, 2017

Basic editor support for build scripts #585

Merged

donat added the was:superseded label Oct 4, 2017

donat assigned cmoine Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#514 Add Gradle editor #564

#514 Add Gradle editor #564

cmoine commented Sep 8, 2017

donat left a comment

cmoine commented Sep 13, 2017

donat left a comment •

edited

donat Sep 14, 2017

donat Sep 14, 2017

donat Sep 14, 2017

donat Sep 14, 2017

donat Sep 14, 2017

donat Sep 14, 2017

cmoine Sep 14, 2017

donat Sep 14, 2017

donat Sep 14, 2017

cmoine Sep 14, 2017

cmoine commented Sep 14, 2017

donat commented Sep 14, 2017

cmoine commented Sep 15, 2017

donat commented Sep 15, 2017

donat commented Sep 20, 2017

cmoine commented Sep 20, 2017

cmoine commented Sep 20, 2017

cmoine commented Sep 20, 2017

donat commented Sep 20, 2017

cmoine commented Sep 21, 2017

cmoine commented Sep 25, 2017

cmoine commented Sep 25, 2017 •

edited

donat commented Sep 25, 2017

cmoine commented Sep 26, 2017 •

edited

cmoine commented Sep 28, 2017

donat commented Sep 28, 2017

cmoine commented Sep 28, 2017

cmoine commented Sep 28, 2017

donat commented Sep 28, 2017 •

edited

cmoine commented Sep 29, 2017

cmoine commented Sep 29, 2017

#514 Add Gradle editor #564

#514 Add Gradle editor #564

Conversation

cmoine commented Sep 8, 2017

donat left a comment

Choose a reason for hiding this comment

cmoine commented Sep 13, 2017

donat left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoine commented Sep 14, 2017

donat commented Sep 14, 2017

cmoine commented Sep 15, 2017

donat commented Sep 15, 2017

donat commented Sep 20, 2017

cmoine commented Sep 20, 2017

cmoine commented Sep 20, 2017

cmoine commented Sep 20, 2017

donat commented Sep 20, 2017

cmoine commented Sep 21, 2017

cmoine commented Sep 25, 2017

cmoine commented Sep 25, 2017 • edited

donat commented Sep 25, 2017

cmoine commented Sep 26, 2017 • edited

cmoine commented Sep 28, 2017

donat commented Sep 28, 2017

cmoine commented Sep 28, 2017

cmoine commented Sep 28, 2017

donat commented Sep 28, 2017 • edited

cmoine commented Sep 29, 2017

cmoine commented Sep 29, 2017

donat left a comment •

edited

cmoine commented Sep 25, 2017 •

edited

cmoine commented Sep 26, 2017 •

edited

donat commented Sep 28, 2017 •

edited