w3c / at-driver

AT Driver defines a protocol for introspection and remote control of assistive technology software, using a bidirectional communication channel.
https://w3c.github.io/at-driver
Other
32 stars 4 forks source link

Settings #22

Closed zcorpan closed 1 year ago

zcorpan commented 2 years ago

This change covers milestone 1 in https://github.com/w3c/aria-at-automation/issues/15 . The approach here reflects what we discussed in our previous meeting, see https://lists.w3.org/Archives/Public/www-archive/2022Jun/att-0004/05-23-aria-at-minutes.html#t02

Currently, these suggested vendor-specific commands defined:

@mcking65 also suggested in the meeting: reset to defaults, and load config file. I'm not sure we need these, at least initially.

However, I've left an inline issue about a possible event, which the remote end could send if a setting is changed by the user or by some other software.

(I've edited this description to reflect the current state.)


Preview | Diff

jscholes commented 2 years ago

@zcorpan

@mcking65 also suggested in the meeting: return all settings, reset to defaults, and load config file. I'm not sure we need these, at least initially.

I think "return all settings" is a requirement. For this milestone, the settings will not be abstracted or unified in any way; each implementation will be free to expose whichever settings it chooses, using whatever format it chooses for the naming of setting keys. As such, obtaining a list of those settings is critical to allow any meaningful usage of the Settings API.

For resetting to defaults, I tend to think that a new session should just be started instead with a fresh config. Loading a config file is, for now, something I view as a non-starter, because all screen readers have a completely different config system, with different scope for different files. Unless the proposal was to support a unified file format, e.g. here's a stored dump of some settings, rather than setting them one by one via the API?

zcorpan commented 2 years ago

Why is obtaining a list of supported settings a requirement? Users of the API can read documentation to know which settings are supported, right? It seems to me you would also have to know what value to set it to, and what behavior to expect, to usefully change a setting.

jscholes commented 2 years ago

Users of the API can read documentation to know which settings are supported, right?

My intent was to indicate that, for users of the API, obtaining a list of setting tokens is a requirement. It can be a technical requirement built into the API, or a requirement indicating that each implementation should (must?) provide such a collection via documentation.

I would prefer the former, personally, as it can easily lead to the latter, and would allow the config of the running AT to be dumped for record keeping. But, if certain documentation is required for each implementation, can that be tracked somewhere?

It seems to me you would also have to know what value to set it to, and what behavior to expect, to usefully change a setting.

That's a documentation problem for sure. In terms of what is exposed by the API, I would only vote for: setting name, data type, and value. If we leave this entirely to documentation, I would be worried that some vendors would leave it out, not have it automatically kept up-to-date, etc. Whereas, even if the accompanying documentation wasn't present or complete, introspection could always be relied upon.

zcorpan commented 2 years ago

We can have a wiki page or a markdown file in this repo that lists known implementations and links to the documentation.

zcorpan commented 2 years ago

@SamuelHShaw @jscholes you said in our check-in meeting yesterday that PAC have reviewed this and had no comments. Can you mark it as reviewed with GitHub's UI? Then we can merge. Thanks!

jscholes commented 2 years ago

@zcorpan If you explicitly require a review, please request it from @jkva on GitHub so he is notified.

jkva commented 2 years ago

One general comment - I notice that params within a Command is sometimes a map { ... } and at other times a list [ ... ]. This complicates generic code to verify that a message payload has a valid general Command structure.

Could this be generalised so the params value is a map by default? The VendorSettingsGetSettingsParameters within VendorSettingsGetSettingsCommand could for example use

"params" : { 
  "settings": []
}

instead of the current spec'ed

"params" : []
zcorpan commented 2 years ago

@jkva yes, that sounds reasonable.

jkva commented 2 years ago

I noticed that the shape for VendorSettingsResult is defined as:

VendorSettingsResult = {
  VendorSettingsSetSettingsResult
}

while VendorSettingsSetSettingsResult is equal to EmptyResult (as the response to VendorSettingsSetSettingsCommand) from how I read the spec; should this not be VendorSettingsGetSettingsResult?

zcorpan commented 2 years ago

while VendorSettingsSetSettingsResult is equal to EmptyResult (as the response to VendorSettingsSetSettingsCommand) from how I read the spec; should this not be VendorSettingsGetSettingsResult?

Good catch, fixed: 213c68aee382164a85ed6f4de2c519aca85c6d2d

zcorpan commented 2 years ago

Could this be generalised so the params value is a map by default?

Done: f48b7c03c3d1327ff236337318e71c2b2c60a5d0