i8beef / node-red-contrib-castv2

MIT License
22 stars 14 forks source link

Update castv2-sender.js #10

Closed jparthum closed 4 years ago

jparthum commented 4 years ago

I got to playing around and made some changes you may want to review:

Hope this is helpful.

i8beef commented 4 years ago

So I incorporated the index change in the latest version... Im not sure on the rest. I think it has merit for sure, but I'm not that familiar with the cast API metadata rules, so before I go merging this I wanna read up on it a bit and see if I should do something like only including the RIGHT metadata elements when they apply (i.e., by that metadataType) instead of always supplying but leaving null, etc. Ill try and spend some time reading this weekend on it.

jparthum commented 4 years ago

I started to make two different PRs -- and in hindsight I probably should have. I'm not very familiar with PRs (or any other aspect of code publishing for that matter), so I wasn't sure what would be easier for you and/or better facilitate testing.

I considered including only those metadata elements applicable to the current stream but foresaw a couple of potential issues. Either:

Or:

I tried the structure as submitted without much confidence that it would actually work, but so far it's been fine in my limited testing. I've been casting daily without issue, but only populating MusicTrackMediaMetadata.
Over the weekend I'll also try casting some Movies and TV shows and report back...

i8beef commented 4 years ago

So if the intention is to expose all of these options. Instead, I have put in an option to allow you to pass your own metadata object that will OVERRIDE the default when needed. This means I've deprecated the 'title' and 'image' properties entirely, in favor of just letting you pass them in the metadata override. As this brings in all the changes you intended here, I'm closing this in favor of the version I just pushed, 2.0.1. See if that works for you, and if not, please open a ticket.

jparthum commented 4 years ago

That's a great idea. I think the number of use-cases for this additional metadata are fairly small right now -- particularly beyond audio casts. Setting metadata.metadataType to anything except 0 effectively invokes 'advanced mode', allowing additional (IE: 'advanced') metadata entries that conform directly to Google's Media namespace data structures... that's brilliant!