search

petrleocompel / hikaxpro_hacs

HACS repository of Hikvision Ax Pro integration for home assistant
MIT License
40 stars 4 forks source link

[FEATURE] Add config option to disable Arm at Home for the alarm panel #122

Open ProtoxiDe22 opened 1 month ago

ProtoxiDe22 commented 1 month ago

I've started using the integration, but in my use case of the alarm panel, i will never use the "Home" arming mode, as such, i thought it could be nice to have an option to disable it in the config flow and having only "Arm Away" and "Disarm" in the Panel.

this PR does exactly that by adding an option in the config flow, and works for both the mail panel and the Areas subpanels. The default configuration does not modify the current behavior of the integration

image

image

I've only added the string for the English language, so that probably needs to be addressed, i added the Italian string as that is my native language, but i would like to hear what's the correct practice for this repo, should we leave the string as is and wait for contributions from native speaking people or should we translate the string with an online service?

petrleocompel commented 1 month ago

This is very user specific scenario. I understand your needs but question is if this what you are asking for is not only an template of alarm_control_panel -> https://www.home-assistant.io/integrations/alarm_control_panel.template/ where you would disable the specific mode.

So in the end there would be no real reason to implement this. I know this seems "easy" to just put it inside of the integration but question is if it is necessary.

petrleocompel commented 1 month ago

It different from issue #90 where FR is to disable all arming functionality.

ProtoxiDe22 commented 1 month ago

This is very user specific scenario. I understand your needs but question is if this what you are asking for is not only an template of alarm_control_panel -> https://www.home-assistant.io/integrations/alarm_control_panel.template/ where you would disable the specific mode.

So in the end there would be no real reason to implement this. I know this seems "easy" to just put it inside of the integration but question is if it is necessary.

I see your point, but i do think implementing this in the integration is more elegant and easy for the final user, in my experience creating template entities is kinda tedious and it would be expecially so if an user needs panels for multiple areas, i don't think giving this as an option is a bad idea.

It different from issue #90 where FR is to disable all arming functionality.

as for #90 i guess if you want, for feature completeness i could also add the option to disable ARM_AWAY, however, there's no feature in the alarm control panel entity to enable/disable disarming of the system, so i don't know what could be done about that.

ProtoxiDe22 commented 1 month ago

In any case, in all of my infinite wisdom, i had the logic backwards yesterday and didn't notice due to lack of sleep. the feature was supposed to remove the option ARM_HOME, was removing the option ARM_AWAY instead. The last commit fixes that

petrleocompel commented 1 month ago

I understand the user point. We will see where will go with this. But I will have to add options to disable arming and disarming completely to make it complete. 1 disable option is not a great. Does not make sense in long run.

I will try to take a look over a weekend.