adding support for multiple buttons on a buttonsDevice #68
Conversation
…ry type into account (although this was true for DefaultDevice which the test is not for). Up until now this didn't matter but now it either needs only a device or deviceId and deviceName.
accessories/base.coffee
Outdated
| else | ||
| # if the parameters were passed to the constructor (and likely to be modified) | ||
| # we will use a combination of deviceId and deviceName for generating the serial | ||
| deviceSerialId = deviceId + deviceName |
There was a problem hiding this comment.
Why not use the passed deviceId? I don't see why we need an extra deviceSerialId. deviceId should be unique.
accessories/button.coffee
Outdated
| #manipulate device name to reflect buttons-device + button name | ||
| deviceName = "#{device.name} #{buttonToTrigger.text}" | ||
| #manipulate device ID to allow multiple instances of device | ||
| deviceId = device.id + buttonToTrigger.id |
There was a problem hiding this comment.
would be good to separate the ids with e.g. underscore, so you can separate the ids.
hap.coffee
Outdated
| @@ -112,6 +120,20 @@ module.exports = (env) => | |||
| env.logger.debug("unsupported device type: " + device.constructor.name) | |||
| return null | |||
|
|
|||
| createAccessoriesFromTemplate: (device) => | |||
There was a problem hiding this comment.
Would be great if ButtonAccessory could handle multiple buttons itself, but this needs some rework at adding the accessory to the bridge, i guess.
There was a problem hiding this comment.
Agree but I had only half a Sunday and didn't want to mess to much with the architecture
hap.coffee
Outdated
| @@ -112,6 +120,20 @@ module.exports = (env) => | |||
| env.logger.debug("unsupported device type: " + device.constructor.name) | |||
| return null | |||
|
|
|||
| createAccessoriesFromTemplate: (device) => | |||
There was a problem hiding this comment.
Please make createAccessoriesFromTemplate the only method and remove createAccessoryFromTemplate. So in most cases the returned array will have just one item, but this way there is no duplicated code
| @@ -11,10 +11,27 @@ module.exports = (env) -> | |||
|
|
|||
| hapConfig: null | |||
|
|
|||
| constructor: (device) -> | |||
| constructor: (device, deviceId, deviceName) -> | |||
There was a problem hiding this comment.
please add some unit tests that verify the behavior of the new constructor.
|
Added some comments to the committed changes. I like the changes but it would be great if you could apply my comments to your changes. |
|
No problem :) all comments made sense and I even found the time to implement the suggested changes. Programming gets a bit rusty if you don't do it on a day to day basis anymore... |
hap.coffee
Outdated
| for accessory in newAccessories | ||
| if accessory? && !accessory.exclude() | ||
| bridge.addBridgedAccessory(accessory) | ||
| env.logger.debug("successfully added device " + device.name) |
There was a problem hiding this comment.
A last comment: would be good to use accessory.displayName instead of device.name, because for ButtonsDevices this would create the same log output for each button.
|
When will be the next release of pimatic-led-light including this change? I would like to finish #54 |
I added support for multiple buttons.
Behaviour for buttonsDevices with one button should be unchanged.