EDCD / EDDI

Companion application for Elite Dangerous
Other
454 stars 82 forks source link

VoiceAttack variables are incompletely documented and in some cases not written #1875

Closed Tkael closed 3 years ago

Tkael commented 4 years ago

EDDI version in which issue is found

3.7.0-rc1 (any version really though)

VoiceAttack version in which issue is found (as applicable)

1.8.5 (any version really though)

Steps to reproduce

N/A

Expected

Full documentation, with all the same properties which are accessible from Cottle.

Observed

Array and object properties are set but not documented. Properties marked with a [JsonIgnore] tag (such as names from the ResourceBasedLocalizedName class) are neither written nor documented.

Investigation

We use entirely different methods for setting variables in VoiceAttack and for generating documentation of those VoiceAttack variables. At present:

Setting variables is done by walking a JToken (ignoring properties with the [JsonIgnore] tag but otherwise setting child properties of arrays and objects).

Generating documentation is done through object reflection of the top level object only. Only the properties contained within the VARIABLES property of the event are documented. Object and array properties are neither walked nor documented.

This creates a situation where users need to engage in a "guessing game" to access the child values of arrays and objects within VoiceAttack, with some expected variable values simply not being available. The user can work around the issue by using the SetState() function but it is a pain point.

richardbuckle commented 4 years ago

Understood. We may want to continue hiding the [JsonIgnore] properties though. What approach do you have in mind?

Tkael commented 4 years ago

Pruning everything with [JsonIgnore] isn't a great option IMO. That leaves us in some cases with no name at all being available to VoiceAttack, since (for example) all of the user facing outputs of the ResourceBasedLocalizedName are marked with [JsonIgnore].

We discussed possibly adding a new custom attribute for this. Possible candidates might be [VoiceAttackIgnore] or (if we want to exclude the properties from Cottle and can figure out how to make Cottle respect the tag) [UIIgnore].

Alternately, we could consider options for opting in properties rather than opting out, e.g. a [UIExpose] property. Once again, if we were to take this approach then we'd ideally want to apply the same to values exposed in Cottle.

Another alternative might be to manipulate the BindingFlags to determine what is included and what is not.

Cottle, by default, includes the public, private, and instance members. It does not seem to support limiting reflection using attributes.