dummylabs / thewatchman

Home Assistant custom integration to keep track of missing entities and actions in your config files
MIT License
481 stars 20 forks source link

Redundant UI: Checkboxes and Toggles #95

Closed lindhe closed 5 months ago

lindhe commented 1 year ago

I'm really confused by the UI when calling the service. There's both checkboxes and toggle switches available for some of the options. Is that a bug?

Screenshot from 2023-07-26 18-30-02

ildar170975 commented 1 year ago

Do not consider this as a bug. Left checkbox is “add this option into the call”. Right - a value of the option.

You may have a look at a “recorder.purge” service, it also has a similar checkbox.

lindhe commented 1 year ago

To my mind, it is awfully confusing. The thing that makes this so confusing to me is that this makes use of two bools for each field:

There's a lot of state for only these two values, because of this!

All possible states for `send_notification` and `parse_configuration`

1. Both `true` ```yaml send_notification: true parse_configuration: true ``` 1. Both `false` ```yaml send_notification: false parse_configuration: false ``` 1. One of each ```yaml send_notification: true parse_configuration: false ``` 1. The other one of each ```yaml send_notification: false parse_configuration: true ``` 1. One `true` ```yaml send_notification: true ``` 1. The other one `true` ```yaml parse_configuration: true ``` 1. One `false` ```yaml send_notification: false ``` 1. The other one `false` ```yaml parse_configuration: false ``` 1. And, of course, nothing! ```yaml ``` The cases for 1–4 are clear as day. They express exactly what the desired state should be. But the rest of them, what do they even indicate? It is impossible to know without considering what the program code for handling the call looks like and what default values it uses. And what is more is that this would get even worse if more options were added in the future – the number of possible states grows exponentially!

Personally, I think it would be much clearer to the user if there was only one bool per option. It makes for only four possible states to consider, rather than nine. In my experience, this this is also much easier to program for since one does not have to parse the data structure and consider whether a key exists or not – all keys always exists!

Also, it would simplify the design by only having a single place where default values are set: the GUI. As it stands right now (if I've understood you correctly), there are two places that needs to be changed if we want to change defaults: both in the GUI but also in the program code where the call is processed.

ildar170975 commented 1 year ago

The current approach seems to be same as for other services (including conventional). Not sure that changing a common way may be accepted. The logic is simple: — the option is added? (yes / no) — the value of the option?

And the value could be bool - or integer, or whatever. This is a COMMON approach.

lindhe commented 1 year ago

The current approach seems to be same as for other services (including conventional).

Could you please elaborate on this? You have mentioned the recorder.purge service, but you wrote "services" (plural). What other services do you have in mind?

ildar170975 commented 1 year ago

Do not have an access to a PC now, cannot name other services, I just remember that at least SOME of them have - I use them more often then others. Many services have optional parameters, these parameters should have that “left checkbox”. And here I may be wrong - probably some services do NOT have this “left checkbox”, which is wrong since not following the common way.

ildar170975 commented 1 year ago

I can list here some more services with optional parameters on a week when I will have an access to a PC.

lindhe commented 1 year ago

Alright, I just want to address a handful of minor points here.

Left checkbox is “add this option into the call”. Right - a value of the option.

Thanks for explaining this to me! I genuinely didn't get it. Perhaps I would have given time.

I still don't know what default values send_notification and parse_configuration have when the left checkbox is unticked. Can you explain that for me too? (it clearly says in the item descriptions, I just had a quick glance at the UI)

The logic is simple: — the option is added? (yes / no) — the value of the option?

It is simple. But my logic is simpler: — Should the value be true or false?

And the value could be bool - or integer, or whatever.

Yes, but these are bools. I do not suggest we change it for other types, just booleans.

This is a COMMON approach. […] Not sure that changing a common way may be accepted. […] And here I may be wrong - probably some services do NOT have this “left checkbox”, which is wrong since not following the common way.

I'm at a loss as to why you think this is common, but let's assume for a minute that it is common. So what? If we find a better way than the common way, I say let's go for it! :rocket:

It is not conventional nor common outside of this specific service and the recorder service you mentioned. Perhaps Home Assistant services in general, but i doubt it (granted its pretty tedious to look for examples, which makes this hard to verify).

But my point here is that it is much more common for software in general to use my suggested pattern for bools.

All of the toggles on my Android phone, for example, uses my suggested pattern of having a single switch. It does not have one toggle for "use this setting" and another one for setting the value. For bools, that is. There's plenty of other types of values that do have both a toggle and additional data, because that's the nature of other data types than bool.

I'd like to encourage you to keep this in mind when navigating graphical interfaces in applications from now on. It's by far more common to use my suggested approach for bools than the two switch approach.

lindhe commented 1 year ago

I just realised there are even inconsistencies within The Watchman! "Create file report" does not have that random checkbox, despite the description saying its optional!

Screenshot_20230728_215338_Home Assistant.png

dummylabs commented 5 months ago

@lindhe It seems that these weird checkboxes are not configurable by integration developer and cannot be controlled. All properties of the Watchman service are declared as optional, but the first one doesn't have the checkbox. If anyone have any documentation on how to control these, please let me know.