[gcal] Add io and persistence feature to OH2 #4891

mhilbush · 2016-12-12T16:42:06Z

Signed-off-by: Mark Hilbush mark@hilbush.com

Signed-off-by: Mark Hilbush <mark@hilbush.com>

watou

Thanks @mhilbush! In addition to my review comments, could you add an ESH-INF/config/config.xml like the JDBC persistence bundle does, so users can configure the io.gcal service without having to edit the gcal.cfg file at all? If you add this, also change GCalEventDownloader.java to replace constructs like (String) config.get("X") with ObjectUtils.toString(config.get("X"), null) since configuring from Paper UI might put non-String objects in the configuration?

watou · 2016-12-12T17:08:11Z

features/openhab-addons/src/main/feature/feature.xml


+  <feature name="openhab-persistence-gcal" description="GCal Persistence" version="${project.version}">
+    <feature>openhab-runtime-base</feature>
+    <feature>openhab-runtime-compat1x</feature>


openhab-persistence-gcal depends on openhab-io-gcal1 (it imports the org.openhab.io.gcal.auth package)

Ok. Dependency added.

watou · 2016-12-12T17:10:10Z

features/openhab-addons/src/main/feature/feature.xml

+    <feature>openhab-runtime-base</feature>
+    <feature>openhab-runtime-compat1x</feature>
+    <bundle start-level="80">mvn:org.openhab.io/org.openhab.io.gcal/${project.version}</bundle>
+  </feature>


Could you add a default gcal.cfg file similar to what is done on line 667?

The binding currently expects the config info in openhab.cfg. I'm not quite sure what needs to be done here besides adding the line to feature.xml.

The binding itself actually doesn't know where its configuration comes from. The 1.x runtime used openhab.cfg, but it's refactored in OH2 to have a .cfg per service, gcal.cfg in this case, for text file configuration. Or via Paper UI so no config file has to be used.

To add gcal.cfg, just take the GCal section from openhab_default.cfg into features/openhab-addons-external/src/main/resources/conf/gcal.cfg (but remove the gcal: prefixes in it). Modify features/openhab-addons-external/pom.xml like in #4814.

To allow GCal to be configured from the Paper UI, add bundles/io/org.openhab.io.gcal/ESH-INF/config/config.xml similar to #4049, and see this documentation.

mhilbush · 2016-12-13T12:01:14Z

I made the changes for gcal.cfg. openhab_default.cfg contained the old (pre-OAuth) config parameters, so I just added client_id, client_secret, and calendar_name to gcal.cfg.

I didn't do the config.xml change, as I'd prefer to get this part right first. :-)

watou · 2016-12-13T12:25:51Z

features/openhab-addons-external/src/main/resources/conf/gcal.cfg

+#client_secret=
+
+# The name of the Google Calendar that GCal should use
+#calendar_name=


Looks good, but I worry that users might not know which are required or optional. So a bit more information as described here might avoid head scratching or hair pulling. Also, when you've updated the helpfulness of the comments a bit, could you also fix openhab_default.cfg to match (adding back the gcal: prefixes in that version)?

Sure. Good point. I didn't want to repeat what was in the Wiki, but I certainly can add more to the comments.

As for openhab_default.cfg, I'm not sure what you're asking as I didn't change that file. It still contains the gcal: prefixes.

openhab_default.cfg contained the old (pre-OAuth) config parameters

I was hoping this PR would be a good opportunity for consistency between the now two .cfg files, seeing as you report that we missed updating openhab_default.cfg file with @SirAd's big refresh.

Ah, I see now. :-/

I wasn't sure if that file was supposed to be updated. I can do that. Note that there also are some gcal-persistence parameters in openhab_default.cfg. Can those be added to gcal.cfg (in a separate section of the file explaining their use for the persistence feature), or is a separate gcal-persistence.cfg needed?

#gcal-persistence:offset= #gcal-persistence:executescript=

What about the Wiki? It still references openhab.cfg and gcal: prefixes?

And, I see there are some log messages in the code that reference openhab.cfg...

Actually, looking at this more closely, GCal Persistence has it's own calendar_name parameter, which may dictate a separate cfg file to avoid conflicting with GCal's calendar_name parameter. WDYT?

#gcal-persistence:calendar_name= #gcal-persistence:offset= #gcal-persistence:executescript=

watou · 2016-12-13T12:28:09Z

I didn't do the config.xml change, as I'd prefer to get this part right first. :-)

Fair enough. Allowing GCal to be configured from Paper UI could be left to another PR if you prefer. So I only would like to see the .cfg files made more "idiot-proof" for this PR. Thanks, Mark!

mhilbush · 2016-12-14T12:53:50Z

Latest commit has the following changes:

updated gcal.cfg comments
added gcal-persistence.cfg
openhab_default.cfg now matches gcal.cfg and gcal-persistence.cfg

watou · 2016-12-14T13:02:32Z

distribution/openhabhome/configurations/openhab_default.cfg

+# The filter criteria by which calendar events are searched. The Google Calendar 
+# API will do a text search to find calendar events that match the supplied terms. 
+# All calendar event fields are searched, except for extended properties (optional)
+#filter=


Prepend gcal:?

watou · 2016-12-14T13:02:37Z

distribution/openhabhome/configurations/openhab_default.cfg

+# Refresh interval (in milliseconds) is the frequency with which the
+# Google calendar will be checked for calendar events (optional, defaults 
+# to 900000 [15 minutes])
+#refresh=


Prepend gcal:?

watou · 2016-12-14T13:03:26Z

distribution/openhabhome/configurations/openhab_default.cfg

-
-# the url of the calendar feed
-#gcal-persistence:url=
+# The following parameters are used to configure the GCal Persistence binding.


"service", not "binding"

watou · 2016-12-14T13:04:21Z

distribution/openhabhome/configurations/openhab_default.cfg

-# represents the State (optional, defaults to
-# '> if (PresenceSimulation.state == ON) sendCommand(%s,%s)')
+# represents the State (optional, defaults to 'send %s %s')
 #gcal-persistence:executescript=


This feature doesn't exist any more, does it?

I left it in because it's still pulled from the config on line 116, and is used on line 166, in GCalPersistenceService.java

watou · 2016-12-14T13:04:49Z

features/openhab-addons-external/src/main/resources/conf/gcal-persistence.cfg

+# the GCal-based presence simulation. It must contain two format markers '%s'.
+# The first marker represents the Item to send the command to and the second
+# represents the State (optional, defaults to 'send %s %s')
+#executescript=


Feature doesn't exist any more, does it?

watou · 2016-12-14T13:08:58Z

features/openhab-addons-external/src/main/resources/conf/gcal.cfg

-# Copied from Credentials page on console.developers.google.com
+################################### GCal Binding ###################################
+#
+# Before using the GCal bindings in openHAB2, you need to have a Google API


Not "bindings," more like "services." Yes, it's a bit confusing. ;-)

mhilbush · 2016-12-14T13:30:21Z

I've corrected everything except for gcal-persistence:executescript=. Since it was still used in the code, I left it in the config. WDYT?

Sorry for all the revisions...

watou · 2016-12-15T13:41:52Z

Sorry for my mistake, @mhilbush. The name executescript is indeed still used, but it's not actually capable of being a script fragment any more so I got myself confused by the old name. Its only valid formats are now send %s %s or update %s %s.

mhilbush · 2016-12-15T13:48:32Z

No worries. Then I think I've incorporated all your feedback so far.

However... I just tried to install gcal-persistence in OH2 and I got the below error. Looks like gcal-persistence imports a bunch of ch.qos.logback.* packages. Not familiar with that, but I can look into it.

2016-12-15 08:24:23.662 [WARN ] [org.apache.felix.fileinstall        ] - Error while starting bundle: file:/opt/openhab/addons/org.openhab.persistence.gcal-1.9.0-SNAPSHOT.jar
org.osgi.framework.BundleException: Could not resolve module: org.openhab.persistence.gcal [198]
  Unresolved requirement: Import-Package: ch.qos.logback.classic
        at org.eclipse.osgi.container.Module.start(Module.java:434)[org.eclipse.osgi-3.10.2.v20150203-1939.jar:]
        at org.eclipse.osgi.internal.framework.EquinoxBundle.start(EquinoxBundle.java:393)[org.eclipse.osgi-3.10.2.v20150203-1939.jar:]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.startBundle(DirectoryWatcher.java:1245)[8:org.apache.felix.fileinstall:3.5.0]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.startBundles(DirectoryWatcher.java:1217)[8:org.apache.felix.fileinstall:3.5.0]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.doProcess(DirectoryWatcher.java:509)[8:org.apache.felix.fileinstall:3.5.0]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.process(DirectoryWatcher.java:358)[8:org.apache.felix.fileinstall:3.5.0]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.run(DirectoryWatcher.java:310)[8:org.apache.felix.fileinstall:3.5.0]

watou · 2016-12-15T13:50:27Z

distribution/openhabhome/configurations/openhab_default.cfg

-#gcal:url=
+# This is the name you gave to your Google Calendar.  GCal will download calendar 
+# events from this calendar (required)
+#gcal:calendar_name=


The code says:

String urlString = (String) config.get("calendar_name"); if (!StringUtils.isBlank(urlString)) { calendar_name = urlString; } else { logger.error( "gcal:calendar_name must be configured in openhab.cfg. Calendar name or word \"primary\" MUST be specified"); throw new ConfigurationException("calendar_name", "gcal:calendar_name must be configured in openhab.cfg. Calendar name or word \"primary\" MUST be specified"); }

but the member calendar_name defaults to "primary" as well. So the code might allow this to be optional. For this PR, could you add that the special keyword primary" can be used? Ideally it would be verified that specifying calendar_name is optional and defaults to primary and the code changed to allow it to be optional (and tested!). 😄

I've confirmed the following:

both gcal and gcal-persistence require a calendar name in the cfg. If not supplied, it doesn't default to anything. It does log errors in the log file, however.

using the keyword primary does use your default Google calendar. My default calendar is now full of presence simulation events. :-(

Therefore, I'm not sure it's a good idea to default to primary if no calendar_name is specified (at least for gcal-presence), unless one wants to discover a bunch of presence simulation events in their default Google calendar.

I would propose that I update the cfg comments (gcal, gcal-persistence, and openhab_default) to reflect the usage of the keyword primary, but leave calendar_name required.

Also, in order to get gcal-persistence to run in OH2, I needed to remove these imported packages from MANIFEST.MF. Should I include an updated MANIFEST.MF in this PR?

ch.qos.logback.classic, ch.qos.logback.classic.encoder, ch.qos.logback.classic.spi, ch.qos.logback.core, ch.qos.logback.core.encoder, ch.qos.logback.core.pattern, ch.qos.logback.core.spi,

both gcal and gcal-persistence require a calendar name in the cfg. If not supplied, it doesn't default to anything. It does log errors in the log file, however.

The code does log the error and throw the exception, but the calendar_name is initialized to "primary" in the class so it didn't make sense -- either have no default for the member, or allow it to be optional. In any case, I'm fine with requiring the user to explicitly set the calendar, for the reasons you gave. Yes, please update the .cfg files to make it clear, and thanks!

Should I include an updated MANIFEST.MF in this PR?

Beat you to it! #4899 😄

watou · 2016-12-15T15:55:40Z

Thanks for trying the persistence bundle on OH2, @mhilbush. I've submitted #4899 to fix the problem you saw. Consider replacing the bundle with this one if you have a chance to test again.

mhilbush · 2016-12-15T16:02:19Z

Confirmed that version works. Thanks, @watou.

Are you ok with the above approach for updating the cfg file comments wrt primary keyword?

[[Nm. I saw your other comment above.]]

mhilbush · 2016-12-15T16:28:41Z

Just pushed the changes re: primary keyword. Anything else you think needs to be done on this?

watou · 2016-12-15T16:29:28Z

LGTM! Anything else from your side before I merge?

mhilbush · 2016-12-15T16:29:46Z

Nope. I'm good.

mhilbush · 2016-12-15T16:31:22Z

BTW, thanks for your help and feedback.

watou · 2016-12-15T16:40:11Z

Thanks for your contribution! It's great to see GCal up and usable again with the group effort! 😄

mhilbush · 2016-12-15T16:43:47Z

It looks like there are some minor edits needed to the GCal Wiki page. Any issue with me making those changes?

watou · 2016-12-15T16:46:07Z

Please go right ahead! Having all questions answered by clear documentation is really the most important thing of all!

watou · 2016-12-15T17:09:25Z

Big thanks again to @SirAd for making it possible with the OAuth2 update.

mhilbush · 2016-12-16T13:03:43Z

@watou I must've done something wrong in feature.xml. gcal-persistence is showing up in PaperUI, but not gcal. This is the feature name I gave it.

<feature name="openhab-io-gcal1" description="GCal" version="${project.version}">

Should it be openhab-binding-gcal1, and should it be up in the binding section?

watou · 2016-12-16T13:19:23Z

Is the service not showing up under the Misc. Tab?

mhilbush · 2016-12-16T13:21:54Z

Correct. Not showing under any tab (including misc).

watou · 2016-12-16T13:25:08Z

In several hours I will be able to have a look.

mhilbush · 2016-12-16T15:45:27Z

From what I can see, currently the only features showing in Misc are from OH2.

Do we need a misc section in the openhab feature.xml, then change the gcal entry to:

<feature name="openhab-misc-gcal1" description="GCal" version="${project.version}">
    <feature>openhab-runtime-base</feature>
    <feature>openhab-runtime-compat1x</feature>
    <bundle start-level="80">mvn:org.openhab.io/org.openhab.io.gcal/${project.version}</bundle>
    <configfile finalname="${openhab.conf}/services/gcal.cfg" override="false">mvn:${project.groupId}/openhab-addons-external/${project.version}/cfg/gcal</configfile>
</feature>

And change gcal-persistence to:

<feature name="openhab-persistence-gcal" description="GCal Persistence" version="${project.version}">
    <feature>openhab-runtime-base</feature>
    <feature>openhab-runtime-compat1x</feature>
    <feature>openhab-misc-gcal1</feature>
    <bundle start-level="80">mvn:org.openhab.persistence/org.openhab.persistence.gcal/${project.version}</bundle>
    <configfile finalname="${openhab.conf}/services/gcal-persistence.cfg" override="false">mvn:${project.groupId}/openhab-addons-external/${project.version}/cfg/gcal-persistence</configfile>
</feature>

watou · 2016-12-16T15:48:37Z

My understanding is that it's done by convention, so your change makes sense. I will delve deeper to find out for sure.

kaikreuzer · 2016-12-16T15:54:59Z

openhab-misc-gcal1 will indeed make it appear on the "MISC" tab under Extensions in the Paper UI - this should be used for any feature that is installable by the user, while openhab-io... are only shared bundles that are required by other features, but do not make sense on their own.

But if you expose it to the UI, you should think of a slighly longer name than "GCal" for it, please!

watou · 2016-12-16T16:09:19Z

But if you expose it to the UI, you should think of a slighly longer name than "GCal" for it, please!

I was just about to suggest that its name be changed to a name that makes it clear 1) what is being integrated into openHAB, and 2) what function it performs. Something like "Google Calendar Scheduler" or "Google Calendar Connector".

mhilbush · 2016-12-16T16:17:12Z

I like Google Calendar Scheduler.

Since OH1 users already know it by GCal, could it be this, as there appears to be precedent for having a parenthetical in the name:
Google Calendar (GCal) Scheduler

And, for gcal-persistence, it could be one of these, which is more descriptive of what it actually does:

Google Calendar Presence Simulation
Google Calendar (GCal) Presence Simulation

mhilbush · 2016-12-16T16:19:22Z

Or, to be a bit more consistent with Scheduler...

Google Calendar Presence Simulator
Google Calendar (GCal) Presence Simulator

watou · 2016-12-16T16:33:45Z

I don't see a need for (GCal) but I like your suggestions.

mhilbush · 2016-12-16T16:36:06Z

Alright, if there are no objections, I'll do:

Google Calendar Scheduler
Google Calendar Presence Simulator

* [gcal] Add io and persistence feature to OH2 Signed-off-by: Mark Hilbush <mark@hilbush.com> * Additional changes for OH2 distro * Additional changes for OH2 distro * Correction and comment revision. * Clarify usage of primary keyword for calendar_name

[gcal] Add io and persistence feature to OH2

293c47c

Signed-off-by: Mark Hilbush <mark@hilbush.com>

watou suggested changes Dec 12, 2016

View reviewed changes

watou added awaiting-feedback openHAB2 labels Dec 12, 2016

watou added this to the 1.9.0 milestone Dec 12, 2016

Additional changes for OH2 distro

9179f88

watou reviewed Dec 13, 2016

View reviewed changes

Additional changes for OH2 distro

60923c1

watou reviewed Dec 14, 2016

View reviewed changes

Correction and comment revision.

fbd3805

watou reviewed Dec 15, 2016

View reviewed changes

watou mentioned this pull request Dec 15, 2016

[gcal-persistence] Remove unneeded ch.qos.logback imports #4899

Merged

Clarify usage of primary keyword for calendar_name

4cd3cec

watou approved these changes Dec 15, 2016

View reviewed changes

watou removed the awaiting-feedback label Dec 15, 2016

watou merged commit b2f779c into openhab:master Dec 15, 2016

mhilbush deleted the gcal-feature branch December 15, 2016 16:35

Uh oh!

[gcal] Add io and persistence feature to OH2 #4891

[gcal] Add io and persistence feature to OH2 #4891

Uh oh!

Conversation

mhilbush commented Dec 12, 2016

Uh oh!

watou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhilbush commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watou commented Dec 13, 2016

Uh oh!

mhilbush commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhilbush commented Dec 14, 2016

Uh oh!

watou commented Dec 15, 2016

Uh oh!

mhilbush commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watou commented Dec 15, 2016

Uh oh!

mhilbush commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhilbush commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

watou commented Dec 15, 2016

Uh oh!

mhilbush commented Dec 15, 2016

Uh oh!

mhilbush commented Dec 15, 2016

Uh oh!

watou commented Dec 15, 2016

Uh oh!

mhilbush commented Dec 15, 2016

Uh oh!

mhilbush commented Dec 15, 2016 •

edited

Loading

mhilbush commented Dec 15, 2016 •

edited

Loading