Skip to content
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

Change order of lines to properly raise events #1545

Merged
merged 5 commits into from May 9, 2018

Conversation

@AlFasGD
Copy link
Contributor

@AlFasGD AlFasGD commented May 8, 2018

Resolves #2460 and every other future similar implementation's issues with resetting values.

This is not really tested, but it should (hopefully) not break anything.

@Aergwyn
Copy link
Member

@Aergwyn Aergwyn commented May 8, 2018

What do you mean not tested? And why does the lines changing order fix it?

@AlFasGD
Copy link
Contributor Author

@AlFasGD AlFasGD commented May 8, 2018

I mean that I did not check if the entire game remains sane. Changing the order of these lines indeed helped resolve the issue due to the ordering of the value adjustment.

@peppy
Copy link
Member

@peppy peppy commented May 9, 2018

This is a hard logic scenario to assess.

@peppy
Copy link
Member

@peppy peppy commented May 9, 2018

@smoogipoo requesting your review on this as I modified the behaviour and added considerably.

@peppy peppy requested a review from smoogipoo May 9, 2018
@peppy peppy added this to the May 2018 milestone May 9, 2018
@@ -164,14 +164,20 @@ public virtual void TriggerChange()

protected void TriggerValueChange(bool propagateToBindings = true)
{
ValueChanged?.Invoke(value);
// check a bound bindable hasn't changed the value again (it will fire its own event)
T beforePropagation = Value;

This comment has been minimized.

@smoogipoo

smoogipoo May 9, 2018
Contributor

Use the field rather than the property, since it is the field value that is propagated, and the property is virtual.

Would do the same for disabled, just for consistency.

@peppy peppy dismissed their stale review via eef186c May 9, 2018
@peppy peppy dismissed smoogipoo’s stale review May 9, 2018

fixed

@peppy
peppy approved these changes May 9, 2018
@peppy peppy merged commit 3cde222 into ppy:master May 9, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
peppy added a commit to peppy/osu-framework that referenced this pull request May 10, 2018
smoogipoo added a commit that referenced this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants