{"action":"created","issue":{"url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/323","labels_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/323/labels{/name}","comments_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/323/comments","events_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/323/events","html_url":"https://github.com/AngryLawyer/rust-sdl2/pull/323","id":56726974,"number":323,"title":"Upgraded `Timer` API and fix `Timer` tests...","user":{"login":"drbawb","id":235438,"avatar_url":"https://avatars.githubusercontent.com/u/235438?v=3","gravatar_id":"","url":"https://api.github.com/users/drbawb","html_url":"https://github.com/drbawb","followers_url":"https://api.github.com/users/drbawb/followers","following_url":"https://api.github.com/users/drbawb/following{/other_user}","gists_url":"https://api.github.com/users/drbawb/gists{/gist_id}","starred_url":"https://api.github.com/users/drbawb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drbawb/subscriptions","organizations_url":"https://api.github.com/users/drbawb/orgs","repos_url":"https://api.github.com/users/drbawb/repos","events_url":"https://api.github.com/users/drbawb/events{/privacy}","received_events_url":"https://api.github.com/users/drbawb/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-02-05T20:46:04Z","updated_at":"2015-02-05T22:57:29Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/pulls/323","html_url":"https://github.com/AngryLawyer/rust-sdl2/pull/323","diff_url":"https://github.com/AngryLawyer/rust-sdl2/pull/323.diff","patch_url":"https://github.com/AngryLawyer/rust-sdl2/pull/323.patch"},"body":"This PR is based on the feedback from issue #320 ... it introduces a more Rust-like interface for the Timer wrapper that is much less error-prone, and also memory safe!\r\n\r\nTo summarize how it works: \r\n\r\n* The user passes a `boxed` closure to the timer, e.g: `let t = Timer::new(Box::new(move|| { ... }));`\r\n* The timer runs until that closure returns a 0-interval OR ...\r\n* The timer is forcibly cancelled when it is dropped, this is because the closure will go out of scope, so further\r\n  invocations would point to freed memory.\r\n\r\n---\r\n\r\nThis also improves thread safety over the previous API(s), from the SDL2 documentation:\r\n\r\n> Use this function to set up a callback function to be run _on a separate thread_ after the specified number of milliseconds has elapsed.\r\n\r\nSince the callback might be run on a separate thread: the closure's environment must satisfy `Send` so that it can be moved to the thread the timer will run on.\r\n\r\nKnowing this: it would also be unsafe to allow the closure to freely mutate its environment, so the closure does not allow for a mutable environment. In practice this simply means you have to protect mutable data w/ a `Mutex` or `RwLock`, and shared data w/ an `Arc`.\r\n\r\n---\r\n\r\nThe tests have been improved so they hopefully don't fail spuriously anymore. I accomplished this by simply adding an upper-bound to the timer's callback so it stops trying to acquire the mutex after a certain number of iterations.\r\n\r\n---\r\n\r\nThe whole thing is marked `unstable` for a number of reasons:\r\n\r\n1. ... well, we've changed it 3 times now, so it really isn't stable\r\n2. Since we're forcing the caller to wrap the closure in a `Box<T>` we may ~~need~~ want to change the API based on how the placement-new syntax (`#![feature(box)]`) pans out in rust-1.0.0\r\n3. We may want some sugar so the caller doesn't have to wrap the closure in a `Box<T>` themselves?\r\n"},"comment":{"url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/comments/73148185","html_url":"https://github.com/AngryLawyer/rust-sdl2/pull/323#issuecomment-73148185","issue_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/323","id":73148185,"user":{"login":"nukep","id":1551458,"avatar_url":"https://avatars.githubusercontent.com/u/1551458?v=3","gravatar_id":"","url":"https://api.github.com/users/nukep","html_url":"https://github.com/nukep","followers_url":"https://api.github.com/users/nukep/followers","following_url":"https://api.github.com/users/nukep/following{/other_user}","gists_url":"https://api.github.com/users/nukep/gists{/gist_id}","starred_url":"https://api.github.com/users/nukep/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nukep/subscriptions","organizations_url":"https://api.github.com/users/nukep/orgs","repos_url":"https://api.github.com/users/nukep/repos","events_url":"https://api.github.com/users/nukep/events{/privacy}","received_events_url":"https://api.github.com/users/nukep/received_events","type":"User","site_admin":false},"created_at":"2015-02-05T22:57:29Z","updated_at":"2015-02-05T22:57:29Z","body":"I think the `F` generic type is superfluous; you should be able to have `Box<FnMut() -> u32 + Sync>` and have it work just fine. The closure environment should be able to move with the `Box`: it's how boxed closures worked before the unboxed closure reform. \r\n\r\nA generic type of `Timer` also means we can't create a polymorphic list of timers:\r\n```rust\r\nlet timers: Vec<Timer>;\r\n```"}}