lucasvdh / com.yamaha.receiver

Yamaha receiver app for Homey
GNU General Public License v3.0
2 stars 5 forks source link

Correct attribute names to work with RX-AS710D #11

Closed axelwathne closed 4 years ago

axelwathne commented 4 years ago

Hi Lucas

I didn't make a PR at first because I was unsure of the reason for the differences. The don't look like just bugs, they look more like changes to a protocol version. But I can tell you they are the correct attributes for the 710. Please have a look at it and understand that it might break support for other devices. I have written no tests for this, as you can see.

-Axel

lucasvdh commented 4 years ago

Awesome, I think we'd need to change this so that it can work with both protocol versions.

lucasvdh commented 4 years ago

I'll take a look at adding some more validation for fetching data from the xml result tonight because the fetching of all the other data kinda needs the same refactor.

lucasvdh commented 4 years ago

I think the best course of action would be to create transformers for each source.

For example:

"use strict"

class AirPlayPlayInfoTransformer {
    transform(xmlResponse) {
        return xml2js.parseStringPromise(xmlResponse)
            .then(result => {
                let playInfoResult = result['YAMAHA_AV']['AirPlay'][0]['Play_Info'][0],
                    availabilityResult = playInfoResult['Feature_Availability'][0],
                    playbackInfoResult = playInfoResult['Playback_Info'][0],
                    metaInfoResult = playInfoResult['Meta_Info'][0];

                playInfo.available = availabilityResult === 'Ready';
                playInfo.playing = playbackInfoResult === 'Play';
                playInfo.artist = Entities.decode(metaInfoResult['Artist'][0]);
                playInfo.album = Entities.decode(metaInfoResult['Album'][0]);
                playInfo.track = Entities.decode(metaInfoResult['Song'][0]);

                return playInfo;
            });
    }
}

module.exports = AirPlayPlayInfoTransformer;
lucasvdh commented 4 years ago

The current state is how it works for the HTR-4067.

I think the state can also use a refactor to a transformer. This would provide better separation of concerns and would make it easier to add more edge cases to the transformer when they are identified. I'm curious to see what you've come up with.

I was also thinking it might be easier to grant you write access to this repo so we can more easily work together without having to resolve the merge conflicts all the time.

axelwathne commented 4 years ago

I see you added another way to handle the differences. Would you rather use that way than the transformer way? I pushed the transformers, as you can see they lead to some code duplication, which should be extracted again. Maybe what you did in bugfix/rx-as710d is more pragmatic after all.

lucasvdh commented 4 years ago

@axelwathne I have some time again this weekend, I will try and merge changes from both branches. I might create a new PR.

lucasvdh commented 4 years ago

@axelwathne this was closed by https://github.com/lucasvdh/com.yamaha.receiver/pull/13