-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
[automation] Added ScriptModuleTypeProvider for Jython and Groovy scripted Actions and Conditions #635
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/multi-zone-home-alarm-script-for-openhab/41425/37 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR removes the NashornScriptEngineFactory, as it was a duplicate of GenericScriptEngineFactory after this PR, except for how the scopeValues() method was implemented. I did not see any need for both files, so I added special handling of ECMAScripts in GenericScriptEngineFactory.scopeValues(). It's a little strange to put something specific in something names Generic, but this elimiated a file and a lot of duplicated code.
If you would like to remove duplicate code, then we should improve the generic implementation to allow the specific child to modify / implement only the special scopeValues part without code duplication of all the remaining one.
Adding specific code to a generic class is IMHO nothing we should do ever in OOP.
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineFactory.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineFactory.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineFactory.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineManager.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
To expedite things, I'll add NashornScriptEngineFactory back in. That way it is no worse off than it was. Cleaning up the duplicated code can then be done in a separate PR. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/paperui-rule-bug-state-with-decimal/70627/8 |
2e52513
to
d223e7f
Compare
@maggu2810, I've reworked this PR, so hopefully all of your comments have now been addressed. I'm not looking forward to wait a couple weeks for a review, so I'm hoping @wborn, @cweitkamp, or @kaikreuzer might take a look at it. Hope your vacation is going well! I have tested this PR for a while now, and am running it on my production system without issues.
I have noticed something though, which also occurs in the current production code. When OH starts up, the SEFs, or the ScriptEningManagerImpl, are started twice. Every now and then, after clearing the cache, this does not happen. The code seems to handle this, but I'm not sure what's causing it yet. This is unrelated to this PR, but I thought I'd mention it in case someone noticed it or had an explanation for it. |
Thank you for all your work and effort. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments left.
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineFactory.java
Show resolved
Hide resolved
...main/java/org/openhab/core/automation/module/script/internal/NashornScriptEngineFactory.java
Outdated
Show resolved
Hide resolved
...main/java/org/openhab/core/automation/module/script/internal/NashornScriptEngineFactory.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
...va/org/openhab/core/automation/module/script/internal/provider/ScriptModuleTypeProvider.java
Outdated
Show resolved
Hide resolved
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
@openhab-5iver Please ping me if you think all is done and I should start another review round. |
@maggu2810, will do! A few more tests and I'll commit the changes. |
d223e7f
to
905f330
Compare
@maggu2810, it's ready... and thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, after that ones it should be ready.
...va/org/openhab/core/automation/module/script/internal/provider/ScriptModuleTypeProvider.java
Show resolved
Hide resolved
.../java/org/openhab/core/automation/module/script/internal/handler/ScriptConditionHandler.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineManager.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineManager.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineManager.java
Outdated
Show resolved
Hide resolved
...dule.script/src/main/java/org/openhab/core/automation/module/script/ScriptEngineFactory.java
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
Outdated
Show resolved
Hide resolved
Added ScriptModuleTypeProvider, which dynamically adds available script languages to the ParameterOptions used in Paper UI when configuring a ScriptAction or ScriptCondition. Signed-off-by: Scott Rushworth <openhab@5iver.com>
905f330
to
8ed7c43
Compare
@maggu2810, thank you very much for your patience and the time you have spent working with me. I have learned a lot from this! I've pushed the latest changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to give it back to you: thank you very much for your patience and the time you have spent working with me. 😉
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/wip-ngre-jsr223-documentation-refactoring/73569/6 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/preparation-for-2-5m2/75738/1 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/windows-all-oh-logs-stop-when-jython-is-in-the-classpath/79654/24 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/namespaces-packaging-for-scripting-apis/86940/6 |
…n page and added an explanation f… (openhab#635) * Clarified Misc on systems integration page and added an explanation for the different installation options. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Review improvements. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Changes due to second review Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Adressed review comments. Added feature command family to console part. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Again some review based changes. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Removed console part, Added addons.cfg part and added a warning to jar file part. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Image not needed without console section. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Again some small improvements for introduction and addons.cfg part. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Improvements to addons.cfg section. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Adressed review comments Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Pointed needed addon types for trailing 1 out. Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Adressed review comments and fixed one remaining typo Signed-off-by: Confectrician <github@luckenba.ch>
Added ScriptModuleTypeProvider, which dynamically adds available script languages to the ParameterOptions used in Paper UI when configuring a ScriptAction or ScriptCondition. Signed-off-by: Scott Rushworth <openhab@5iver.com> GitOrigin-RevId: 5f880e1
Fixes #521. This PR replaces #519. As discussed there, this PR will provide a dynamic list of script languages when using ScriptAction or ScriptCondition, based on the ScriptEngines in use on the system. It implements the ScriptModuleTypeProvider, which replaces the ScriptTypes ResourceBundle. I do not see anything that would make this a breaking change. The ParameterOption labels and values for the currently known working script languages are...
Things to be aware of:
@kaikreuzer, you had suggested a ConfigOptionProvider, but I could not find a way to add one to a ResourceBundle. I based this provider off of your MediaActionTypeProvider, in which you state...
ConfigOptionProvider had been around for about a year when you wrote this, so I figured you would have used one if it was possible. That is why I went this route.
@openhab/core-maintainers, please review. I've tested this, and everything seemed to function properly, but I don't think of myself as much of a Java developer. 😉
Signed-off-by: Scott Rushworth openhab@5iver.com (github: openhab-5iver)