-
Notifications
You must be signed in to change notification settings - Fork 903
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
Rename LoopDestroyed
to LoopExiting
#2975
Conversation
@@ -29,6 +29,7 @@ And please only add new entries to the top of this list, right below the `# Unre | |||
- **Breaking** Removed `EventLoopExtRunReturn` / `run_return` in favor of `EventLoopExtPumpEvents` / `pump_events` and `EventLoopExtRunOnDemand` / `run_ondemand` ([#2767](https://github.com/rust-windowing/winit/pull/2767)) | |||
- `RedrawRequested` is no longer guaranteed to be emitted after `MainEventsCleared`, it is now platform-specific when the event is emitted after being requested via `redraw_request()`. | |||
- On Windows, `RedrawRequested` is now driven by `WM_PAINT` messages which are requested via `redraw_request()` | |||
- **Breaking** `LoopDestroyed` renamed to `LoopExiting` ([#2900](https://github.com/rust-windowing/winit/issues/2900)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we don't link to the PR changing it things.
Would suggest to drop the links, they are redundant and easily obtainable with git blame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely there's no harm in making more context / information easily accessible from the changelog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just overly verbose for what it is, the context is already there with blame
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not verbose when it's rendered though.
I really appreciate when I see project changelogs that link changes back to issues / PRs and find that useful.
Tracking down context with git blame can be a faff, and inevitably at some point something will be re-formatted and it becomes harder to track context down via blame.
615c359
to
df8eaea
Compare
ff4f2d4
to
aa2cb60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you target a different branch?
df8eaea
to
4ed3e31
Compare
okey, just updated the base branch to The other branch that's stacked on this should automatically update it's base branch if this gets merged is just that this PR was stacked on a branch in my personal winit fork and so I had to make a dummy branch on origin that I could use as a temporary base. |
@rib could you rebase + force push this branch again? I'm not sure I can get CI unstuck. |
Considering the possibility of re-running an event loop via run_ondemand then it's more correct to say that the loop is about to exit without assuming it's going to be destroyed.
4ed3e31
to
aabc349
Compare
I've repushed to trigger CI. |
Considering the possibility of re-running an event loop via
run_ondemand
then it's more correct to say that the loop is about to exit without assuming it's going to be destroyed.This is one of the follow ups for #2900, assuming that #2767 hopefully lands soon.
Note: the base for this PR isn't currently set to master, since it's stacked on the changes for #2767