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

enrich memberOf with options to just listen on items or groups #95

Merged
merged 8 commits into from
Feb 12, 2023

Conversation

querdenker2k
Copy link
Collaborator

@querdenker2k querdenker2k commented Nov 26, 2022

@JRuleWhenItemChange(item = GROUP_ITEM, memberOf = true)
changed to
@JRuleWhenItemChange(item = GROUP_ITEM, memberOf = JRuleMemberOf.All)
or
@JRuleWhenItemChange(item = GROUP_ITEM, memberOf = JRuleMemberOf.Items)
or
@JRuleWhenItemChange(item = GROUP_ITEM, memberOf = JRuleMemberOf.Groups)

In some of my rules I have something like that:

if (JRuleItem.forName(event.getMemberName()).isGroup()) {
    return;
}

To not do this inside the rule method, the rule should just be triggered for Items (and not Groups).

Further on the triggered itemName will be in itemName and not in memberName anymore, because #93

@seime
Copy link
Collaborator

seime commented Dec 11, 2022

I like the idea.
And would like to propose a similar extension to listen for events for members in subgroups. Ie
ITEM_A memberOf GROUP_A memberOf GROUP_B

It would be great if I could get changes about ITEM_A if I listen for changes in GROUP_B even if ITEM_A isn't a direct member of GROUP_B but only indirectly via GROUP_A

Use case: Motion sensing in different zones of the premises. If I have multiple sensors in GROUP_A and the group status isn't changed, I do not think listening for memberOf events in GROUP_B triggers.

@seime
Copy link
Collaborator

seime commented Dec 11, 2022

Also remember to update the README with updated/new examples :)

@querdenker2k
Copy link
Collaborator Author

I like the idea. And would like to propose a similar extension to listen for events for members in subgroups. Ie ITEM_A memberOf GROUP_A memberOf GROUP_B

It would be great if I could get changes about ITEM_A if I listen for changes in GROUP_B even if ITEM_A isn't a direct member of GROUP_B but only indirectly via GROUP_A

Use case: Motion sensing in different zones of the premises. If I have multiple sensors in GROUP_A and the group status isn't changed, I do not think listening for memberOf events in GROUP_B triggers.

Thought about the same and started doing it but reverted it again. Maybe we could this in an additional MR. I have to much open MRs and I have to merge them all again in my own developer branch. This should be simply from the user view with additional enums for JRuleMemberOf

@seime
Copy link
Collaborator

seime commented Dec 28, 2022

@querdenker2k could you rebase and resolve conflicts?

# Conflicts:
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemChangeExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemReceivedCommandExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemReceivedUpdateExecutionContext.java
@querdenker2k querdenker2k marked this pull request as draft December 28, 2022 17:36
Copy link
Collaborator

@seime seime left a comment

Choose a reason for hiding this comment

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

Is this PR still a draft or ready for review?

@querdenker2k
Copy link
Collaborator Author

not ready yet, have to resolve merge conflicts (therefor the outcommented tests for better debugging) but will not get it to work in the next 5 days.

# Conflicts:
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemReceivedCommandExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemReceivedUpdateExecutionContext.java
#	src/test/java/org/openhab/automation/jrule/rules/user/TestRules.java
@querdenker2k querdenker2k marked this pull request as ready for review January 11, 2023 11:50
@querdenker2k
Copy link
Collaborator Author

Ready again, added IT tests.
Currently this is working just for direct members of groups (as before).
Additionaly an *_Recursive could be added to the enum.

@querdenker2k querdenker2k requested a review from seime January 11, 2023 13:41
@querdenker2k
Copy link
Collaborator Author

PUSH

Copy link
Collaborator

@seime seime left a comment

Choose a reason for hiding this comment

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

Not tested, but has unit tests

@seime seime self-requested a review February 11, 2023 18:13
@seime
Copy link
Collaborator

seime commented Feb 11, 2023

@querdenker2k Tested this a bit tonight, and something is not correct.

I replaced all my memberOf = true with memberOf = JRuleMemberOf.All as indicated in the README file.

When using JRuleMemberOf.All, rules listening for changes in a specific group suddenly gets triggered by changes happening to items in unrelated groups.

Here is one example;
CleanShot 2023-02-11 at 19 15 48

CleanShot 2023-02-11 at 19 16 11

The event claims to be for group gFloodSensors, but the member is part of gMovementSensorsHouse. In fact the gFloodSensors group does not have any members at the moment, and the groups are unrelated.

I have also seen my batteryGroup monitoring rules suddenly getting triggered with motion sensor events etc.

Not sure where the problem is; either parsing of which groups contains which members or the rule filtering mechanism.

Changing from All to Items seem to stop this erratic behaviour, but not really sure if this makes it correct.

@querdenker2k
Copy link
Collaborator Author

Damn, will look into it. Could already create a negative test which describes your behaviour.

@querdenker2k
Copy link
Collaborator Author

fixed this, problem was very stupid

@seime
Copy link
Collaborator

seime commented Feb 12, 2023

Appears to be working as expected now!

Thanks @querdenker2k

@seime seime merged commit b3cb4a4 into seaside1:main Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants