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

Docs: document UI rules event and @runtime #110

Merged
merged 12 commits into from
May 13, 2022

Conversation

ssalonen
Copy link
Contributor

@ssalonen ssalonen commented Apr 24, 2022

Open questions

  • is @runtime available in file based rules? If not, it should be mentioned in docs

Not documented

  • trigger rules in UI
  • cron rules in UI
  • ...

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

Overall looks really well, just a few spelling and formatting points.

@florian-h05
Copy link
Contributor

is @runtime available in file based rules? If not, it should be mentioned in docs

Yes, it is also available in file-based scripts/rules.

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM👍

Thank you @ssalonen.

@florian-h05
Copy link
Contributor

We need @digitaldan or @jpg0 for merging.

Copy link
Contributor

@digitaldan digitaldan left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, i had only one suggestion

@florian-h05
Copy link
Contributor

@digitaldan
What about using Squash and Merge instead of just merging in this and any other future PR?
addons and core seem to squash and merge and this keeps commit history very clean.

@digitaldan
Copy link
Contributor

Yeah, i have been thinking this was happening already, but forgot repos defaults to not squashing, i just changed the settings so now squashing always happens, thanks for noticing !

@ssalonen
Copy link
Contributor Author

ssalonen commented May 7, 2022

I took the liberty of moving init/deinit hook sections: 9f03898

@digitaldan
Copy link
Contributor

@ssalonen this looks great, thanks! I like the advanced scripting section, i can see additional parts being added to that over time.

@digitaldan
Copy link
Contributor

Can you fix the conflicts that have popped up due to other PR merges?

ssalonen and others added 10 commits May 8, 2022 16:42
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Co-Authored-By: Dan Cunningham <dan@digitaldan.com>

Co-authored-by: Dan Cunningham <dan@digitaldan.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Also:
- title capitalization now reflecting code

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@ssalonen ssalonen force-pushed the doc-improvements branch from 9f03898 to 0e33899 Compare May 8, 2022 13:51
ssalonen added 2 commits May 8, 2022 16:53
Emulating merge error with openhab#99

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Emulating merge error with openhab#99

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@ssalonen ssalonen force-pushed the doc-improvements branch from 5f334f0 to 1fa5bc5 Compare May 8, 2022 13:57
@ssalonen
Copy link
Contributor Author

ssalonen commented May 8, 2022

@digitaldan should be now OK. PR #99 caused a lot of issue for rebase, it seemed to break havoc to many parts of the doc... Best to verify diff of this PR to ensure it is OK

@rkoshak
Copy link
Contributor

rkoshak commented May 9, 2022

is @runtime available in file based rules? If not, it should be mentioned in docs

I didn't see an answer to this. You can require(@runtime), however there are conflicts. For example, @runtime defines items to be a java.util.Map of Item names and their current state whereas openhab-js defines items to be your interface for doing all things related to Items and the Item registry and such. So if you are using openhab-js, you probably shouldn't be using @runtime.

@ssalonen
Copy link
Contributor Author

ssalonen commented May 9, 2022

is @runtime available in file based rules? If not, it should be mentioned in docs

I didn't see an answer to this. You can require(@runtime), however there are conflicts. For example, @runtime defines items to be a java.util.Map of Item names and their current state whereas openhab-js defines items to be your interface for doing all things related to Items and the Item registry and such.

It was confirmed that it is also in file-based rules.

So if you are using openhab-js, you probably shouldn't be using @runtime.

Personally I do not agree with this (to my mind) simplified statement. Without @runtime it is impossible to construct QuantityTypes and compare values, right? Or sure, you can import your QuantityType using Java.type as well.

However, you are correct that some of the functionality makes sense only for migration purposes. In fact, I have written guidance as follows:

require("@runtime") also defines "services" such as items, things, rules, events, actions, ir, itemRegistry.
You can use these services for backwards compatibility purposes or ease migration from JSR223 scripts.
Generally speaking, you should prefer to use Standard Library provided by this library instead.

Anecdotally, you can still do plenty without @runtime.

@florian-h05
Copy link
Contributor

Of course the use of @runtime can cause conflicts, but it is listed under advanced scripting and there is a note that the usage of openhab-js instead is recommended.
For some use cases, e.g. QuantityType, openhab-js is type of incomplete now and I think it is great to document @runtime even though openhab-js is recommended over it.

Without @runtime it is impossible to construct QuantityTypes and compare values, right? Or sure, you can import your QuantityType using Java.type as well.

That would be one-thing that we should improve in openhab-js.

@jpg0
Copy link
Collaborator

jpg0 commented May 9, 2022

To be clear, whilst we should prefer using openhab-js objects rather than importing @runtime, it should not cause any conflicts. No JS Dev who knows what they are doing will copy all the imports to the global namespace, this is just asking for trouble and no-one does it. They would import it to a const or let, named something like runtime and access it with runtime.items.

@rkoshak
Copy link
Contributor

rkoshak commented May 9, 2022

Without @runtime it is impossible to construct QuantityTypes and compare values, right?

No, you don't need the entirety of @runtime to work with QuantityTypes. And I've already opened an issue and started experimenting with ways to manage QuantityTypes in the library where possible (see #102) . Lack of operator overloading will make it challenging though. But ideally I'd like users to be able to compare and do math with numbers and QuantityTypes interchangeably without having to worry about units unless they want to.

Anyway, you have two choices. Option one is to throw out the units:

if(items.getItem('MyTemp').rawState.floatValue() > 65)

You can event force it to °F if you need to.

if(items.getItem('MyTemp').rawState.toUnit('°F').floatValue() > 65)

No imports are required at all with this approach but, of course, it works best when you are working with constants and such. If you are working with to QuantityTypes it's probably best not to throw out the units. It also requires you to know ahead of time that the Item is a Number:X.

But yes, you'd have to either Java.type it or just import QuantityType from @runtime, not all of @runtime to work strictly in QuantityTypes.

var QT = Java.type('org.openhab.core.library.types.QuantityType'); // or

if(items.getItem('MyTemp').rawState.compareTo(new QT(65, '°F')) > 1)

Importing just QuantityType from @runtime would look something like the following I think.

var {QuantityType} = require('@runtime');

I've no problem with documenting @runtime in the library at all. But I, like @florian-h05, do not use @runtime in any of my rules. In those exceptionally rare cases where I need something that is normally in @runtime, I Java.type them or import just those from @runtime. It will provide a much smoother experience for end users to not use @runtime unless they really know what they are doing because of the naming conflicts. The library also goes out of its way to wrap and abstract away from the raw openHAB Java Objects to provide a pure(ish) JavaScript environment whereas @runtime gives you raw Java which causes hard to understand (for many users) type problems.

So I agree it's an advanced topic and not something most users should be using as a kind of default import.

@jpg0
Copy link
Collaborator

jpg0 commented May 10, 2022

A rule of thumb as to when @runtime is required is when you need an instance of an osgi service from inside openHAB. You do not need it to import classes (they are not instances, nor inside the osgi container). You would need it to get hold of the various providers, registries etc.

@rkoshak
Copy link
Contributor

rkoshak commented May 10, 2022

A rule of thumb as to when @runtime is required is when you need an instance of an osgi service from inside openHAB

Wouldn't you want to use osgi.lookupService() or osgi.getService() for that? That's what I've been using.

@jpg0
Copy link
Collaborator

jpg0 commented May 10, 2022

Wouldn't you want to use osgi.lookupService() or osgi.getService() for that? That's what I've been using.

Sorry, yes, you're right. I forgot that I added this to the JS layer. I guess the only remaining reason is for anything which the devs have (a) added to @runtime but (b) not exposed in the JS layer. I hope this trends to nothing, leaving no remaining reason to use @runtime.

@ssalonen
Copy link
Contributor Author

ssalonen commented May 11, 2022

@runtime offers important migration path from Nashorn, as guided by @rkoshak https://community.openhab.org/t/migrate-from-nashorn-to-jsscripting-for-ui-rules/129788

(@rkoshak )

if(items.getItem('MyTemp').rawState.floatValue() > 65)

Personally, (and this is just me personally!), I prefer to have the framework protect against incompatible units and such.

Also, the newState and such are stringified (!) in file based rules, so you have to re-parse the string to utilize units.

As you said

If you are working with to QuantityTypes it's probably best not to throw out the units.


Don't get me wrong, I am all for 'native' js adaptation of core java types, and have those documented from a common place. I can imagine the current setup is jarring for new users as you need to lookup and combine documentation from many places.

This would be ideal, user would live in pure javascript world and would have no need to understand java and how the GraalVM adapts Java types into js.

@ssalonen ssalonen requested a review from digitaldan May 11, 2022 17:53
@rkoshak
Copy link
Contributor

rkoshak commented May 11, 2022

@runtime offers important migration path from Nashorn, as guided by @rkoshak

With the introduction of Nashorn as a separate add-on (see https://community.openhab.org/t/jsscripting-nashorn/134770) I don't think there really is a need for such migration any more. To move to JS Scripting with openhab-js will require a significant rewrite of the rules, a rewrite that needs to not include @runtime.

Even so, it's not really migration as much as it is backward compatibility. By including @runtime your Nashorn JS Rules will work in JS Scripting, but here's the key, without using openhab-js. openhab-js doesn't really make sense with @runtime so it's really an either/or situation. Either use openhab-js or use the raw JSR223 API. Both are indeed supported but not very well at the same time.

Personally, (and this is just me personally!), I prefer to have the framework protect against incompatible units and such.

Agreed, that's why I opened #102.

This would be ideal, user would live in pure javascript world and would have no need to understand java and how the GraalVM adapts Java types into js.

I think we all agree that is the goal for openhab-js. But @runtime brings back in all that Java stuff again, overriding some of the JS wrappers that openhab-js already provides. For the QuantityType issue in particular, I encourage everyone to join in the conversation on #102 and see if we can find some smart way to wrap them so we can get to a point where dealing with Java types at all (as well as complexities with dealing with values that have units and those that don't) becomes unnecessary most of the time. I've got some ideas and some experimental code but would love to get more ideas.

Also, the newState and such are stringified (!) in file based rules, so you have to re-parse the string to utilize units.

This is a decision I've struggled with. Early on a decision was made to just make everything a String. It makes a lot of sense most of the time but breaks down when you have QuantityTypes, as you point out.

You can get at the raw Java State Object from Item using rawState (as shown above).

You can maybe get at the raw Item's state from the Event Object using event.state (or what ever you've named the variable passed into the rule's action function). It's not clear that's how it works or not to me though (though I wonder why it's there at all if it doesn't). Unfortunately there isn't an equivalent to that for oldState, maybe it needs to be added.

And of course, in the UI we get that raw Java event Object an have to mess with the raw Java types. :-(

@digitaldan digitaldan merged commit 829a3ee into openhab:main May 13, 2022
@florian-h05 florian-h05 added the documentation Improvements or additions to documentation label May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants