Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

AutomaticInboxProcessor registered as an RegistryListener #4192

Merged
merged 2 commits into from
Sep 6, 2017
Merged

AutomaticInboxProcessor registered as an RegistryListener #4192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fab34d6
AutomaticInboxProcessor registered as an RegistryListener for the thi…
afuechsel Sep 5, 2017
fce857d
Removed unnecessary comments
afuechsel Sep 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 4 additions & 12 deletions ...est/java/org/eclipse/smarthome/config/discovery/internal/AutomaticInboxProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,14 @@ public void testThingWentOnline() {
assertThat(results.size(), is(1));
assertThat(results.get(0).getThingUID(), is(equalTo(THING_UID)));
}

@Test
public void testNoDiscoveryResultIfNoRepresentationPropertySet() {
List<DiscoveryResult> results = inbox.stream().filter(withFlag(DiscoveryResultFlag.NEW))
.collect(Collectors.toList());
assertThat(results.size(), is(0));
}

@Test
public void testThingWhenNoRepresentationPropertySet() {
inbox.add(DiscoveryResultBuilder.create(THING_UID).withProperty(DEVICE_ID_KEY, DEVICE_ID).build());
Expand Down Expand Up @@ -215,11 +215,7 @@ public void testThingIsBeingRemoved() {
assertThat(results.size(), is(1));
assertThat(results.get(0).getThingUID(), is(equalTo(THING_UID)));

when(thingRegistry.get(THING_UID)).thenReturn(thing);
when(thingStatusInfoChangedEvent.getStatusInfo())
.thenReturn(new ThingStatusInfo(ThingStatus.REMOVING, ThingStatusDetail.NONE, null));
when(thingStatusInfoChangedEvent.getThingUID()).thenReturn(THING_UID);
inboxAutoIgnore.receive(thingStatusInfoChangedEvent);
inboxAutoIgnore.removed(thing);

results = inbox.getAll();
assertThat(results.size(), is(0));
Expand Down Expand Up @@ -278,11 +274,7 @@ public void testThingWithConfigIsBeingRemoved() {
assertThat(results.size(), is(1));
assertThat(results.get(0).getThingUID(), is(equalTo(THING_UID2)));

when(thingRegistry.get(THING_UID2)).thenReturn(thing2);
when(thingStatusInfoChangedEvent.getStatusInfo())
.thenReturn(new ThingStatusInfo(ThingStatus.REMOVING, ThingStatusDetail.NONE, null));
when(thingStatusInfoChangedEvent.getThingUID()).thenReturn(THING_UID2);
inboxAutoIgnore.receive(thingStatusInfoChangedEvent);
inboxAutoIgnore.removed(thing2);

results = inbox.getAll();
assertThat(results.size(), is(0));
Expand Down
32 changes: 23 additions & 9 deletions ...rc/main/java/org/eclipse/smarthome/config/discovery/internal/AutomaticInboxProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.eclipse.smarthome.config.discovery.DiscoveryResultFlag;
import org.eclipse.smarthome.config.discovery.inbox.Inbox;
import org.eclipse.smarthome.config.discovery.inbox.InboxListener;
import org.eclipse.smarthome.core.common.registry.RegistryChangeListener;
import org.eclipse.smarthome.core.events.AbstractTypedEventSubscriber;
import org.eclipse.smarthome.core.events.EventSubscriber;
import org.eclipse.smarthome.core.thing.Thing;
Expand Down Expand Up @@ -55,7 +56,7 @@
"service.config.description.uri=system:inbox", "service.config.label=Inbox", "service.config.category=system",
"service.pid=org.eclipse.smarthome.inbox" })
public class AutomaticInboxProcessor extends AbstractTypedEventSubscriber<ThingStatusInfoChangedEvent>
implements InboxListener {
implements InboxListener, RegistryChangeListener<Thing> {

private final Logger logger = LoggerFactory.getLogger(this.getClass());

Expand Down Expand Up @@ -97,12 +98,6 @@ public void thingAdded(Inbox inbox, DiscoveryResult result) {
}
}

private String getRepresentationValue(DiscoveryResult result) {
return result.getRepresentationProperty() != null
? Objects.toString(result.getProperties().get(result.getRepresentationProperty()), null)
: null;
}

@Override
public void thingUpdated(Inbox inbox, DiscoveryResult result) {
}
Expand All @@ -111,11 +106,28 @@ public void thingUpdated(Inbox inbox, DiscoveryResult result) {
public void thingRemoved(Inbox inbox, DiscoveryResult result) {
}

@Override
public void added(Thing element) {
}

@Override
public void removed(Thing element) {
removePossiblyIgnoredResultInInbox(element);
}

@Override
public void updated(Thing oldElement, Thing element) {
}

private String getRepresentationValue(DiscoveryResult result) {
return result.getRepresentationProperty() != null
? Objects.toString(result.getProperties().get(result.getRepresentationProperty()), null)
: null;
}

private void autoIgnore(Thing thing, ThingStatus thingStatus) {
if (ThingStatus.ONLINE.equals(thingStatus)) {
checkAndIgnoreInInbox(thing);
} else if (ThingStatus.REMOVING.equals(thingStatus)) {
removePossiblyIgnoredResultInInbox(thing);
}
}

Expand Down Expand Up @@ -195,9 +207,11 @@ protected void activate(Map<String, Object> properties) {
@Reference
protected void setThingRegistry(ThingRegistry thingRegistry) {
this.thingRegistry = thingRegistry;
thingRegistry.addRegistryChangeListener(this);
}

protected void unsetThingRegistry(ThingRegistry thingRegistry) {
thingRegistry.removeRegistryChangeListener(this);
this.thingRegistry = null;
}

Expand Down
1 change: 1 addition & 0 deletions ...binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/handler/HueBridgeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ private void onConnectionResumed() throws IOException, ApiException {
properties.put(Thing.PROPERTY_SERIAL_NUMBER, config.getMACAddress().replaceAll(":", "").toLowerCase());
properties.put(Thing.PROPERTY_FIRMWARE_VERSION, config.getSoftwareVersion());
updateProperties(properties);
updateThing(thing);
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this change related to the AutomaticInboxProcessor registered as an RegistryListener?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the properties would not be persisted, the automatic inbox listener would be unable to retrieve the representation property during deletion. Please note, that this was on purpose as long as the hue binding was updating the properties in each polling cycle. Now that it is guarded, that problem won't exist anymore and the properties should be persisted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just came across this here. How does it relate to #1682 and esp. #588 (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SJKA I see your point but I wonder how to solve this issue without persisting the properties. As the thing is already gone when the removed callback is called I would not be able to get the property value. Hmm.

Copy link
Contributor

Choose a reason for hiding this comment

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

@afuechsel I is always a bad smell to do some specific code changes in one binding for a concept that should be applicable in the whole framework (and thus be the same for all bindings). Note that after #1682 had been implemented, we meanwhile had #2629 in place. This allows to do Thing updates (structure AND configuration) and persists those updates in case it is a managed Thing. Imho, what is missing in that PR is that it also tries to persist properties in the very same way. If this was done, it would be in line with how configuration updates are handled and it would be exactly what you need here (i.e. this line can be removed as the framework already takes care of everything). Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, if properties would be persisted in some other way, this line could be removed. But did I understand correctly, that there is still some tiny piece missing (persisting the properties)? Would that mean to revert #1682?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. See #4688.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, then this PR needs to be changed too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. See #4688.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't see it in the first place :)

propertiesInitializedSuccessfully = true;
}
}
Expand Down
1 change: 1 addition & 0 deletions ....binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/handler/HueLightHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ private synchronized void initializeProperties() {
}
updateProperty(LIGHT_UNIQUE_ID, fullLight.getUniqueID());
isOsramPar16 = OSRAM_PAR16_50_TW_MODEL_ID.equals(modelId);
updateThing(thing);
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this change related to the AutomaticInboxProcessor registered as an RegistryListener?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

propertiesInitializedSuccessfully = true;
}
}
Expand Down