Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic editor support for build scripts #585

Merged
merged 1 commit into from
Oct 6, 2017
Merged

Conversation

cmoine
Copy link
Contributor

@cmoine cmoine commented Sep 29, 2017

Follows up #564

@cmoine cmoine mentioned this pull request Sep 29, 2017
@donat donat self-requested a review September 29, 2017 14:56
Copy link
Contributor

@donat donat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave the editor a second spin and the syntax highlight and the color update works perfectly.

I've found an issue though. It seems that pressing the tab key doesn't work. If the tab character is inserted at the beginning of a line, the characters on the right don't move. The file gets modified though, so only the presentation is broken. Can you please have a look what's going on?

@cmoine
Copy link
Contributor Author

cmoine commented Sep 29, 2017 via email

@donat
Copy link
Contributor

donat commented Oct 3, 2017

Sorry, if my previous message sounded a bit pushy. Take your time and let us know if we can help.

@cmoine
Copy link
Contributor Author

cmoine commented Oct 3, 2017

Ah, no, I didn't take it that way, no pb :)

The method TextSourceViewerConfiguration.getTabWidth was returning zero because of wrong PreferenceStore :) Don't hesitate if you find other problems.

Cheers.

@cmoine
Copy link
Contributor Author

cmoine commented Oct 3, 2017

I also moved the @author into "Contributor" section in the comment header, because I saw it was like that in other files :)

@donat
Copy link
Contributor

donat commented Oct 4, 2017

About the license header: What you found is the old header. If you have a look at the newer source files (like this) then you'll find the proper header.

Copy link
Contributor

@donat donat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest changes work nicely. I'll merge this PR later - and maybe polish a few things - unless @oehme has something to say.

Copy link
Contributor

@donat donat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I have to ask one more change. I think it's the same problem as before: when I try to merge your changes I see unrelated commits. Can you please rebase your branch to the latest master?

In case you are interested, here is an article about PRs and rebasing.

@cmoine
Copy link
Contributor Author

cmoine commented Oct 4, 2017

I think it is because of my mistake when I did a merge with an old branch. I forced a push: I hope it will be much better.

Copy link
Contributor

@donat donat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer, thanks for adjusting the history. I'll soon integrate this PR.

@cmoine
Copy link
Contributor Author

cmoine commented Oct 4, 2017

Great ! Ouch, I forgot to replace to @author back. Do you want me to do it ?

@donat
Copy link
Contributor

donat commented Oct 4, 2017

Yes, it would be better if you do it :)

eclipse#560 Add editor contribution
+ Get rid of the GenericEditor, because it doesn't allow several
partition types + Reuse Java syntax color highlighting + Fix tab size in
the Gradle editor

Signed-off-by: Christophe Moine <christophe.moine@free.fr>
@cmoine
Copy link
Contributor Author

cmoine commented Oct 4, 2017

It would be a shame to give up at this point :) Done !

Copy link
Contributor

@donat donat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cmoine! I wanted to give the code a final polish but it ended up being a bigger refactoring than I expected. Would you please have a look at my changes and look around if there's something else to simplify?

I'm particularly interested in the common rules in the presentation reconciler and the scanner class.

You can review/merge my code in the PR I've just opened in your repository: cmoine#1

@donat donat changed the title #514 Add Gradle editor Add editor support with basic highlighting capabilities for build scripts Oct 6, 2017
@donat donat added this to the Buildship 2.1.3 milestone Oct 6, 2017
@donat donat changed the title Add editor support with basic highlighting capabilities for build scripts Editor support for build scripts Oct 6, 2017
@donat donat removed this from the Buildship 2.1.3 milestone Oct 6, 2017
@donat donat changed the title Editor support for build scripts Basic editor support for build scripts Oct 6, 2017
@donat donat merged commit 1984095 into eclipse:master Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants