Combinables should respect an empty set value

[BretJohnson]

Click on a single checkbox for a combinable property.

• Its state changes from unset -> disabled.
• SetValueAsync is called, with 0 values enabled (most are unset, 1 is disabled). No enabled values = empty string on the Android side, which is the same as the entire property being unset.
• GetValueAsync is called, from the OnPropertyChange, and the new value is that all flags are unset, same as it was originally.

Result: It's currently impossible to enable any combinable flags, starting from the nothing enabled state.

[BretJohnson]

This is especially a problem for combinable properties, where, due to other changes, they now have his behavior when no values are currently set:

Click on a single checkbox for a combinable property.

• Its state changes from unset -> disabled.
• SetValueAsync is called, with 0 values enabled (most are unset, 1 is disabled). No enabled values = empty string on the Android side, which is the same as the entire property being unset.
• GetValueAsync is called, from the OnPropertyChange, and the new value is that all flags are unset, same as it was originally.

Result: It's currently impossible to enable any combinable flags, starting from the nothing enabled state.

[BretJohnson]

And per our earlier discussion, I'm OK, not adding a "none" choice here, as long as unchecking everything switches all checkboxes to the unset state, while checking at least one checkbox sets them all to either enabled (for at least one) or disabled, with none showing as unset. That should be fine, UI wise, and I believe is how it'll behave, once this fix is made.

[BretJohnson]

Adding back the subset milestone as this one is must fix for Android combineable properties, as they don't work at all currently (when they are in the everything off state - the typical default - you can't change anything currently).

[ermau]

As discussed previously, this is an issue of provider implementation. Receiving a SetValue with an empty value is not the same as an Unset. The value is set to empty, it's the same situation as null vs. String.Empty. Fixing this will fix the unset combinable issue.

[BretJohnson]

For our current Android property editors (win and mac) and for Android Studio, turning off all flags for a flags (combineable) property removes the property from the AXML (unset) rather than storing an empty string for the value in the AXML. We'd like to stay consistent with that in the proppy version. That also keeps it consistent with string properties.

Making all off (blank property value) and unset (no property present) be separate states in the UI makes for a more complicated UI with no real user benefit (that I can tell). It also differs from how things work today & likely user expectations.

So I'd like to see us fix this by making the change suggested originally, where clicking on an unset checkbox goes to on state rather than off state, at least for combinable properties. That's also a better UI in general, arguably.

cc: @JonDouglas as this is ultimately an Android Designer UX decision

[ermau]

Something you have to keep in mind is there's a fundamentally different approach to this property editor. It is not a pass-through window to the document behind it, its goal is to represent the value that the property is, which is not necessarily what's in a document (if there even is one). To that end, our existing solution and (AFAICT) Android Studio take the other approach which is just a pass-through to the backing document. This is why they set the property to blank when no values are selected, they are simply reflecting the document. However, the absence of values does not mean that property is set to the virtual "none". That's where the distinction comes in. Since the Android integration can not (yet) provide what the value for the property is in the absence of one in the document, we can only accurately reflect this as "we don't know."

Making all off (blank property value) and unset (no property present) be separate states in the UI makes for a more complicated UI with no real user benefit (that I can tell). It also differs from how things work today & likely user expectations.

You say they shouldn't be separate states, but make the case that you should be able to cycle through all three in #220. Which is it?

In the realm of "this is the actual value" and not "this is what the AXML says", anything other than this would be to user detriment. If I set value so that nothing is checked, I'd expect the value to be effectively "none", but it's really just unset. So, if the default value of this property isn't a "none", now the user isn't getting what they explicitly set.

I'd argue that the user expects what they set to be what they get, and what exists today works against user expectations. Not to mention the consistency with other designers in VS that treat things the same way (the value you're shown is what you're getting, best effort.)

The exception to all this is if every combinable property for Android has a default value of "none". If that's the case (which I highly doubt), report ValueSources.Default for the property.

So I'd like to see us fix this by making the change suggested originally, where clicking on an unset checkbox goes to on state rather than off state, at least for combinable properties.

This wouldn't actually fix the problem anyway. As soon as the user checks that box back to the off state, everything would go back to unset.

[BretJohnson]

Every combinable property for Android has a default value of unset. Given that should I really change to report ValueSources.Default instead of just ValueSources.Local?

The originally suggested change should fix this issue, as unchecking everything goes back to the unset state, true, but that's ok because checking something still works (it goes back to the set state).

In any case, do you have any objection to changing the UI so that clicking on an unset checkbox goes to the checked (not unchecked) state, esp. if PM says they prefer that UI?

cc: @cobey here it looks like @JonDouglas will be on paternity leave starting this weekend and @cobey will likely field most of this

[ermau]

Every combinable property for Android has a default value of unset. Given that should I really change to report ValueSources.Default instead of just ValueSources.Local?

If you know that all combinable properties default to no flags set, yes, you should report ValueSources.Default | ValueSources.Local. "Unset" is a term with a specific meaning so it's not interchangeable. Being unset in the XML is not necessarily the same as having no flags set when the property is live. If you can be sure that it is, then you know the Default and can report as such.

The originally suggested change should fix this issue, as unchecking everything goes back to the unset state, true, but that's ok because checking something still works (it goes back to the set state).

If I see a bunch of indeterminates and I want to make sure something is turned off, I click it, it turns on. Now I click it again to turn it off but it goes back to indeterminate. That is not "ok", it betrays the user's intention.

In any case, do you have any objection to changing the UI so that clicking on an unset checkbox goes to the checked (not unchecked) state, esp. if PM says they prefer that UI?

That's a separate discussion.

[BretJohnson]

I'm suggesting that it's:

indeterminate -> on -> off
instead of
indeterminate -> off -> on

(which is also how Android Studio behaves)

[ermau]

I understand what you're suggesting, I'm saying that that in-isolation does not fix the issue without other weird effects and the UX question of it is a separate discussion.

[BretJohnson]

What are the other weird effects?

[ermau]

If you report default this discussion becomes moot.

[BretJohnson]

Anyway, let's leave this bug as what was originally reported, the UX thing:

indeterminate -> on -> off
instead of
indeterminate -> off -> on

PM can make the call there.

[ermau]

The original issue described a bug with a prescribed fix. The issue is for the bug, not your desired fix. If you want a specific UX behavior, make a separate issue. When the issue is resolved in the integration, this will be closed.

[BretJohnson]

OK. Here's the UX bug: #232

Fixing that, at least for combineable properties, will fix this issue as well. If we don't change the UX as described there, we can make a different change here (which will probably happen after my vacation). But making that 232 change, at least for combinable properties, is the best option here IMO, UX wise.

[BretJohnson]

If I report ValueSources.Default will it change the UI to no longer show checkboxes in the indeterminate state for combinable properties?

[ermau]

If I report ValueSources.Default will it change the UI to no longer show checkboxes in the indeterminate state for combinable properties?

Yes, that's the point. If you know what the default is, it has no need to be indeterminate.

[BretJohnson]

Yeah, I don't think we want that UX consistency wise, as it'll be different than the non-combinable checkbox properties, but I'll let the UX folks decide.

[alanmcgovern]

Hey,

It looks like there are two completely distinct questions here. The first is whether or not clicking a checkbox in indeterminate mode should default to 'true' or 'false' as the next value. This question seems to have only been mentioned in the original issue description and every subsequent comment has been related to this bug, which was in the first comment: #210 (comment) . I'll edit the issue description to contain the same text as the first comment to ensure things are correctly in context :)

[alanmcgovern]

Ok, with that edit of the original issue complete, I have a question:

* Start off with a checkbox which is in the indeterminate state. Click on it.
* Its state changes from unset -> disabled.
* SetValueAsync is called, with 0 values enabled (most are unset, 1 is disabled). No enabled values = empty string on the Android side, which is the same as the entire property being unset.
* GetValueAsync is called, from the OnPropertyChange, and the new value is that all flags are unset, same as it was originally.

I tested one regular checkbox property, coincidentally the property called 'indeterminate' which has a default value of 'indeterminate' in android studio. When the property is set to indeterminate then there's no value in the android xml. When i enable the property the value android:indeterminate="true" is in the xml. When i disable the property the value android:indeterminate="false" is in the Xml.

From this we can know that this property does support a true indeterminate state and we can/should expose it.

I tested one combinable property, the foregroundGravity property. This does not support an indeterminate state. It is either enabled or disabled. From the user point of view there is no 'indeterminate' state as the format we work with only allows for it to be disabled or enabled. If we attempted to expose an indeterminate state for this property there would be no user interaction which clearly disambiguates 'indeterminate' and 'everything is disabled' because the underlying format has no representation of those two states.

If this was the only information we had then it is likely that we would adjust our implementation to treat this property as 'everything unchecked' when the xml value is unset, as that is a 1:1 mapping with how the user will interact with it and there will never be confusion for that case.

However, if you read the documentation for that property (https://developer.android.com/reference/android/view/View.html) it says that the default, when nothing is specified, is Fill. If we want to trust the documentation is accurate then for the case where the xml has no value set we have a deterministic state which is not just setting everything to unchecked - we know that fill corresponds to this:

For this case the 'not set in xml' state could be accurately represented by pre-setting the checkboxes in a way that makes it clear that this is an implicit value and then whenever something is manually checked/unchecked we would refresh the state of all the others.

This is just some food for thought on the topic, i'm not intending to prescribe a solution to every case because I haven't checked every single property and there are probably other cases which need different handling.

Also, using the documentation as a fallback to provide an indicator of the expected 'default behaviour is probably valuable, but would likely be too time consuming to do for V1. If that is true then we could focus on exposing the underlying format correctly (in terms of how the user interacts with it) and then layer the additional information about what really happens when the xml value is unset on a case by case basis :)

[garuma]

I already discussed with @ermau but basically flags properties (here called combinable) are the one case where the difference between unset and nothing set can become tricky (and the perfect example is indeed any of the gravity attributes).

Fortunately it's easy for us to determine which of those flags property have a neutral value (corresponding to the integer "0") so when everything is unchecked and we know there is no such value, we can use @ermau value constraint support to transform the nothing set case into either a unset case or the neutral value.

[BretJohnson]

It’s true that the AXML doesn’t normally allow distinguishing between the everything is disabled state and indeterminate state for combinable properties, like foregroundGravity. But when a combinable property is missing from the AXML arguably that means it’s in the indeterminate state (meaning it has an implicit default, same as regular bool properties missing from the AXML). It’s just the everything is disabled state that can’t normally be represented in the AXML for combinable properties (Android Studio removes the property completely from the AXML if unchecking all checkboxes).

Thus the current UI, where if foregroundGravity, for instance, is missing from the AXML (unset) we show all checkboxes in the indeterminate state - there's some implicit default there (like Fill for gravity), but the code (at least in V1) doesn't know what it is. Android Studio has more smarts around implicitly set flags, along with distinct UI (greyed checkboxes) to show implicitly set flags, but we don't currently and thus show as indeterminate. Regular checkboxes show indeterminate in the same scenario (unset in AXML = implicit, but unknown, default). So that's why the current UI behaves as it does.

(And FYI I originally wrote this before I saw jeremie’s comment above, though our comments aren't really inconsistent, at least for properties w/o a known neutral value)