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

Automation: [RFC] Add scriptlanguage context #627

Closed
wants to merge 1 commit into from

Conversation

davidgraeff
Copy link
Member

We have the "script" context so far to tell a user-interface to render a script editor. The user-interface does not know however which language is going to be edited (-> syntax highlighting modules or even another editor component might be required).

Ideally "context" tells exactly about the content like "script_javascript". That is not possible here for obvious reasons.

I therefore publish this as a RFC, suggesting that we add another "context" value called "scriptlanguage". A "scriptlanguage" config parameter would always accompany a "script" config parameter.
A user-interface would then have all required information for rendering the correct widget.

Signed-off-by: davidgraeff david.graeff@web.de

We have the "script" context so far to tell a user-interface to render a script editor. The user-interface does not know however which language is going to be edited (-> syntax highlighting modules or even another editor component might be required).

Ideally "context" tells exactly about the content like "script_javascript". That is not possible here for obvious reasons.

I therefore publish this as a RFC, suggesting that we add another "context" value called "scriptlanguage". A "scriptlanguage" config parameter would always accompany a "script" config parameter. 
A user-interface would then have all required information for rendering the correct widget.

Signed-off-by: davidgraeff <david.graeff@web.de>
@maggu2810
Copy link
Contributor

Why not using the context "script" and a config parameter "scriptlanguage"?

@5iver
Copy link

5iver commented Mar 5, 2019

First, I am doing some final testing for a PR that removes the ScriptTypes.json resource bundle. 😄 The ScriptAction and ScriptCondition ModuleTypes will be provided with a ScriptModuleTypeProvider, which will dynamically generate the options available in the script type dropdown, based on what has been installed in the system. This is an evolution of #519.

Ideally "context" tells exactly about the content like "script_javascript".

Nah... the context tells the UI that the control is a multiline text box. The value the user selects for this config option tells us the script type. You can't have a scripting language dependent context in the ModuleType, since the ModuleType is what is asking the user which language is going to be used.

When context=="script", could you just read what the user has selected? You won't know which type of script will be used until the user has populated this, but it will provide the script language used. If the editor is going to change based on the selection, you'll also need to be watching for when the user changes it, and update the editor accordingly.

My PR will change which values the UI would find. For the currently known supported ScriptEngines, the values are js, py, and groovy. These are the file extensions, but we previously used the MIME type. This list will grow, so you'll need a default editor in case you come across a value not in the list.

@davidgraeff
Copy link
Member Author

davidgraeff commented Mar 5, 2019

My PR will change which values the UI would find. For the currently known supported ScriptEngines, the values are js, py, and groovy.

Would be awesome if you could add the mimetype as well. That is the defacto standard to tell a target what type of data has been exchanged.

When context=="script", could you just read what the user has selected?

That's the point. The script and the script type are in two separate configuration parameters.

The editor only sees the configuration parameter containing the script. My suggestion in this PR is to semantically tag the script-type configuration parameter via the context, so that a heuristic search within the user-interface code can provide the editor with the script type. How the editor reacts when the type is changed, should not concern us here.

Why not using the context "script" and a config parameter "scriptlanguage"?

That's possible and I thought about it. In C++ you would put all min/max/step values dedicated for number types only into a struct into a union and add scriptlanguage into the union as well without any growing runtime memory concerns.

But in the Java world every field we add, will add memory overhead, and this class is used very frequently. You would usually solve this with inheritance in Java, but that'd be a (de)serialization mess as a consequence.

@5iver
Copy link

5iver commented Mar 5, 2019

Would be awesome if you could add the mimetype as well.

The trouble with using mimetype is that there are multiple mimetypes that the script engine.will support. But they will only support the one (at least from what I've seen) file extension, which made it a lot easier.

getEngineName getEngineVersion getLanguageName getLanguageVersion getMimeTypes getExtensions
Oracle Nashorn 1.8.0_201 ECMAScript ECMA - 262 Edition 5.1 [application/javascript, application/ecmascript, text/javascript, text/ecmascript] js
Groovy Scripting Engine 2.0 Groovy 2.4.12 [application/x-groovy] groovy
jython 2.7.1 python 2.7 [text/python, application/python, text/x-python, application/x-python] py

But this would be the value selected in the 'type' configuration parameter, which you said you cannot access from the 'script' one, so it doesn't sound like the mimetype would be useful for you anyhow. Hmmm...

@davidgraeff
Copy link
Member Author

davidgraeff commented Mar 5, 2019

You can pick one of the mimetypes. Over time people invented different ones, true. But for javascript all browsers settled on "application/javascript", and "application/python" sounds correct (== mostly used) as well. I guess, the heuristic is to prefer the type with "application" in it and disfavor types with "x-" in it.

The mimetype is important for any UI inline editor. I'm using the visual studio code one (monaco) for example, and it requires me to set the mimetype for syntax highlighting etc.

If you just give me the file extension, I would need to add data to the client for performing the mapping, although you have the mimetype already at your fingertips.

@5iver
Copy link

5iver commented Mar 5, 2019

But this would be the value selected in the 'type' configuration parameter, which you said you cannot access from the 'script' one, so it doesn't sound like the mimetype would be useful for you anyhow.

@davidgraeff
Copy link
Member Author

At the moment I'm searching all configuration parameters for a context="scriptlanguage" one, when I encounter a context="script" and I'm assuming to find a mimetype. That's doable. It's a heuristic approach though: What if the developer specifies more than one "script" or "scriptlanguage" configuration parameter for a rule component.

So we either go with Markus suggestion of adding another field to the class, or we forbid to have more than one context "script" parameter in a rule component.

@5iver
Copy link

5iver commented Mar 5, 2019

What if the developer specifies more than one "script" or "scriptlanguage" configuration parameter for a rule component.

Then whatever solution we come up with here for ScriptAction and ScriptCondition will not work for their ModuleType in your UI.

So we either go with Markus suggestion of adding another field to the class

If I'm understanding @maggu2810's suggestion correctly, I do not see how adding a scriptlanguage parameter could work, since it's value would need to be an option that the user can select.

or we forbid to have more than one context "script" parameter in a rule component.

By 'rule component', I'm assuming you mean ModuleType. We're only talking about ScriptAction and ScriptCondition, so this should not be an issue. The 'script' context is specific to these ModuleTypes.

It is still not clear to me why a scriptlanguage context would need to be added, as it seems like you already have everything that you need. You're using the REST API to pull in the ModuleTypes, so you have the UIDs. If the UID is script.ScriptAction or script.ScriptCondition, then you know there will be a config description named 'type' and another named 'script'. The ParameterOption in the 'type' config description could contain more than one mimetype, and one will eventually be selected by the user. Each time that occurs, you would need to change the editor for the 'script' config description options to suit the mimetype selected, without losing the content. However, the next round of testing I do for my PR, I'll add in a context of scriptlanguage to see if there may be any issues.

My understanding is that you, or someone in the future, would be able to use the mimetype value selected by the user in the 'type' configuration description ParameterOption, so I will rework my PR to use them instead of file extension for the values of the options.

@5iver
Copy link

5iver commented Mar 6, 2019

In testing this, I found the values of the options are being displayed when the context is set to 'scriptlanguage', which is kinda ugly...

Without context
image

.withContext("scriptlanguage")
image

I also made the changes to use MimeType, so if the PR is accepted, the options will be...

I made the changes to use MimeType, and the values are showing up like this...

options
0
label "ECMAScript (ECMA - 262 Edition 5.1)"
value "application/javascript"
1
label "Groovy (2.4.12)"
value "application/x-groovy"
2
label "Python (2.7)"
value "application/python"

@davidgraeff
Copy link
Member Author

davidgraeff commented Mar 6, 2019

I mean Paper UI is not maintained, but for the meantime I can make Paper UI handle "scriptlanguage" contexts like regular texts (so not showing the value). For other UIs that context is still helpful, if we don't want to have that addition configuration class field "scriptlanguage".

If I'm understanding @maggu2810's suggestion correctly, I do not see how adding a scriptlanguage parameter could work, since it's value would need to be an option that the user can select.

It's like with min/max/step and all the other class fields. The UI need to support those, of course. But Paper UI doesn't care about that field anyway. It doesn't perform any syntax highlighting or validation. We would hack that functionality in, to display a selection box next to the multi-line text area, if that option is what we want to go for.

@5iver
Copy link

5iver commented Mar 8, 2019

but for the meantime I can make Paper UI handle "scriptlanguage" contexts like regular texts (so not showing the value).

I've submitted my PR (#635). I did not add the 'scriptlanguage' as the context, due to the change it would make in the UI. It will be a simple change to make, if/when Paper UI is updated. If you'd like, ping me when you submit the PR and I'll take care of the PR to add the conext into ScriptModuleTypeProvider.

For other UIs that context is still helpful, if we don't want to have that addition configuration class field "scriptlanguage".

IMO, this is clearly the best approach, with no user input needed, other than what is already being provided when selecting the script type.

@maggu2810
Copy link
Contributor

I lost track. What is the current status here?

@5iver
Copy link

5iver commented Jun 21, 2019

The file modified in this PR was removed in #635, so would need to be implemented in ScriptModuleTypeProvider. The proposed change also required another PR to go in before the new context could be added due to graphical issues in Paper UI, but that hasn't been submitted yet.

@ghys, will something like this be needed for the Paper UI replacement?

@cweitkamp cweitkamp added awaiting feedback stale As soon as a PR is marked stale, it can be removed 6 months later. labels Oct 2, 2019
@openhab-bot
Copy link
Collaborator

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

1 similar comment
@openhab-bot
Copy link
Collaborator

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

@ghys
Copy link
Member

ghys commented Oct 4, 2019

@openhab-5iver apologies for failing to follow up on your question...
As you mentioned above for scriptActions and scriptConditions (module types configuration) the language is defined in the sibling parameter "type" therefore is not constant. The UI has to consider that other parameter when initializing the script editor rather than the config description.
The only other case I know of where a "context: script" parameter is used is in HABPanel; the "panel registry" is actually a JSON file, and in that case, actually constant so it might be interesting to specify it in the config description e.g. scriptLanguage: "application/json"

@kaikreuzer
Copy link
Member

As this PR is stale, I'd tend to close it - if we have specific needs from UI side, we can at any time revive it.

@kaikreuzer kaikreuzer closed this Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation awaiting feedback stale As soon as a PR is marked stale, it can be removed 6 months later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants