Skip to content

Remove assert for recovereable issue. Fixes #2597#2599

Merged
madsmtm merged 2 commits intorust-windowing:masterfrom
maximetinu:fix-assertion-failed-event-matches
Dec 23, 2022
Merged

Remove assert for recovereable issue. Fixes #2597#2599
madsmtm merged 2 commits intorust-windowing:masterfrom
maximetinu:fix-assertion-failed-event-matches

Conversation

@maximetinu
Copy link
Copy Markdown
Contributor

Fixes #2597

  • Tested on all platforms changed. Web. ✅
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users. Not relevant enough to go into CHANGELOG.md imo.
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior. No documentation to update
  • Created or updated an example program if it would help users understand this functionality No need
  • Updated feature matrix, if new features were added or implemented No new features, it's a fix

maximetinu added a commit to Sophya/winit that referenced this pull request Dec 21, 2022
maximetinu added a commit to Sophya/winit that referenced this pull request Dec 21, 2022
@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Dec 21, 2022

My best guess as to why this happens is that there's some sort of race-condition on the event registration - or maybe the scale-factor just changes enough such that the media query-state changes, while it is still within the specified bounds?

Idk, anyhow, I would rather just remove the assertion altogether

@madsmtm madsmtm added B - bug Dang, that shouldn't have happened DS - web Affects the Web backend (WebAssembly/WASM) labels Dec 21, 2022
@maximetinu
Copy link
Copy Markdown
Contributor Author

Should I remove it then? If so, then I can also remove the event parameter completely, as it won't be used anymore inside that function. What do you think?

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Dec 22, 2022

Yeah, go ahead and remove the assert and the parameter.

@maximetinu
Copy link
Copy Markdown
Contributor Author

Done! ✅

I haven't finally removed the event: MediaQueryListEvent parameter, I've just marked it as _unused. It cannot be removed because the listener at the MediaQueryListHandle needs it to be used by other parts of the code, like the canvas to check if the browser is on dark mode or not

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Dec 23, 2022

That's fine, thanks!

@madsmtm madsmtm merged commit 94e4c39 into rust-windowing:master Dec 23, 2022
@maximetinu
Copy link
Copy Markdown
Contributor Author

maximetinu commented Dec 23, 2022

Great, thanks! How often do you release? I'd love to have it as a 0.27.x hotfix, if possible 🙏 Or should I open another PR cherry-picking the fix to the 0.27.x branch?

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Dec 23, 2022

See #2600 - I'd guess that 0.28 will land some time before February.

@maximetinu maximetinu changed the title Swap assert by debug_assert for recovereable issue. Fixes #2597 Remove assert for recovereable issue. Fixes #2597 Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B - bug Dang, that shouldn't have happened DS - web Affects the Web backend (WebAssembly/WASM)

Development

Successfully merging this pull request may close these issues.

'assertion failed: !event.matches()' hits erratically for some users. Should this actually be an assertion?

2 participants