-
-
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
[expiry] extend ExpiryManager to allow ignoring state updates #2739
Conversation
I don't understand why the test fails. I can't get it to fail on my MacBook Air M1 and not on an old Windows (i5-4570) machine. Neither with serial build nor with parallel build. Edit: I think I found it. The expiry check is only done every second, so sub-second waits may be problematic. |
Don't you have to change the event types subscriptions as well? If I understand it well, the ExpiryManager does not subscribe to the ItemStateChangedEvents at the moment. Or am I missing something? |
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
396bd0e
to
9d7962d
Compare
Signed-off-by: Jan N. Klug <github@klug.nrw>
} else if (event instanceof ItemCommandEvent) { | ||
ItemCommandEvent icEvent = (ItemCommandEvent) event; | ||
processEvent(icEvent.getItemName(), icEvent.getItemCommand()); | ||
} else if (event instanceof GroupItemStateChangedEvent) { |
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.
Is there a specific reason to remove this? I can see a use case for the expire functionality on a group (e.g. to turn off all lights in a group if they stay on for too long).
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.
GroupItemStateChangedEvent
is a sub-class of ItemStateChangedEvent
, so every GroupItemStateChangedEvent
will also fulfill this condition.
@@ -166,7 +170,7 @@ private void processEvent(String itemName, Type type) { | |||
private @Nullable ExpireConfig getExpireConfig(String itemName) { | |||
Optional<ExpireConfig> itemConfig = itemExpireConfig.get(itemName); | |||
if (itemConfig != null) { | |||
return itemConfig.isPresent() ? itemConfig.get() : null; | |||
return itemConfig.orElse(null); |
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.
return itemConfig.get() may be enough. You should remove the Nullable annotation on the itemExpireConfig map value. As it is a ConcurrentHashMap, null values are not allowed.
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.
Mistake on my side, you do need the code you put in, but you can still remove the @nullable annotation from the declaration.
I am just wondering if explictely making the syntax 'ignoreStateUpdates=true' or 'ignoreStateUpdates=false' (with a default of false) would not be cleaner. It also further reduces the (very limited) risk of conflict with a '..., ignoreStateUpdates' channel update value. I was on a path implementing that before I noticed you had already done this. |
The syntax
Meaning we now should use a syntax like this:
|
Signed-off-by: Jan N. Klug <github@klug.nrw>
@@ -325,6 +329,15 @@ public ExpireConfig(Item item, String configString) throws IllegalArgumentExcept | |||
? configString.substring(commaPos + 1).trim() | |||
: null; | |||
|
|||
Object ignoreStateUpdatesConfigObject = configuration.get("ignoreStateUpdates"); |
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.
Did you remember the ConfigParser
? Will be a one liner if you use it. 😉
And please refactor the String "ignoreStateUpdates"
to be a static final constant.
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.
Yes, but it's not available here. It's part of org.openhab.core.config.core
which is not a dependency of org.openhab.core
.
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.
Oh, I see. I missed that
void testIgnoreStateUpdateExtendsExpiryOnCommand() throws InterruptedException, ItemNotFoundException { | ||
Item testItem = new NumberItem(ITEMNAME); | ||
when(itemRegistryMock.getItem(ITEMNAME)).thenReturn(testItem); | ||
when(metadataRegistryMock.get(METADATA_KEY)).thenReturn(config("2s", Map.of("ignoreStateUpdates", true))); |
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.
The const can be reused in tests.
Signed-off-by: Jan N. Klug <github@klug.nrw>
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.
LGTM. Thanks.
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Fixes #1994
Fixes #2542
This needs an additional PR in UI to make it configurable via UI.
Signed-off-by: Jan N. Klug github@klug.nrw