Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): incorrect state tracking #291

Closed
wants to merge 1 commit into from

Conversation

maxileith
Copy link
Contributor

@maxileith maxileith commented Dec 28, 2023

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

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • What is the current behavior? (You can also link to an open issue here)
  • What is the new behavior (if this is a feature change)?
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  • Other information:

Pull Request Checklist

  • My code follows the code style of this project
    • Run npm run lint to double check
  • My change requires a change to the documentation
    • I have updated the documentation accordingly
  • I have added tests to cover my changes
    • Run npm run test to run the unit tests and npm run coverage to generate a coverage report
  • All new and existing tests passed
  • My commit messages follow the commit guidelines

@sebbo2002
Copy link
Owner

I assume that's the same issue as reported in sebbo2002/pyatv-mqtt-bridge#285. I assume the output you posted above is complete? I would like to solve this as in sebbo2002/pyatv-mqtt-bridge#285 (comment) 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.

@sebbo2002 sebbo2002 self-assigned this Dec 28, 2023
@sebbo2002 sebbo2002 added the bug Something isn't working label Dec 28, 2023
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. 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.
@maxileith
Copy link
Contributor Author

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
Copy link
Owner

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

@maxileith
Copy link
Contributor Author

maxileith commented Dec 28, 2023

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

@maxileith
Copy link
Contributor Author

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
Copy link
Owner

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

@sebbo2002 sebbo2002 closed this in 48010f3 Dec 29, 2023
@sebbo2002
Copy link
Owner

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

The release is available on:

Your semantic-release bot 📦🚀

@sebbo2002
Copy link
Owner

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

@sebbo2002 sebbo2002 mentioned this pull request Dec 29, 2023
@maxileith
Copy link
Contributor Author

It works. Thanks for the fast fix!

github-actions bot pushed a commit that referenced this pull request Dec 29, 2023
## [7.2.1](v7.2.0...v7.2.1) (2023-12-29)

### Bug Fixes

* **DeviceEvents:** Handle power_state/focus_state/volume events properly ([48010f3](48010f3)), closes [/github.com/sebbo2002/pyatv-mqtt-bridge/issues/285#issuecomment-1797978006](https://github.com//github.com/sebbo2002/pyatv-mqtt-bridge/issues/285/issues/issuecomment-1797978006) [#291](#291)
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 7.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants