[Kodi] Added dynamic state descriptions for opening pvr stream channels #3207

cweitkamp · 2018-02-04T11:30:32Z

Added dynamic state descriptions for opening pvr stream channels
Added REQUEST_CACHE
Added model for Kodi specific entities (e.g. KodiPVRChannelGroup, KodiPVRChannel, KodiBaseItem, etc.)
Adapted README.md

Also-by: Gregory Moyer [email protected]
Signed-off-by: Christoph Weitkamp [email protected]

Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp · 2018-02-04T11:31:46Z

@mhilbush You can return the favor - if you like. ;-)

mhilbush

Just a few minor comments. Sorry for the delay in getting to this.

mhilbush · 2018-02-05T21:28:30Z

...ab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/protocol/KodiConnection.java

-        JsonElement response = socket.callMethod("PVR.GetChannelGroups", params);
+        String method = "PVR.GetChannelGroups";
+        String hash = method + "#channeltype=" + channelType;
+        if (!REQUEST_CACHE.containsKey(hash)) {


Could you use REQUEST_CACHE.putIfAbsentAndGet here?
https://github.com/eclipse/smarthome/blob/4204ce06bb28c28e5f711e720f87ef83beff2e27/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/cache/ExpiringCacheMap.java#L116

mhilbush · 2018-02-05T21:28:40Z

...ab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/protocol/KodiConnection.java

-        JsonElement response = socket.callMethod("PVR.GetChannels", params);
+        String method = "PVR.GetChannels";
+        String hash = method + "#channelgroupid=" + channelGroupID;
+        if (!REQUEST_CACHE.containsKey(hash)) {


Same comment as above.

mhilbush · 2018-02-05T21:36:59Z

...ing/org.openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/handler/KodiHandler.java

-                    KodiChannelConfig config = getThing().getChannel(channelUID.getId()).getConfiguration()
-                            .as(KodiChannelConfig.class);
-                    playPVRChannel(command, "tv", config);
+                    playPVRChannel(command, "tv", CHANNEL_PVR_OPEN_TV);


I realize it was like this already, but should tv and radio be defined as constants, as they seem to be used elsewhere in the code?

mhilbush · 2018-02-05T21:37:14Z

...ing/org.openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/handler/KodiHandler.java

-                    KodiChannelConfig config = getThing().getChannel(channelUID.getId()).getConfiguration()
-                            .as(KodiChannelConfig.class);
-                    playPVRChannel(command, "radio", config);
+                    playPVRChannel(command, "radio", CHANNEL_PVR_OPEN_RADIO);


See above comment.

Signed-off-by: Christoph Weitkamp <[email protected]>

…com:cweitkamp/openhab2-addons into feature-kodi-pvr-dynamic-state-descriptions

Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp · 2018-02-08T10:29:20Z

@mhilbush Thanks for your first review. I finished my work. Can you check it again?

mhilbush

@cweitkamp Just a few minor comments. Otherwise, LGTM.

mhilbush · 2018-02-08T13:24:21Z

...ab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/protocol/KodiConnection.java

        if (response instanceof JsonObject) {
            JsonObject result = response.getAsJsonObject();
-            if (result.has("channelgroups")) {
-                return result.get("channelgroups").getAsJsonArray();
+            for (JsonElement element : result.get("channelgroups").getAsJsonArray()) {


The old code checks to see if the member channelgroups exists in the response. Not being familiar with the protocol, was that check unnecessary? If it's not there, won't this line generate a NPE?

In this case it is not crucial. There is always one default channel group of each channel type.

mhilbush · 2018-02-08T13:29:42Z

...ab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/protocol/KodiConnection.java

        if (response instanceof JsonObject) {
            JsonObject result = response.getAsJsonObject();
-            if (result.has("channels")) {
-                return result.get("channels").getAsJsonArray();
+            for (JsonElement element : result.get("channels").getAsJsonArray()) {


Same comment as above concerning the assumption that the response always will contain the member channels.

In this case it is crucial. The field is missing if a channel group doesn't contain any channels. Thanks for the hint. I changed it in both cases.

mhilbush · 2018-02-08T13:39:17Z

...enhab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/model/KodiPVRChannel.java

+    /**
+     * The PVR channel id
+     */
+    private int channelId;


Very minor, but noting slight inconsistency in naming. Here it is channelId and getId. Elsewhere it's channelID and getChannelID. I might not even bring this up, except that elsewhere in the code channelId refers to the ESH channelId.

That's true. I think about a renaming.

I thought about it and feel that my naming is OK in thus place. But I changed it in the rest of the binding to have a clear line when we talk about an ESH channel or a TV channel.

mhilbush · 2018-02-08T13:41:59Z

....binding.kodi/src/main/java/org/openhab/binding/kodi/internal/model/KodiPVRChannelGroup.java

+        return channelGroupId;
+    }
+
+    public void setId(final int channelGroupId) {


Also channelGroupId is referred to as channelGroupID elsewhere in the code (e.g. the method getChannelGroupID).

mhilbush · 2018-02-08T13:51:10Z

addons/binding/org.openhab.binding.kodi/README.md

-        Selection item=myKodi_pvropentv mappings=[Add your PVR TV channels here ...]
-        Selection item=myKodi_pvropenchannel mappings=[Add your PVR radio channels here ...]
+        Selection item=myKodi_pvropentv
+        Selection item=myKodi_pvropenchannel


Since this depends on this feature being implemented in ESH, is the intention to delay the merge of this until that feature is part of a future ESH stable?

Also, WDYT about a HABpanel Selection widget example, too?

Yes and No. The example was bad before. But since it depends on the users configuration of Kodi I can't think of a better one.

WDYT about a HABpanel Selection widget example

Hm, ... I wouldn't support that. IMHO the documentation should be independent and not related to any UI. Shouldn't it?

Ideally, yes, the documentation should be independent of the UI. OTOH, the downside is that it forces the user to figure it out on their own, or to hunt on the forum for someone who's already done it.

My motive for asking was purely selfish, as I was struggling with this exact question for the squeezebox favorites. ;-)

Signed-off-by: Christoph Weitkamp <[email protected]>

…aning of PVR and ESH channel Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp · 2018-02-22T17:06:21Z

@martinvw may I ask you to have a look at this PR? It's already reviewed. Thank you.

martinvw

Only some nitpicking 😄

martinvw · 2018-02-22T19:16:38Z

...ing/org.openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/handler/KodiHandler.java

@@ -60,9 +64,13 @@

    private ScheduledFuture<?> statusUpdaterFuture;

-    public KodiHandler(@NonNull Thing thing) {
+    private KodiDynamicStateDescriptionProvider stateDescriptionProvider;


martinvw · 2018-02-22T19:20:46Z

...openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/model/KodiBaseItem.java

+    /**
+     * The label of the item
+     */
+    protected String label;


Why protected can it be private

martinvw · 2018-02-22T19:22:20Z

...ab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/protocol/KodiConnection.java

@@ -50,6 +51,8 @@
            .asList(new Integer[] { -32, -16, -8, -4, -2, 1, 2, 4, 8, 16, 32 });
    private static final ExpiringCacheMap<String, RawType> IMAGE_CACHE = new ExpiringCacheMap<>(
            TimeUnit.MINUTES.toMillis(15)); // 15min
+    private static final ExpiringCacheMap<String, JsonElement> REQUEST_CACHE = new ExpiringCacheMap<>(
+            TimeUnit.MINUTES.toMillis(5)); // 5min


Is the comment still needed when using the TimeUnit?

martinvw · 2018-02-22T19:23:44Z

...ab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/protocol/KodiConnection.java

-                }
+    public int getPVRChannelId(final int pvrChannelGroupId, final String pvrChannelName) {
+        List<KodiPVRChannel> pvrChannels = getPVRChannels(pvrChannelGroupId);
+        for (KodiPVRChannel pvrChannel : pvrChannels) {


I think that I would not assign this first to a var

What do you mean exactly?
for (KodiPVRChannel pvrChannel : getPVRChannels(pvrChannelGroupId)) { instead?

Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp · 2018-02-23T14:07:16Z

Thank you all.

martinvw · 2018-03-06T09:58:28Z

addons/binding/org.openhab.binding.kodi/META-INF/MANIFEST.MF

@@ -37,5 +37,6 @@ Import-Package:
 org.openhab.binding.kodi.handler,
 org.osgi.framework,
 org.osgi.service.component,
+ org.osgi.service.component.annotations,


FTR: this should not be done like this, see also #3292 & #3294

…ls (openhab#3207) * Changed README.md * Added dynamic state descriptions for opening pvr stream channels * Added model for Kodi specific entities * Added NPE safeguard Signed-off-by: Christoph Weitkamp <[email protected]>

Added dynamic state descriptions for opening pvr stream channels

1259a40

Signed-off-by: Christoph Weitkamp <[email protected]>

mhilbush reviewed Feb 5, 2018

View reviewed changes

cweitkamp mentioned this pull request Feb 6, 2018

[Squeezebox] Add LMS favorites channels to server and player #3190

Merged

cweitkamp added 4 commits February 6, 2018 16:45

Incorporate changes from review

4e658c0

Signed-off-by: Christoph Weitkamp <[email protected]>

Added model for Kodi specific entities

f1cf030

Signed-off-by: Christoph Weitkamp <[email protected]>

Merge branch 'feature-kodi-pvr-dynamic-state-descriptions' of github.…

cddc17f

…com:cweitkamp/openhab2-addons into feature-kodi-pvr-dynamic-state-descriptions

Added model for Kodi specific entities

9b49743

Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp force-pushed the feature-kodi-pvr-dynamic-state-descriptions branch from 5a2756d to 9b49743 Compare February 8, 2018 09:38

Changed README.md

da43a3c

Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp changed the title ~~[WIP][Kodi] Added dynamic state descriptions for opening pvr stream channels~~ [Kodi] Added dynamic state descriptions for opening pvr stream channels Feb 8, 2018

mhilbush reviewed Feb 8, 2018

View reviewed changes

cweitkamp added 2 commits February 8, 2018 20:16

Added NPE safeguard

d2d64a7

Signed-off-by: Christoph Weitkamp <[email protected]>

Renamed 'channel*' to 'pvrChannel*' to distinguish clearly between me…

4a93828

…aning of PVR and ESH channel Signed-off-by: Christoph Weitkamp <[email protected]>

martinvw approved these changes Feb 22, 2018

View reviewed changes

cweitkamp added 2 commits February 23, 2018 07:48

Incorporated changes from review

fd61b7a

Signed-off-by: Christoph Weitkamp <[email protected]>

Incorporated changes from review

a435f61

Signed-off-by: Christoph Weitkamp <[email protected]>

martinvw approved these changes Feb 23, 2018

View reviewed changes

Reduced log level

c2f2efc

Signed-off-by: Christoph Weitkamp <[email protected]>

martinvw merged commit 4a25680 into openhab:master Feb 23, 2018

cweitkamp deleted the feature-kodi-pvr-dynamic-state-descriptions branch February 23, 2018 13:42

martinvw reviewed Mar 6, 2018

View reviewed changes

martinvw added this to the 2.3 milestone May 25, 2018

martinvw added the enhancement An enhancement or new feature for an existing add-on label May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kodi] Added dynamic state descriptions for opening pvr stream channels #3207

[Kodi] Added dynamic state descriptions for opening pvr stream channels #3207

cweitkamp commented Feb 4, 2018 •

edited

Loading

cweitkamp commented Feb 4, 2018

mhilbush left a comment

mhilbush Feb 5, 2018

mhilbush Feb 5, 2018

mhilbush Feb 5, 2018

mhilbush Feb 5, 2018

cweitkamp commented Feb 8, 2018

mhilbush left a comment

mhilbush Feb 8, 2018

cweitkamp Feb 8, 2018

mhilbush Feb 8, 2018

cweitkamp Feb 8, 2018

mhilbush Feb 8, 2018

cweitkamp Feb 8, 2018

cweitkamp Feb 22, 2018

mhilbush Feb 8, 2018

cweitkamp Feb 22, 2018

mhilbush Feb 8, 2018

cweitkamp Feb 8, 2018

mhilbush Feb 8, 2018

cweitkamp commented Feb 22, 2018

martinvw left a comment

martinvw Feb 22, 2018

martinvw Feb 22, 2018

martinvw Feb 22, 2018

martinvw Feb 22, 2018 •

edited

Loading

cweitkamp Feb 23, 2018

martinvw Feb 23, 2018

cweitkamp commented Feb 23, 2018

martinvw Mar 6, 2018

[Kodi] Added dynamic state descriptions for opening pvr stream channels #3207

[Kodi] Added dynamic state descriptions for opening pvr stream channels #3207

Conversation

cweitkamp commented Feb 4, 2018 • edited Loading

cweitkamp commented Feb 4, 2018

mhilbush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp commented Feb 8, 2018

mhilbush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp commented Feb 22, 2018

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp commented Feb 23, 2018

Choose a reason for hiding this comment

cweitkamp commented Feb 4, 2018 •

edited

Loading

martinvw Feb 22, 2018 •

edited

Loading