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: target specific window for window events #8826

Merged
merged 6 commits into from
Feb 12, 2024

Conversation

vonPB
Copy link
Contributor

@vonPB vonPB commented Feb 9, 2024

When using the onCloseRequested JS Api, the event was triggered even when closing other windows, not just the one of interest.

I believe it would be more logical for this event to be dispatched exclusively for the current window's close requests.

Before:

before.mp4

After:

after.mp4

@vonPB vonPB requested a review from a team as a code owner February 9, 2024 00:35
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Nice catch, I believe all window events should use emit_filter and target, EventTarget::Any, EventTarget::Window and EventTarget::WebviewWindow

@vonPB
Copy link
Contributor Author

vonPB commented Feb 9, 2024

Nice catch, I believe all window events should use emit_filter and target, EventTarget::Any, EventTarget::Window and EventTarget::WebviewWindow

Thanks for the feedback @amrbashir, I've changed the emit_to to emit_filter.
However, I'm curious about your suggestions to use EventTarget::Any as well. Wouldn't Any already target the others inherently? This would also continue to send the close requested event to all windows...

Or did you just mean to say that all events should use emit_filter?

@amrbashir
Copy link
Member

You're right, no need to target EventTarget::Any, I suggested that so the event would still trigger listeners from app.listen_any but it will be emitted to it anyway. For this PR, EventTarget::Window and EventTarget::WebviewWindow should be enough.

@vonPB
Copy link
Contributor Author

vonPB commented Feb 10, 2024

Ah, gotcha. So even if there is no matchingWindow or WebviewWindow the event should still be emitted internally for listen_any to catch, right? 257e95b

Copy link
Member

@amrbashir amrbashir 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

@amrbashir amrbashir changed the title fix: target specific window for CLOSE_REQUESTED_EVENT emission fix: target specific window for window events Feb 12, 2024
@amrbashir amrbashir merged commit 46b6598 into tauri-apps:dev Feb 12, 2024
19 checks passed
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.

None yet

2 participants