action.onUserSettingsChanged proposal

[EmiliaPaz]

Proposal based on #346: New API: UserSettings.isOnToolbar change event

[bershanskiy]

This API permits addition of new observers, but not removal of old ones: action.onUserSettingsChanged().

Instead, it could match the established pattern of <eventName>.addListener() and <eventName>.removeListener() and become action.onUserSettingsChanged.addListener() and action.onUserSettingsChanged.removeListener().

[oliverdunk]

Instead, it could match the established pattern of <eventName>.addListener() and <eventName>.removeListener()

Thanks for the callout. I'll make sure @emilia-paz sees this but I suspect it is just a mistake in the proposal. We would almost certainly want to follow the normal event listener pattern here :)

[oliverdunk]

@zombie / @dotproto / @xeenon - could you take a look at this?

[Rob--W]

This looks good. Just one question, which is mainly relevant for the future implementation (and current documentation).

[Rob--W]

The type of the event parameter is identical to the return value of getUserSettings. Is that an intentional design?

With the current proposal consisting of one property only, it does not make that much of a difference. Once multiple events get added, the answer depends on the design decision:

• If we want to always include the latest value of all settings, then we can use the same type as the event parameter to onUserSettingsChanged listeners and as the return value of getUserSettings.
• If we want to only include modified values, then we need a new type that looks like onUserSettingsChanged, but with all fields optional.

[EmiliaPaz]

Great catch! We think it's more valuable to the user to only return the modified values. Updated the proposal