Conversation
|
Great addition, I think it can be merged! Do you think the old value should be passed along the event as well? |
|
I can't think of any scenarios where you'd want to know the previous value, but it's probably safer to just implement it now in case it's necessary in the future. |
|
|
||
| requirement: Requirement; | ||
|
|
||
| protected _onChange = new SimpleEventDispatcher<SettingsValue[]>(); |
There was a problem hiding this comment.
I suggest using the regular EventDispatcher which takes 2 arguments instead of an array with a fixed length
There was a problem hiding this comment.
The EventDispatcher uses a sender and arguments no? I'm not entirely sure what you mean since it would just be EventDispatcher<Setting, number[]>.
There was a problem hiding this comment.
No the first argument doesn't have to be the sender
protected _onChange = new SimpleEventDispatcher<SettingsValue, SettingsValue>();And invoke with
_onChange.dispatch(prevValue, newValue)Instead of having an array with length 2
There might be some cases where you want to trigger some code when a setting is changed. A couple of examples:
Working off of the event handling for Achievements, I've added an
onChangeevent dispatcher to the baseSettingclass. I couldn't match the Achievements implementation exactly since it's possible to update the setting value from theSettinginstance directly, rather than it being fully controlled by the baseFeature. It's also probably better to have individual subscriptions since you only want change handlers for specific settings anyways.Also for this implementation I had to fix
BooleanSettingto use thesetmethod so it'll dispatch theonChangeevent as well.