-
Notifications
You must be signed in to change notification settings - Fork 84
We can now specify service name #17
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
Conversation
|
Added 2 more things to the PR.
|
|
Found a NPE after an accessory was removed from the bridge. This was a silent error since iOS apps are still removing the accessor from the UI.
This PR includes a fix for this. Logs now print something like this instead of the NPE:
|
| * @param accessory HomeKit accessory exposed as a service. | ||
| * @param serviceName name of the service. This information is usually the name of the accessory. | ||
| */ | ||
| public AbstractServiceImpl(String type, HomekitAccessory accessory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave the old constructor. Not necessarily for API compatibility, but because I think most use cases would probably just use the accessory name. The old constructor can just call the new one with accessory.getLabel()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which, granted, wouldn't be called in any of the framework provided services - what you did with the constructors there makes sense. But still, people implementing their own services would likely find it convenient.
|
@gdombiak - awesome work! One small change and then I'll merge. |
|
Hey Andy, I added back the deprecated constructor. Things should work fine even when using that constructor. Let me know if you see anything else. Thanks, |
|
Thanks for another great contribution! |
Andy,
Here are 2 small improvements:
I have an accessory that offers 2 services and both services were appearing with the same name (the name of the accessory). I added a new constructor to ServicesImpl so that we can pass a serviceName. Old constructor is still around and defaults to accessory label.
Fine-prints: 1) Had to remove the deprecated constructor in AbstractServiceImpl. I think you were ok with this. 2) Changed constructor of Name to accept a String instead of a HomekitAccessory.
Noticed a few more "noisy" entries in the log around IOExceptions so went ahead and change code to log with DEBUG all IOExceptions instead of just "Connection timed out".
Let me know if you are ok with these changes.
Thanks,
Gaston