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 for #154 and #160 #253

Merged

Conversation

neopholus
Copy link
Contributor

Trying to read id from configuration before trying to determine MAC as ID to fix #154 and #160.
Add additional documentation on the Device tab.

…rs#160

Add additional documentation on the Device tab. Trying to read id from configuration before trying to determine MAC as ID.
@neopholus
Copy link
Contributor Author

I am new as active contributor here. If something is wrong or missing, please let me know so that I can fix it.

@mcm1957
Copy link
Member

mcm1957 commented Mar 16, 2024

@neopholus

Thanks for providing this PR. If noone else takes care I will look at it in more detail end of march. I cannot work anything for ioBroker starting next week and lasting app 2 weeks. If I forget and this PR is still open, please ping my by adding a comment and notice me @mcm1957

From the technical view (and after a first look) I would suggest to use the mac address if available and to use the name only as fallback. This would avoid a breaking change for existing users and keep the idea of unique state names.

In any case the name provided by users must be filtered much more. It might not contain any of the forbidden characters. So at minimum a "replace(adapter.FORBIDDEN_CHARS, '')" is reuired but '.', ' ' must be replaced in addition. (Spaces are known to cause problems with some vis but are not blocke due to historical reasons). In general I would disallow any characters except [a-zA-Z0-9-]. Further more a check should be added to ensure that the id is unique as non unique states could cause sideeffects.

@raintonr
Copy link
Contributor

Also, please update the readme change log and add instructions for your proposed change.

@neopholus
Copy link
Contributor Author

neopholus commented Mar 17, 2024

@raintonr:

Also, please update the readme change log and add instructions for your proposed change.

Thanks for the hint. Done.

@mcm1957:

From the technical view (and after a first look) I would suggest to use the mac address if available and to use the name only as fallback. This would avoid a breaking change for existing users and keep the idea of unique state names.

I think, the expected behavior is different. If I define a device manually, I expect that the name, that I provided is used. That was also the behavior before version 3.0.3. I already added some documentation to the devices tab (and now also to the readme), that if user wants names to correspond to the names of the automatically found devices, that the user must use the MAC address as name. Also I hope it will not be a breaking change, as usually users only added the devices manually when they have not been detected automatically.

Thanks for the other hints, I try to add code to check the validity of the name as soon as possible.

@mcm1957
Copy link
Member

mcm1957 commented Mar 17, 2024

If the name is preferred over the MAC its definitly breaking. But we could decide to do it and incremnte major version number. So this should be OK too.

added information about breaking change
Changed name filtering according to @mcm1957 recommendations
@neopholus
Copy link
Contributor Author

filtering names now as @mcm1957 recommended

Checking for duplicates in the ids as @mcm1957: requested
@neopholus
Copy link
Contributor Author

neopholus commented Mar 22, 2024

Added a check for duplicate IDs as @mcm1957 requested. Duplicates are detected when the adapter creates the ChromecastDevices objects. Would love to integrate this already in the admin device tab, but I do not have any clue how to do that AND also duplicates to automatically detected devices would not be detectable there.

Using the WORK IN PROGRESS indicator now in README.md
@neopholus
Copy link
Contributor Author

Ping @mcm1957, I hope I did all the requested changes and commited them already.
If anything is missing, just tell me, I am still an beginner here with ioBroker adapters. And GitHub...

@neopholus
Copy link
Contributor Author

Hi @mcm1957,
if you find some time, please approve this workflow....
If anything is missing, just tell me, I will try and fix it.
Thanks, Stefan

@mcm1957 mcm1957 merged commit 9941ca2 into iobroker-community-adapters:master Apr 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Home Mini in different subnet does not work any more
3 participants