Skip to content

Conversation

@HLXII
Copy link
Contributor

@HLXII HLXII commented Jul 17, 2021

There might be some cases where you want to trigger some code when a setting is changed. A couple of examples:

  • You're using a separate handling of dark theming from TailwindCSS that modifies the data-theme of the page.
  • Changing a setting requires additional handling that's not fully handled by Vue.
  • Possibly in the future when KeyBinding settings are implemented, you will want to update the Keybind whenever the player changes that setting.

Working off of the event handling for Achievements, I've added an onChange event dispatcher to the base Setting class. I couldn't match the Achievements implementation exactly since it's possible to update the setting value from the Setting instance directly, rather than it being fully controlled by the base Feature. 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 BooleanSetting to use the set method so it'll dispatch the onChange event as well.

@Ishadijcks
Copy link
Contributor

Great addition, I think it can be merged!

Do you think the old value should be passed along the event as well?

@HLXII
Copy link
Contributor Author

HLXII commented Jul 25, 2021

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[]>();
Copy link
Contributor

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

Copy link
Contributor Author

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[]>.

Copy link
Contributor

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

@HLXII HLXII force-pushed the settings-onchange branch from 8a2df9f to ddac6dd Compare July 25, 2021 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants