-
Notifications
You must be signed in to change notification settings - Fork 8
Add onChange handler #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.