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

Use of ServiceLoader for ease of adding new languages #142

Closed
wants to merge 35 commits into from
Closed

Use of ServiceLoader for ease of adding new languages #142

wants to merge 35 commits into from

Conversation

ZenHarbinger
Copy link

I have gone through for my own purposes and added some interfaces and classes which can be loaded using the Java ServiceLoader class.

I added:
org.fife.ui.rsyntaxtextarea.TokenMakerRegistration
From here each language then implements a class that derives from this interface.
The interface provides 3 methods.

String getLanguage();
String getTokenMaker();
FoldParser getFoldParser();

which are used in DefaultTokenMakerFactory. Instead of manually specifying every language, the ServiceLoader class can find the files from the META-INF.services/org.fife.ui.rsyntaxtextarea.TokenMakerRegistration file which keeps a record of all implementing classes. Using maven (not sure about gradle) there are plugins which will automatically fill in this file when building.

Simlarly, I have added this functionality to LanguageSupportFactory in RSTALanguageSupport for finding supported languages.
A new interface LanguageSupportRegistration is added with methods:

String getLanguage();
String getLanguageSupportType();

The ServiceLoader will then find all implemented classes in LanguageSupportFactory and add them on the fly.
META-INF.services/org.fife.rsta.ac.LanguageSupportRegistration keeps the record of all implementing classes similar to the above TokenMakerRegistration.

@bobbylight bobbylight added this to the java6 milestone Sep 19, 2015
@bobbylight
Copy link
Owner

This would be slick, but I'm old and crotchety and want RSTA to stay buildable/runnable with JDK 5. For this reason I won't be merging this pull request until I decide to get with the times.

I'm creating a milestone for things like this just so I don't lose track of them.

@bobbylight bobbylight self-assigned this Sep 19, 2015
@ZenHarbinger
Copy link
Author

Same as RSTALanguage...
I was able to set the compiler settings in the pom.xml file for maven to use java 1.5 and it worked.
<maven.compiler.source>1.5</maven.compiler.source>
<maven.compiler.target>1.5</maven.compiler.target>

@bobbylight
Copy link
Owner

Yes, but the java.util.ServiceLoader class was not added until 1.6, so compiling with -target 1.5 is not sufficient, unfortunately.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 36.896% when pulling b468af7 on ZenHarbinger:master into e9020fa on bobbylight:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 41.696% when pulling d613332 on ZenHarbinger:master into fe44cd3 on bobbylight:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 41.692% when pulling e5dbf22 on ZenHarbinger:master into 35b4142 on bobbylight:master.

@ftomassetti
Copy link

Maybe now is a good time to move to Java 6?

@ZenHarbinger
Copy link
Author

That would be awesome.
My concern is that if you are to merge in my changes, there is no plugin for generating the services file in gradle that I know of. You can hand generate it with no problems though.

@rednoah
Copy link

rednoah commented Jan 11, 2017

Is there anyone writing Desktop applications not using Java 8 at this point? Ant 1.10 just moved from 1.5 to 1.8 compatibility.

@bobbylight
Copy link
Owner

RSTA actually moved to Java 6 last year; I just forgot about this pull request.

Concerns I have with this pull request currently include stuff not related to the feature itself:

  • Unit test failures (check the CI build log)
  • Inclusion of unnecessary files (*.yml, pom.xml since we generate it via gradle build)
  • Header template in new files - for consistency, please use something like the existing class files use. (I know I'm not entirely consistent here myself, but I've recently moved to a file-agnostic, copy-paste template):
/*
 * 01/11/2017 (whatever your the current date is)
 *
 * This library is distributed under a modified BSD license.  See the included
 * RSyntaxTextArea.License.txt file for details.
 */
  • Unit tests for the new files. I know this will be a PITA, and copy-paste, but I'm making an honest effort to never let RSTA's code coverage drop moving forward. I hate that our code coverage is so low, but it's quite a large undertaking to retroactively write tests, but new code can have tests added easily.
  • Finally, make sure there are no checkstyle violations in the build output. IIRC I wired this up to break the build if it is the case, but still worth a double-check.

One concern I have with merging this is that there's no longer one handy-dandy place to look for a listing of supported lanaguages, such as SyntaxConstants today. Perhaps that's mitigated by the fact that it's one step toward aggregating all information specific to a language into one place: syntax highlighting, code folding, etc. Stuff like language-specific code formatting, and language-support features like you mention, could be included as well.

@ZenHarbinger
Copy link
Author

I'm going to rebase and resubmit.
I can guess why the tests are failing. They pass for me on circleci under maven. It needs the services file.

@ZenHarbinger
Copy link
Author

Rebased pull: #223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants