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

mobile_app_notification_cleared for wearos #4035

Merged

Conversation

JosephAbbey
Copy link
Contributor

@JosephAbbey JosephAbbey commented Nov 28, 2023

Summary

Moved the NotificationDeleteRecieve to common and added it to the WearOS app.

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#1009

Any other notes

I have not been able to run the WearOS app at all on the simulator (even without any of my changes) so I haven't been able to test it but it compiles.

@jpelgrom jpelgrom linked an issue Nov 28, 2023 that may be closed by this pull request
@dshokouhi
Copy link
Member

dshokouhi commented Nov 28, 2023

I think for this PR we also need to make a manifest change to add the receiver to the`wear manifest like it is added for the main app manifest

what issue are you running into with the emulator?

edit: this PR will also need a docs update

@JosephAbbey
Copy link
Contributor Author

Ok, I have added the receiver to the manifest.

The entire simulator just would crash as soon as the app started loading, I wasn't too worried because I have only just installed the wearos virtual device and I hoped someone else could test this change.

@dshokouhi
Copy link
Member

have you also setup the notification service per the readme instructions from this repo? If you have then you dont really need to open the app, if you can login to the app it will register the device and create a service call....if you were able to login but cant keep the app open thats a known issue actually. For notifications the app just needs to be logged in to receive and send events but that all depends if you setup the firebase stuff otherwise notifications will not work in debug builds on the watch.

@JosephAbbey
Copy link
Contributor Author

I can't get logs because the whole simulator crashes and the logcat logs disappear to quickly. I haven't setup the notification service yet but the app won't even start anyway.

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the changes locally and it works as expected, the cleared event gets sent. Just need to move the method to common so the code can be completely shared. We also need to update the Wear OS companion docs to mention support is added in the beta version of the app.

@home-assistant home-assistant bot marked this pull request as draft December 1, 2023 20:09
@home-assistant
Copy link

home-assistant bot commented Dec 1, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@JosephAbbey JosephAbbey marked this pull request as ready for review December 1, 2023 20:49
Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and tested it on my watch locally. Still needs docs update before we can merge. Please submit a PR to add a mention in the Wear OS docs.

We need to update this page: https://companion.home-assistant.io/docs/wear-os/#notifications

@JBassett JBassett merged commit 6b56bc5 into home-assistant:master Dec 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WearOS: Add support for notification cleared event
4 participants