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

revise association handling #1193

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jimzrt
Copy link
Contributor

@jimzrt jimzrt commented Jul 1, 2019

Hi @cdjackson ,

I know this is the third pull request regarding association handling, but just like last time it fixes my particular problems and maybe you deem it a general problem / fix, so I just want to put it out there.

I tried to find out what is really causing UI problems regarding group associations and after the last pull request, habmin is working correctly for me whereas paperUI is not. (short discussion #43)

paperUI displays associations depending on the device's configuration and expects this configuration in a specific format:
if the group can have multiple members, it expects a list, and if not, it expects just the member.

The group configuration in the binding is set irregardless of the device being able to have multiple members - it returns as a list if there are multiple members and it returns the first member if it is currently the only member. When there is no member currently set, it returns an empty string.

I changed it so that the device's configuration is taken into account in the ZWaceThingHandler and it sets and return its group association members depending on the device being able to have multiple members in that particular group.
I.e. if the device can have multiple members, a list is always returned (even if there are no members).

I think this behaviour is more intuitive and what I would expect when asking for or setting group members.
Additionally, paperUI and habmin are working without any problems when setting and displaying group associations now.

I added a few fixes like preventing to add nodes as group members if the node is not know by the controller or adding multiple members if the device can only have one member.

Signed-off-by: James Tophoven [email protected]

Signed-off-by: James Tophoven <[email protected]>
@cdjackson
Copy link
Collaborator

I would probably prefer not to buffer the configuration locally in the thing handler as it could get out of sync with the real configuration. I don't think there's a need to store it as a field in the class is there?

I'd also question if it's the best approach in general or if we shouldn't just use the maxNodes field in the AssociationGroup class which is already available here. This would remove the dependancy on the configuration which I think it nicer, but it has the drawback that if the database is defined differently than what the device reports, there will be a problem - my guess if this happens is there will be a problem anyway though!

@cdjackson
Copy link
Collaborator

Oh - and I meant to add - thanks for looking at this :)

@lsiepel
Copy link
Contributor

lsiepel commented May 31, 2020

Does this PR also fix this issue? #1205
I'm happy to test this, but someone has to provide a jar file before i can do so.

Base automatically changed from master to main January 18, 2021 20:05
@lsiepel
Copy link
Contributor

lsiepel commented Jan 4, 2022

@jimzrt any progress on this? I'm happy to test as i still have problems with the associations.

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.

3 participants