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

Fix fpsWhen, including consideration for hotswapping #143

Merged
merged 5 commits into from Jan 26, 2015

Conversation

Projects
None yet
3 participants
@jvoigtlaender
Contributor

jvoigtlaender commented Jan 26, 2015

fixes #139
yet another alternative to #141 and #142
based on @jwmerrill's version of fpsWhen

All local state now packaged in a signal, to allow Elm Reactor to pick it up during hotswapping.

@jwmerrill

This comment has been minimized.

Show comment
Hide comment
@jwmerrill

jwmerrill Jan 26, 2015

What's the reason for this? Does, e.g., plain fps (which starts with a true value for isOn) not start without it?

What's the reason for this? Does, e.g., plain fps (which starts with a true value for isOn) not start without it?

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 26, 2015

Owner

Exactly. Note that with foldp below, the update-functions isn't called on signal initialization at all. The initial value of a foldp is the explicit initial value argument, not anything obtained from a call of the updating function. So without this here, no timeout will be created at signal initialization (since the only other call to setTimeout is in the update-function, which will not get invoked).

Owner

jvoigtlaender replied Jan 26, 2015

Exactly. Note that with foldp below, the update-functions isn't called on signal initialization at all. The initial value of a foldp is the explicit initial value argument, not anything obtained from a call of the updating function. So without this here, no timeout will be created at signal initialization (since the only other call to setTimeout is in the update-function, which will not get invoked).

This comment has been minimized.

Show comment
Hide comment
@jwmerrill

jwmerrill Jan 26, 2015

Another alternative would be to unroll the foldp one step, so replace

A3(Signal.foldp, F2(update), initialState, input)

in line 68 with

A3(Signal.foldp, F2(update), update(input.value, initialState), input)

In that case, you'd also want initialState.wasOn to start false in line 37.

If you did that, you could drop the conditional here.

The advantage of unrolling is that it's more obvious that you won't end up with two timeout loops running in parallel.

jwmerrill replied Jan 26, 2015

Another alternative would be to unroll the foldp one step, so replace

A3(Signal.foldp, F2(update), initialState, input)

in line 68 with

A3(Signal.foldp, F2(update), update(input.value, initialState), input)

In that case, you'd also want initialState.wasOn to start false in line 37.

If you did that, you could drop the conditional here.

The advantage of unrolling is that it's more obvious that you won't end up with two timeout loops running in parallel.

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 26, 2015

Owner

Oh yes, I like that. Also, concerning the initial value handling it brings back some clarity that was a quality of the non-foldp version.

Owner

jvoigtlaender replied Jan 26, 2015

Oh yes, I like that. Also, concerning the initial value handling it brings back some clarity that was a quality of the non-foldp version.

@jwmerrill

This comment has been minimized.

Show comment
Hide comment
@jwmerrill

jwmerrill Jan 26, 2015

Contributor

Great. This looks good to me.

Contributor

jwmerrill commented Jan 26, 2015

Great. This looks good to me.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 26, 2015

Member

Great work guys, this looks really good!

Member

evancz commented Jan 26, 2015

Great work guys, this looks really good!

evancz pushed a commit that referenced this pull request Jan 26, 2015

Merge pull request #143 from jvoigtlaender/fix-fpsWhen-with-hotswapping
Fix fpsWhen, including consideration for hotswapping

@evancz evancz merged commit 21dad1f into elm:master Jan 26, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 26, 2015

Contributor

Looks like I pushed this commit: jvoigtlaender@f11f7ca too late, it occurs in my branch but not in the merge.

Think it is relevant?

Contributor

jvoigtlaender commented Jan 26, 2015

Looks like I pushed this commit: jvoigtlaender@f11f7ca too late, it occurs in my branch but not in the merge.

Think it is relevant?

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Feb 6, 2015

Contributor

I followed up my previous question with a new pull request (#160).

Contributor

jvoigtlaender commented Feb 6, 2015

I followed up my previous question with a new pull request (#160).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment