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

mhilbush · 2018-01-29T22:13:08Z

Signed-off-by: Mark Hilbush [email protected]
Also-by: Gregory Moyer [email protected]

Add two new channels to enable use of the favorites list on the LMS (Logitech Media Server).

Add a readonly favoritesList channel of type String to the squeezebox server thing.

the SqueezeBox Server favoritesList channel is updated immediately when the binding starts, and whenever the favorites list is updated on the LMS (there’s an asynchronous notification from the LMS to the binding when the favorites list is updated).
the channel is formatted with comma-separated values of the form favoriteId=favoriteName, like this:

1=Rock,2=Folk,3=Classic Alternative

there’s a config option that allows you to wrap the favoriteName with double quotes

1="Rock",2="Folk",3="Classic Alternative"

only favorites at the root level of the LMS favorites hierarchy are returned
the server handler sends the favorites list to the player handler

Add a playFavorite channel of type String to the squeezebox player thing.

when you send the favoriteId to an item linked to a player's playFavorite channel, it plays the favorite on that player.
the player uses the favorites list from the server handler to update the playFavorite channel's state description to include the favorites as options.

Dependencies:

Sitemap needs to be updated to be able to use the item's state options as the selection list whenever the state options change
HABpanel needs to be able to update the choices in the Selection widget whenever the state options change

Signed-off-by: Mark Hilbush <[email protected]>

tavalin · 2018-01-30T12:09:50Z

Instead of having this channel as a string of comma-separated values, wouldn't it be better for the list (received from the server) to dynamically update the state options for the channel?

mhilbush · 2018-01-30T12:50:41Z

Yes, I'm actually working on that right now. :-) Passing the favorites list to the players, who will then update the state options on their favoritesPlay channel. Just learning how to do that...

I still may keep the server channel the way it is, since someone might find it handy to process that in a rule.

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush · 2018-01-30T13:13:51Z

BTW @tavalin , would you be able to point me to a good example of how to update the StateDescription during runtime? ;-)

tavalin · 2018-01-30T13:27:35Z

The HarmonyHub binding updates the state options during runtime. Have a look here https://github.com/openhab/openhab2-addons/blob/master/addons/binding/org.openhab.binding.harmonyhub/src/main/java/org/openhab/binding/harmonyhub/handler/HarmonyDeviceHandler.java#L160

cweitkamp · 2018-01-30T13:30:29Z

I added some details to eclipse-archived/smarthome#4942. But lets continue our discussion about the best practice how to implement that feature here.

imho the best way to solve this is to implement a DynamicStateDescriptionProvider for the binding.

Have a look here for an example: https://github.com/openhab/openhab2-addons/pull/2345/files#diff-005ee22fbafd96dfe88650528963939f

mhilbush · 2018-01-30T17:14:02Z

@cweitkamp That's a great example, and it seems to work very well. Much simpler than everything else I looked at. Thanks!

It doesn't look like HABpanel updates the options dynamically during runtime. I posed a question about that on the habpanel repo.

In fact, it looks a refresh of the Control page is needed in PaperUI to refresh the options. Is that you're understanding of how it works, or how it's supposed to work?

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush · 2018-01-31T13:56:08Z

@cweitkamp Your review of this would be appreciated. I followed your recommendation to use DynamicStateDescriptionProvider for managing the update to the channel's state options. Thank you for that!

cweitkamp · 2018-01-31T18:31:53Z

You are welcome. I'm going to review your PR in the next few days. Thanks for asking.

it looks a refresh of the Control page is needed in PaperUI to refresh the options. Is that you're understanding of how it works, or how it's supposed to work?

To be honest, I can't tell you. I suppose such an event doesn't exist. Paper UI has no chance to listen for a dynamically updated options list. Could be true for other UIs as well.

cweitkamp

Nice work. I added some thoughts.

cweitkamp · 2018-01-31T18:05:11Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxPlayerHandler.java

+            options.add(new StateOption(favorite.shortId, favorite.name));
+        }
+        stateDescriptionProvider.setStateOptions(
+                new ChannelUID(getThing().getUID(), SqueezeBoxBindingConstants.CHANNEL_FAVORITES_PLAY), options);


Remove SqueezeBoxBindingConstants..

cweitkamp · 2018-01-31T18:09:30Z

addons/binding/org.openhab.binding.squeezebox/ESH-INF/thing/thing-types.xml

+		<description>Comma-separated list of favorites of form favoriteId=favoriteName</description>
+		<state readOnly="true" pattern="%s"></state>
+	</channel-type>
+	<channel-type id="favoritesPlay">


I was asking myself if playFavorite sounds better. Wdyt?

cweitkamp · 2018-01-31T18:09:54Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxPlayerHandler.java

@@ -39,10 +41,13 @@
 import org.eclipse.smarthome.core.types.Command;
 import org.eclipse.smarthome.core.types.RefreshType;
 import org.eclipse.smarthome.core.types.State;
+import org.eclipse.smarthome.core.types.StateOption;
 import org.eclipse.smarthome.core.types.UnDefType;
 import org.eclipse.smarthome.io.net.http.HttpUtil;
 import org.openhab.binding.squeezebox.SqueezeBoxBindingConstants;


Is this import necessary?

Yes. It's used in updateFavoritesListEvent to create the arraylist for setStateOptions.

You removed the reference in one of your last commits. And as you already imported it static it's redundant.

Yeah, there was another reference that I missed. I'll fix it.

cweitkamp · 2018-01-31T18:16:12Z

.../java/org/openhab/binding/squeezebox/internal/SqueezeBoxStateDescriptionOptionsProvider.java

+/**
+ * Dynamic provider of state options while leaving other state description fields as original.
+ *
+ * @author Gregory Moyer - Initial contribution


If you do it like this you should add the Also-by tag to the description or your PR.

I can do that. I wasn't sure the best way to handle this. Is there another way that's more preferred?

cweitkamp · 2018-01-31T18:16:49Z

.../java/org/openhab/binding/squeezebox/internal/SqueezeBoxStateDescriptionOptionsProvider.java

+    public void deactivate() {
+        channelOptionsMap.clear();
+    }
+}


Missing newline.

cweitkamp · 2018-01-31T18:18:39Z

...binding.squeezebox/src/main/java/org/openhab/binding/squeezebox/internal/model/Favorite.java

+ *
+ */
+public class Favorite {
+    // id is of form xxxxxxxx.nn


Can you please use JavaDoc here?

cweitkamp · 2018-01-31T18:20:02Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+
+        private void updateChannelFavoritesList(List<Favorite> favorites) {
+            // Get config parameter indicating whether name should be wrapped with double quotes
+            final boolean includeQuotes = getConfigAs(SqueezeBoxServerConfig.class).quoteFavoritesList;


Why do you need those quotes?

Was looking to provide some flexibility, depending on where/how someone was going to use this list (presumably in a rule).

1=Rock,2=Folk,3=Classic Alternative

Or

1="Rock",2="Folk",3="Classic Alternative"

Alright, just a guess: Isn't the quoteFavoritesList configuration more a channel configuration than a thing configuration? Is it? But I don't want you to blow up this PR with it.

Well, yeah, I actually thought about that after I implemented this way. It probably should be a channel configuration, huh?

cweitkamp · 2018-01-31T18:20:50Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+                return;
+            }
+
+            List<Favorite> favorites = new ArrayList<Favorite>();


Diamond operator -> new ArrayList<>().

cweitkamp · 2018-02-01T19:16:07Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+            for (Favorite favorite : favorites) {
+                // If not quoting, we don't want any embedded commas
+                adjustedName = includeQuotes ? favorite.name : favorite.name.replaceAll(",", "");
+                sb.append(favorite.shortId + "=");


Avoid concatenation of strings, if you use a StringBuilder. Use the append method all the time.

cweitkamp · 2018-02-01T19:28:55Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+
+            // If the channel doesn't exist we can't update it
+            if (getThing().getChannel(CHANNEL_FAVORITES_LIST) == null) {
+                logger.debug("Channel '{}' does not exist.  Delete and readd player thing to pick up channel.",


Do I see double whitespaces between the sentences?

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush · 2018-02-01T21:12:09Z

@cweitkamp Thanks for the feedback; very much appreciated.

I believe I addressed all your feedback except for where I added comments/questions above.

That double-space between sentences thing... Showing my age. :-(

cweitkamp · 2018-02-02T10:27:28Z

You're welcome.

That double-space between sentences thing... Showing my age. :-(

Don't bother. You are doing a good job and I really enjoy working together with you. I thought twice about commenting that line or not. ;-)

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush · 2018-02-02T11:35:04Z

@martinvw This is ready for final review. Thanks.

cweitkamp

LGTM

mhilbush · 2018-02-02T17:21:01Z

@cweitkamp So what do you think about moving the config to the channel? If that's the more correct approach, then it would be better to do now. WDYT?

cweitkamp · 2018-02-02T18:52:42Z

I'm pretty sure you can foresee my answer...

mhilbush · 2018-02-02T18:59:02Z

Yeah, it was somewhat of a rhetorical question. LOL

It's already done, so I just need to push it. :-)

Signed-off-by: Mark Hilbush <[email protected]>

cweitkamp

That was quick. Some minor remarks.

cweitkamp · 2018-02-03T08:57:44Z

...box/src/main/java/org/openhab/binding/squeezebox/internal/config/SqueezeBoxServerConfig.java

+    /*
+     * When true, wrap the name of the favorite in quotes
+     */
+    public boolean quoteFavoritesList;


Want to remove this property?

cweitkamp · 2018-02-03T08:58:42Z

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

 {
    Thing squeezeboxplayer myplayer[ mac="00:f1:bb:00:00:f1" ]
 }
 ```

-## Channels
+## Server Channels


Touch documentation.

Yep. Missed that.

cweitkamp · 2018-02-03T09:00:25Z

addons/binding/org.openhab.binding.squeezebox/ESH-INF/thing/thing-types.xml

+		<description>Comma-separated list of favorites of form favoriteId=favoriteName</description>
+		<state readOnly="true" pattern="%s"></state>
+		<config-description>
+			<parameter name="quoteFavoritesList" type="boolean" required="true">


What about renaming it to quoteList? For the sake of reusability.

cweitkamp · 2018-02-03T09:06:42Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+        }
+
+        private void updateChannelFavoritesList(List<Favorite> favorites) {
+            final Channel channel = thing.getChannel(CHANNEL_FAVORITES_LIST);


getThing()?

Ok. But why does BaseThingHandler expose both?

I have to admit. This was nitpicking. Sry for that.

Signed-off-by: Mark Hilbush <[email protected]>

cweitkamp

Thank you very much. LGTM.

mhilbush · 2018-02-03T13:29:59Z

Thanks for the help @cweitkamp!

@martinvw Ready for final review

martinvw

Thanks guys, I added some additional comments

martinvw · 2018-02-05T20:35:53Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+        logger.trace("Updating channel {} for {} to state {}", channelID, getThing().getUID(), state);
+        try {
+            updateState(channelID, state);
+        } catch (IllegalStateException e) {


In what case will this happen of did I already ask that?

The API spec for BaseThingHandler says if handler is not initialized correctly, because no callback is present

And, I chose to not do this. ;-)
https://github.com/openhab/openhab2-addons/blob/master/addons/binding/org.openhab.binding.squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxPlayerHandler.java#L471

Hmm, I'm not happy with this, @SJKA @triller-telekom do you have any useful suggestions here.

These lines are in the BaseThingHandler since 17.03.2015. So this Exception was thrown ever since then. Since it is a RuntimeException it does not have to be caught at compile time. And since all the other bindings do not catch it at compile time I think it's alright to let it pass through to the code above.

Ok. I'll remove it from here and SqueezeBoxPlayerHandler.

Did you do that already?

Duh. It would help if I actually pushed the commit.

martinvw · 2018-02-05T20:38:35Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+                includeQuotes = (Boolean) channel.getConfiguration().get(CHANNEL_CONFIG_QUOTE_LIST);
+            }
+
+            final String quote = includeQuotes.booleanValue() ? "\"" : "";


I prefer to leave away the final here since they are not consistently used (in this binding) anyways.

martinvw · 2018-02-05T20:40:33Z

.../java/org/openhab/binding/squeezebox/internal/SqueezeBoxStateDescriptionOptionsProvider.java

+    }
+
+    @Override
+    public @Nullable StateDescription getStateDescription(@NonNull Channel channel, @Nullable StateDescription original,


CC @triller-telekom if I remember correctly we should not be using both of these in this case it should

See also https://www.eclipse.org/smarthome/documentation/development/guidelines.html#a-code-style A.12

@triller-telekom Can you also weigh in on this?

I can jump in here: In general we want to avoid using @NonNull but use @NonNullByDefault on class level. You can have a look at #3207 I already changed adapted it in my PR.

@mhilbush Yes I second @cweitkamp's comment, we are only using @NonNullByDefault and @Nullable, see the link that @martinvw has posted.

martinvw · 2018-02-05T20:42:04Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+            }
+
+            List<Favorite> favorites = new ArrayList<>();
+            String title = "";


Where is this used for?

I chose to capture the favorites title and count, as they are returned in the response from the LMS, even though they currently are not used anywhere. Would you rather they be removed, or commented as currently unused?

Or you can log them inside a isTraceEnabled and not store it at all?

martinvw · 2018-02-05T20:42:21Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+
+            List<Favorite> favorites = new ArrayList<>();
+            String title = "";
+            String count = "";


Where is this used for?

See my comment above.

martinvw · 2018-02-05T20:43:25Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+                // Favorite type
+                else if (part.startsWith("type%3A")) {
+                    type = decode(part.substring("type%3A".length()));
+                    if (f != null) {


Is the order guaranteed, will we revisit this if f is null?

Guaranteed is a little strong. :-)

The order is defined in the LMS code, and is highly unlikely to change at this point. The convention in the LMS code generally is that the id is the first value returned in a response. I was simply trying to be defensive in case of a corrupted response.

The response from the LMS looks like this.

favorites = title favorite1 favorite2 ... count where favoriteN = id name type isaudio hasitems

martinvw · 2018-02-05T20:44:08Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+            String count = "";
+            Favorite f = null;
+            for (String part : messageParts) {
+                String id;


Why do you need all these fields a quick looks suggest that you either assign them immediately or discard the data, wdyt?

Same as my comment above. They are returned in the response from the LMS, even though they currently are not used.

Signed-off-by: Mark Hilbush <[email protected]>

martinvw · 2018-02-06T06:01:53Z

...squeezebox/src/main/java/org/openhab/binding/squeezebox/handler/SqueezeBoxServerHandler.java

+
+            List<Favorite> favorites = new ArrayList<>();
+            Favorite f = null;
+            for (String part : messageParts) {


I did actually not mean to remove all the fields but the big benefit is that this method now more or less fits on my screen :-)

Yes, I know. But this is actually more consistent with how the binding handles other LMS messages. ;-)

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush · 2018-02-06T12:56:47Z

@martinvw I think I've addressed your comments.

I do have a couple more questions.

Is there a more preferred way here to handle the case where the favorites list is empty?
Perhaps for another PR, but what is the guidance for which classes should be in the org.openhab.binding.squeezebox.handler package?

martinvw · 2018-02-06T13:45:20Z

Thanks all!

mhilbush · 2018-02-06T14:01:42Z

Thank you @martinvw and @cweitkamp.

staehler · 2018-02-06T20:11:07Z

@mhilbush Could you please provide an example how to use favoritList and playFavorit. I would like to use this feature :-)
Thank you very much.

mhilbush · 2018-02-06T21:17:41Z

Could you please provide an example

@staehler I added some directions here.
https://community.openhab.org/t/squeezebox-favorites/39355/12

triller-telekom · 2018-02-07T08:09:33Z

@mhilbush How about a follow-up PR where you add the instructions to the official README.md of the binding? :)

mhilbush · 2018-02-07T12:57:02Z

@triller-telekom Good idea. I'm working on a PR to replace the notification volume channel with a config parameter. I'll also add the favorites instructions to the README.

Add favoriteList/favoritesPlay channel to Server/Player

f7f1552

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush changed the title ~~[squeezebox] Add favoriteList/favoritesPlay channel to server/player~~ [squeezebox] WIP Add favoriteList/favoritesPlay channel to server/player Jan 29, 2018

Remove subfolders earlier in processing

5286818

Signed-off-by: Mark Hilbush <[email protected]>

Pass favorites list to players

23724ad

Signed-off-by: Mark Hilbush <[email protected]>

cweitkamp mentioned this pull request Jan 30, 2018

Feature request: Populate Selection elements in sitemaps automatically eclipse-archived/smarthome#4942

Closed

mhilbush added 3 commits January 30, 2018 12:18

Update state options when favorites updated

2f9aafb

Signed-off-by: Mark Hilbush <[email protected]>

Cleanup

ccd4e17

Signed-off-by: Mark Hilbush <[email protected]>

Fix formatting

ccefd39

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush changed the title ~~[squeezebox] WIP Add favoriteList/favoritesPlay channel to server/player~~ [squeezebox] Add LMS favorites channels to server and player Jan 31, 2018

cweitkamp requested changes Feb 1, 2018

View reviewed changes

mhilbush added 2 commits February 1, 2018 15:59

Incorporate review feedback.

1b7d64a

Signed-off-by: Mark Hilbush <[email protected]>

Add newline

f889f3d

Signed-off-by: Mark Hilbush <[email protected]>

mhilbush added 2 commits February 2, 2018 06:23

Remove unneeded import

8bc3d57

Signed-off-by: Mark Hilbush <[email protected]>

Fix documentation

07aa617

Signed-off-by: Mark Hilbush <[email protected]>

cweitkamp approved these changes Feb 2, 2018

View reviewed changes

Move config parameter from thing to channel.

290a8e3

Signed-off-by: Mark Hilbush <[email protected]>

cweitkamp requested changes Feb 3, 2018

View reviewed changes

Incorporate review feedback

6dd820e

Signed-off-by: Mark Hilbush <[email protected]>

cweitkamp approved these changes Feb 3, 2018

View reviewed changes

martinvw reviewed Feb 5, 2018

View reviewed changes

Incorporate review feedback

70ecca6

Signed-off-by: Mark Hilbush <[email protected]>

martinvw reviewed Feb 6, 2018

View reviewed changes

Additional rework from review

a03e904

Signed-off-by: Mark Hilbush <[email protected]>

martinvw approved these changes Feb 6, 2018

View reviewed changes

martinvw merged commit d449969 into openhab:master Feb 6, 2018

mhilbush deleted the squeezebox-favorites branch February 6, 2018 22:29

martinvw changed the title ~~[squeezebox] Add LMS favorites channels to server and player~~ [Squeezebox] Add LMS favorites channels to server and player May 25, 2018

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

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

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

Conversation

mhilbush commented Jan 29, 2018 • edited Loading

tavalin commented Jan 30, 2018

mhilbush commented Jan 30, 2018 • edited Loading

mhilbush commented Jan 30, 2018

tavalin commented Jan 30, 2018

cweitkamp commented Jan 30, 2018 • edited Loading

mhilbush commented Jan 30, 2018

mhilbush commented Jan 31, 2018

cweitkamp commented Jan 31, 2018 • edited Loading

cweitkamp 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 Feb 2, 2018 • edited Loading

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

mhilbush commented Feb 1, 2018

cweitkamp commented Feb 2, 2018

mhilbush commented Feb 2, 2018

cweitkamp left a comment

Choose a reason for hiding this comment

mhilbush commented Feb 2, 2018

cweitkamp commented Feb 2, 2018

mhilbush commented Feb 2, 2018

cweitkamp 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

cweitkamp left a comment

Choose a reason for hiding this comment

mhilbush commented Feb 3, 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

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 Feb 6, 2018 • edited Loading

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

mhilbush Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhilbush commented Jan 29, 2018 •

edited

Loading

mhilbush commented Jan 30, 2018 •

edited

Loading

cweitkamp commented Jan 30, 2018 •

edited

Loading

cweitkamp commented Jan 31, 2018 •

edited

Loading

cweitkamp Feb 2, 2018 •

edited

Loading

cweitkamp Feb 6, 2018 •

edited

Loading

mhilbush Feb 5, 2018 •

edited

Loading