Skip to content

Added some "how" and "why" docs to event handling.#1032

Merged
Osspial merged 6 commits into
rust-windowing:masterfrom
icefoxen:event-docs
Jan 5, 2020
Merged

Added some "how" and "why" docs to event handling.#1032
Osspial merged 6 commits into
rust-windowing:masterfrom
icefoxen:event-docs

Conversation

@icefoxen
Copy link
Copy Markdown
Contributor

@icefoxen icefoxen commented Jul 9, 2019

Basically I had questions when I started exploring the new
event API's, and as I figured out the answers I put down more info
about how everything works. This is not final, and suggestions
are welcome -- the code example in the event module docs is
particularly dubious, but it's how I'm used to thinking abou things
so it only made sense to me once I wrote that.

Note that my bias is towards using winit for writing games, so that's
the sort of things I was interested in. This may not be valid for
more general use cases.

  • Tested on all platforms changed
  • cargo fmt has been run on this branch
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@Osspial Osspial self-requested a review July 10, 2019 18:15
Copy link
Copy Markdown
Contributor

@Osspial Osspial left a comment

Choose a reason for hiding this comment

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

Thank you for contributing these improvements!

I've added some comments where they seemed necessary, but overall these changes are a good improvement.

Comment thread src/event.rs Outdated
Comment thread src/event.rs Outdated
//! for e in (window events, user events, device events) {
//! event_handler(e, ..., control_flow);
//! }
//! event_handler(EventsCleared, ..., control_flow);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This currently doesn't handle the RedrawRequested model which is... fair, since the current RedrawRequested model is unnecessarily complex and should be revised. See #1041.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't try to make a complete example, more a cartoon of how the pieces fit together... because I didn't understand the complete picture. Do we want a full and more complex example?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like to wait for #1041 to get resolved, since a full example that includes the current redraw complexity is complex enough that it would largely defeat the point of having this sort of example.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since it looks like #1041 might take a fair bit of work, should I remove those bits of the docs so we can merge this, or should I update them to reflect the current model so we can merge this?

Copy link
Copy Markdown
Contributor

@Osspial Osspial Jul 23, 2019

Choose a reason for hiding this comment

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

Could you remove this for now, but open another PR adding this example, so we can merge it once the #1041 changes have been finalized?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Coming back to this - we've got a redraw-requested-2.0 branch that implements the new API. There's a clear model for the redraw API there, so if you'd like to re-target this PR against that branch we should be able to merge it promptly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Life is rather busy now, so this is squarely in the "when I can get around to it" category.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The redraw-requested-2.0 branch has been merged onto master.

Comment thread src/event.rs Outdated
Comment thread src/event.rs Outdated
//! Some of these events represent different "parts" of a traditional event-handling loop. You could
//! approximate the basic ordering loop of [`EventLoop::run(...)`] like this:
//!
//! ```rust,no_run
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is failing the tests, since it doesn't even compile. Change this to ignore.

@goddessfreya goddessfreya added C - waiting on author Waiting for a response or another PR S - docs Awareness, docs, examples, etc. labels Nov 11, 2019
@Osspial Osspial removed the C - waiting on author Waiting for a response or another PR label Jan 3, 2020
@Osspial Osspial mentioned this pull request Jan 4, 2020
icefoxen and others added 4 commits January 4, 2020 13:47
Basically I had these questions when I started exploring the new
event API's, and as I figured out the answers I put down more info
about how everything works.  This is not final, and suggestions
are welcome -- the code example in the `event` module docs is
particularly dubious, but it's how I'm used to thinking abou things
so it only made sense to me once I wrote that.

Note that my bias is towards using winit for writing games, so that's
the sort of things I was interested in.  This may not be valid for
more general use cases.
@Osspial Osspial merged commit 57f29aa into rust-windowing:master Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S - docs Awareness, docs, examples, etc.

Development

Successfully merging this pull request may close these issues.

3 participants