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

4770 gcal oh2 compatibility #4817

Merged
merged 4 commits into from Nov 29, 2016

Conversation

SirAd
Copy link
Contributor

@SirAd SirAd commented Nov 24, 2016

Remove ConsoleInterpreter class no longer support in OH2. Replaced with EventPublisher class (described in ticket #4770 )

Copy link
Contributor

@watou watou left a comment

Choose a reason for hiding this comment

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

Looks like a nice solution to handling conditional running of PresenceSimulation without requiring the script interpreter! Just a few review comments. Please let me know if you would like me to try it out on OH2.

@@ -27,6 +27,7 @@ Import-Package: com.google.common.collect;version="1.0.0",
org.osgi.service.cm,
org.osgi.service.component,
org.osgi.service.event,
org.osgi.util.tracker;version="1.5.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless this version constraint is needed, please remove it.

Item item = registry.getItem("PresenceSimulation");
if (item.getState() != OnOffType.ON) {
logger.info(
"Presence Simulation job detected, but PresenceSimulation is not in ON state. Job is not executed");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be reduced to debug, otherwise there might be many unneeded log entries. WDYT?

}
} catch (ItemNotFoundException e) {
logger.error(
"Presence Simulation job detected, but PresenceSimulation item does not exists. Check configuration");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be reduced to warn, since the situation does not lead to system instability.

State state = TypeParser.parseState(item.getAcceptedDataTypes(), args[2]);
publisher.postUpdate(item.getName(), state);
logger.debug("Published update {}", Arrays.asList(args));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to log a warn here instead of silently doing no postUpdate.

@watou watou added the openHAB2 label Nov 25, 2016
@watou watou added this to the 1.9.0 milestone Nov 25, 2016
@SirAd
Copy link
Contributor Author

SirAd commented Nov 25, 2016

I added requested after review

@watou
Copy link
Contributor

watou commented Nov 25, 2016

Thanks @SirAd! Please report when you think it's good to merge, and I will give it a spin (io and persistence). When the wiki is updated, it will be done and very nice to have it working on OH2!

@SirAd
Copy link
Contributor Author

SirAd commented Nov 25, 2016

I did tests on OH1 - all looks fine. I also put some basic tests in OH2 environment, but it would be great if somebody could do some more tests. Wiki is update. In my opinion PR is ready for merge.

@kaikreuzer kaikreuzer merged commit 6f9a2ba into openhab:master Nov 29, 2016
@kaikreuzer
Copy link
Member

Great, thanks!

mvolaart pushed a commit to mvolaart/openhab that referenced this pull request Dec 23, 2016
* GCal - update for OH2 compatibility
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants