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

refactor(core): refactor and fix event system following multiwebview support #8621

Merged
merged 27 commits into from
Feb 1, 2024

Conversation

amrbashir
Copy link
Member

closes #8435

@amrbashir amrbashir requested a review from a team as a code owner January 17, 2024 01:27
@amrbashir amrbashir closed this Jan 30, 2024
@amrbashir amrbashir force-pushed the fix/window-close-requested-from-another-webview branch from caeb9e8 to 57e3d43 Compare January 30, 2024 00:28
@amrbashir amrbashir reopened this Jan 30, 2024
@amrbashir amrbashir marked this pull request as draft January 30, 2024 01:45
@amrbashir amrbashir changed the title fix(core): prevent close if any js close event listener exists refactor(core): refactor and fix event system following multiwebview support Jan 30, 2024
@amrbashir amrbashir marked this pull request as ready for review January 30, 2024 19:15
@lucasfernog-crabnebula
Copy link
Contributor

I like how this simplifies the event system (most complex part of our system tbh.. even though it shouldn't be). But it changes a lot about the mental model when developing compared to v1. On v1 you treat the object that you can emit() as the source of the event, but now it always triggers to all listeners and you need to use emit_to instead (opposite of the current approach).

I'm fine with it (it's weird to me because I'm so used to the current approach but I can live with it and learn it) but this needs to be the last breaking change on this API. ever. And I'd like to get input from others @FabianLars @simonhyll

Co-authored-by: Lucas Nogueira <118899497+lucasfernog-crabnebula@users.noreply.github.com>
@amrbashir
Copy link
Member Author

I have been also thinking about maybe adding a spec or a diagram of how the event system emits and listens to events but don't hold this PR on it, I can add it later when I get to it.

@FabianLars
Copy link
Member

FabianLars commented Jan 31, 2024

I think it makes sense. It was quite messy before, and the event system (for us and the user) is indeed quite complicated so i like the explicitness in this approach.

In v1 many people tripped up over the emit vs trigger approach and the current v2 version is a bit ambiguous.

Looking at the diagram, one last thing that feels a bit meh is the listen_global api. I can't really put it into words (i'll try to later once my headache is gone lol), but maybe make the App/Handle an actual target too?
Then you'd also be able to emit events strictly to the apphandle without bothering the window/webview listeners.

I think we could then get rid of listen_global directly, or make it an "event sniffer" (that can listen to events no matter the target).

@amrbashir
Copy link
Member Author

amrbashir commented Jan 31, 2024

I think we could then get rid of listen_global directly, or make it an "event sniffer" (that can listen to events no matter the target).

That's what it currently does and should be kept that way IMO.

Edit: actually no it doesn't sniff, let's make it a sniffer then

I can't really put it into words (i'll try to later once my headache is gone lol), but maybe make the App/Handle an actual target too?

While we can definitely do this, and uses can use emit or emit_filter to emit to App only, it won't be possible using emit_to unless we change emit_to to take an EventTarget which will make the UX of the function a little worse IMO. But I definitely like this idea, I will think about it but if there is better suggestion, let me know.

@amrbashir
Copy link
Member Author

Alright, I have added App event target and thanks to Rust trait system we can still keep emit_to UX as is, I have also removed the diagram and made a simple spec markdown file.

@FabianLars
Copy link
Member

I really like what you came up with, nice work!

@lucasfernog
Copy link
Member

Looks amazing. I'm only wondering if we really need to keep the app/window/webview/webviewWindow emit and emit_to functions now that they all run the same logic. Having a single "entry point" for emitting events might be easier to understand and simplify migration too.

@amrbashir
Copy link
Member Author

They are all the same now, it is just a method on Manager trait

@lucasfernog lucasfernog merged commit a093682 into dev Feb 1, 2024
3 checks passed
@lucasfernog lucasfernog deleted the fix/window-close-requested-from-another-webview branch February 1, 2024 11:06
@canxin121
Copy link
Contributor

#8916

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.

[bug] The CloseRequestedEvent cannot prevent the closing of a dynamically created window.
5 participants