sebbo2002 / node-pyatv

A lightweight node.js wrapper around pyatv…
MIT License
27 stars 2 forks source link

fix(core): incorrect state tracking #291

Closed maxileith closed 11 months ago

maxileith commented 11 months ago

Hi,

I think I found a major bug in this otherwise awesome library.

The current state of a device was not taken into consideration when generating the updated state after receiving an update from atvscript. This way, attributes that are not reported in that specific result of atvscript, default to null, which is unwanted since in reality they are still unchanged and not null.

E.g. when changing the volume, all attributes beside the volume are set to null, since the volume attribute is the only one reported by atvscript when changing the volume:

{
    "result": "success",
    "datetime": "2023-12-28T22:19:30.570684+01:00",
    "volume": 50.0
}

All attributes beside volume will default to null resulting in a wrong state and unnecessary event listener triggers:

https://github.com/sebbo2002/node-pyatv/blob/65cbc62b8e65e85c6f82a0474d4dfb91c6663eeb/src/lib/tools.ts#L239C4-L239C4

About this Pull Request

Pull Request Checklist

sebbo2002 commented 11 months ago

I assume that's the same issue as reported in https://github.com/sebbo2002/pyatv-mqtt-bridge/issues/285. I assume the output you posted above is complete? I would like to solve this as in https://github.com/sebbo2002/pyatv-mqtt-bridge/issues/285#issuecomment-1797978006 and not based on the current state. This should prevent broken states from being created. Thanks for posting such an event, I wasn't able to reproduce this, please give me some days to fix this.

maxileith commented 11 months ago

Hi @sebbo2002,

thanks for the fast reply. The output that I have posted above is a complete JSON response from atvscript. Looking forward to your solution to that problem :)

sebbo2002 commented 11 months ago

No problem. Another question: Is the volume field included in atvscript playing? Or does it only appear when changing the volume?

maxileith commented 11 months ago

volume does not appear in atvscript playing. It does only appear when changing the volume.

maxileith commented 11 months ago

Though, I cannot speak for HDMI CEC because my HiFi setup is a bit oldschool. For testing, I selected a HomePod Mini as the default speaker. But I suppose reporting the volume will be the same for HDMI CEC devices.

sebbo2002 commented 11 months ago

Yes, I would just assume that too. Thank you, I'll let you know when I have something.

sebbo2002 commented 11 months ago

:tada: This issue has been resolved in version 7.2.1-develop.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

sebbo2002 commented 11 months ago

@maxileith Hey, can you check if 7.2.1-develop.1 works for you? Thank you.

maxileith commented 11 months ago

It works. Thanks for the fast fix!

sebbo2002 commented 11 months ago

:tada: This issue has been resolved in version 7.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: