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

Make period a nullable property of reactive timer and clarify documentation #1957

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

glopesdev
Copy link
Member

@glopesdev glopesdev commented Aug 6, 2024

Ignoring the value of TimeSpan.Zero for the Period property is an unfortunate historical legacy. The property should really expose a nullable type when deciding between which Observable.Timer overload to call.

This PR changes the type signature of Period to be a nullable timespan, but retains the same behavior by requiring Period to be larger than zero for periodic timers. We also introduce a slight modification to its serialization behavior to ensure that a value of zero is always serialized as null.

This will start the migration away from using TimeSpan.Zero as a valid value, and might give us some room to consider reinstating the zero period as a high frequency periodic timer for 3.0.

We could also introduce an automatic conversion at the level of the editor, but this might break embedded include workflows which happen to be using the old timer semantics, since these are not converted automatically by the editor until they are ungrouped and resaved.

The embedded description string was also fixed for the shaders timer.

Fixes #1947

@glopesdev glopesdev added the fix Pull request that fixes an issue label Aug 6, 2024
@glopesdev glopesdev added this to the 2.9 milestone Aug 6, 2024
@glopesdev glopesdev changed the title Make period nullable and clarify documentation Make period a nullable property of reactive timer and clarify documentation Aug 6, 2024
Copy link
Member

@PathogenDavid PathogenDavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nitpick over wording, could go either way

Bonsai.Core/Reactive/Timer.cs Outdated Show resolved Hide resolved
Bonsai.Shaders/Timer.cs Outdated Show resolved Hide resolved
@glopesdev glopesdev merged commit bfc8409 into bonsai-rx:main Aug 6, 2024
10 checks passed
@glopesdev glopesdev deleted the issue-1947 branch August 6, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected Reactive.Timer behavior
3 participants