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

Fix the additional lifecycle events to work as documented #149

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

DJMcNab
Copy link
Contributor

@DJMcNab DJMcNab commented Sep 16, 2023

Required for Smithay/calloop-wayland-source#1.

I think this only needs to be a patch release, as this just brings the behaviour in-line with the docs

@DJMcNab DJMcNab marked this pull request as ready for review September 16, 2023 17:48
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

it's an internal change, so could be released as a patch release normally.

CHANGELOG.md Outdated
Comment on lines 5 to 6
#### Bugfixes
- Make the additional lifecycle events match the docs
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unclear what was exactly changed, probably should describe the behavior that could be observed (like endless loop due to use of lifecycle events).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is sufficient because:

  1. there are, to my knowledge, no users of this API, so no-one will have run into this
  2. The pr is readily available
  3. I don't want to make the changelog be really long with useless content

But I'll put whatever you prefer, if you give me the text you want to put here

Copy link
Member

@kchibisov kchibisov Sep 16, 2023

Choose a reason for hiding this comment

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

I mean, - Fix infinite loop when using a source with lifecycle events is short enough from what I can say? Or is it not reflecting the situation correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the infinite loop was only because the events were never getting read - we were still making progress, and other sources would have worked.

That is, the infinite loop is in the WaylandSource - yes because of this issue, but that isn't the only way this could manifest

Copy link
Member

Choose a reason for hiding this comment

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

Hm, yeah, that's actually fair. I'm fine with the changelog like that, it's just reads without providing a real value to the user.

Copy link
Member

Choose a reason for hiding this comment

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

probably @elinorbgr has a better suggestion on that matter, since we'd need to patch release and maybe yank 0.12.

Copy link
Member

Choose a reason for hiding this comment

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

Overall, I'd prefer to have a quick description of what the bug was, so something like this would be fine imo:

- Fix `EventSource::before_handle_events()` being erroneously give an iterator over synthetic events instead of real events

@kchibisov
Copy link
Member

CI should be fixed after #150

@elinorbgr
Copy link
Member

👍 with a rebase & changelog adjusment

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.62% ⚠️

Comparison is base (2feb1cf) 86.82% compared to head (a5415ee) 86.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   86.82%   86.20%   -0.62%     
==========================================
  Files          14       12       -2     
  Lines        1806     1624     -182     
==========================================
- Hits         1568     1400     -168     
+ Misses        238      224      -14     
Flag Coverage Δ
macos-latest ?
ubuntu-latest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/loop_logic.rs 88.69% <100.00%> (+1.78%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elinorbgr
Copy link
Member

Perfect thanks!

@elinorbgr elinorbgr merged commit 3f84ed6 into Smithay:master Sep 19, 2023
11 of 12 checks passed
@DJMcNab DJMcNab deleted the fixup-additional-lifecycle branch September 19, 2023 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants