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 GatewayAction sensor not reset to default #284

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

mikechouto
Copy link
Contributor

@mikechouto mikechouto commented Aug 4, 2024

Thank you @niceboygithub

@niceboygithub
Copy link
Owner

@EricCorleone any option?

@EricCorleone
Copy link
Contributor

As far as I know, the "unknown" state only appears when you reboot your HA and no event has been reported yet, and it should be the empty value None rather than the string "unknown".

@mikechouto
Copy link
Contributor Author

I can change it to none instead of unknown but i'm sure that it not empty value in previous version of HA

@EricCorleone
Copy link
Contributor

I'm not sure if you are talking about frontend display, because when the state is None, it will be displayed as "Unknown" on the frontend.

@mikechouto mikechouto force-pushed the fix/vibration_sensor branch from d7eb069 to ab4ecde Compare August 5, 2024 16:19
@mikechouto
Copy link
Contributor Author

@niceboygithub @EricCorleone I've change the default value for the vibration sensor to none and repush the branch.

Thanks

@EricCorleone
Copy link
Contributor

EricCorleone commented Aug 6, 2024

@niceboygithub @EricCorleone I've change the default value for the vibration sensor to none and repush the branch.

Thanks

Sorry I forgot to mention that using the empty string '' as the default value would be a better choice😂because it will be displayed completely blank on the frontend, which looks much better than "unknown" (which is none in the code).
IMG_8888

@mikechouto
Copy link
Contributor Author

mikechouto commented Aug 6, 2024

@niceboygithub @EricCorleone I've change the default value for the vibration sensor to none and repush the branch.
Thanks

Sorry I forgot to mention that using the empty string '' as the default value would be a better choice😂because it will be displayed completely blank on the frontend, which looks much better than "unknown" (which is none in the code). IMG_8888

Hi, sorry you might miss understood from my previous reply, when the state is none HA can correctly display it and it wouldn't show unknown please find the information in screenshot below. IMHO using a default state would be better than empty string as having a readable default state can bring more advantage when we write and maintain script/automation.

The default values might be able to be like none for sensors, off for switch... etc.

screenshot 2024-08-06 上午10 00 57

@EricCorleone
Copy link
Contributor

EricCorleone commented Aug 6, 2024

The None I meant is not a string, but the empty value in python without quotation marks and the first letter is capital. It will be rendered as "unknown" on the frontend according to my last try. But I guess the string "unknown" also works according to your screenshot.

And the reason I prefer the empty string over "unknown" is simply because it looks better, but both are ok I guess (also I don't think I will use the empty/unknown state in automations).

@mikechouto
Copy link
Contributor Author

mikechouto commented Aug 6, 2024

Hey Eric yeah I've also tested the None in python which will be render as unknown on frontend. And I have some automation base on state changes so having a default value would let my automation be more readable LOL. I think we can wait for @niceboygithub to see what he would like the default value to be for long term usage, then i can modify. Thanks again for the detail explain.

@caibinqing
Copy link
Contributor

Both "" and "none" are hardcoded. If I had to choose one, I'd choose "".
I searched the code history and found that it was always "". If you change this default, you might break other people's automation.

@mikechouto mikechouto force-pushed the fix/vibration_sensor branch from ab4ecde to b1399f3 Compare August 6, 2024 10:36
@mikechouto
Copy link
Contributor Author

@Necroneco Thanks for pointing it out I've modify the branch to just moved reset_state into GatewayBinarySensor class and the function will always be '' now.

@caibinqing
Copy link
Contributor

But this is not much different from #280. Currently, only these two classes need this method.

@mikechouto
Copy link
Contributor Author

mikechouto commented Aug 6, 2024

@Necroneco I’m not sure if there are other devices that got effected as I don’d have a large variety to test against. And yeah merging both should be fine, I was just thinking to reduce duplicate code. ^_^

@niceboygithub niceboygithub merged commit 7f658d5 into niceboygithub:master Aug 14, 2024
@caibinqing caibinqing mentioned this pull request Aug 16, 2024
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.

4 participants