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

Added group and system triggers to automation component #1509

Merged
merged 3 commits into from
Jun 4, 2020

Conversation

kaikreuzer
Copy link
Member

This PR completes the automation component to provide the same triggers as the "old" rule engine. It thus specifically adds modules for "member of group" triggers as well as a start level trigger, which can be used to run rules once at system startup (disclaimer: No start level events are defined in this PR yet, so the trigger itself is still rather useless).

This was part of #1451 and has been separated as an independent PR that can already been reviewed and merged independently.

Signed-off-by: Kai Kreuzer kai@openhab.org

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Thanks. I left some comments inside.

"description": "the received command",
"required": false,
"limitToOptions": false,
"options": [
Copy link
Contributor

Choose a reason for hiding this comment

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

What about other Commands (e.g. PLAY, PAUSE, etc.)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Might become a rather long list - I did it the same way here as we have it for all the other item triggers, so this is in line. It can be discussed whether only the most frequently used commands should be listed or all enum types, but this can be done in a separate PR if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know. Does it make sense to add / introduce a "context": "command" for these options? Then is can be handled globally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be an option, but it would possibly shift the implementation work to the UIs then.
I would wait and see how it is really used in daily life and what we feel is missing and should be changed. I don't have a clear view on it yet.

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer
Copy link
Member Author

Thanks @cweitkamp, I have addressed your comments.

Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Thanks for addressing them. Do not call me nit-picking. I some additional remarks.

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer
Copy link
Member Author

Thanks again, @cweitkamp, everything should be addressed now!

@kaikreuzer kaikreuzer requested a review from cweitkamp June 3, 2020 20:05
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your work.

@cweitkamp cweitkamp added this to the 3.0 milestone Jun 4, 2020
@cweitkamp cweitkamp merged commit d265e16 into openhab:master Jun 4, 2020
@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/jython-when-item-added-removed-modified-not-working/100994/4

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
…penhab#1509)

* Added group and system triggers to automation component

Signed-off-by: Kai Kreuzer <kai@openhab.org>
GitOrigin-RevId: d265e16
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.

3 participants