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: enhance event system rust apis #7996

Merged
merged 16 commits into from
Oct 23, 2023
Merged

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Oct 10, 2023

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

The goal of this PR is to remove the trigger term from the APIs which I don't see very often and would prefer to ditch and stick to listen and emit only.

Changed emit to emit events to JS and Rust side which should be the default IMO but should we provide another set of APIs to target either JS or Rust?

@amrbashir amrbashir requested a review from a team as a code owner October 10, 2023 16:56
@amrbashir amrbashir marked this pull request as draft October 10, 2023 17:05
@amrbashir
Copy link
Member Author

amrbashir commented Oct 11, 2023

So I keep going back and forth between two decisions about one final piece of this PR which is Manager::listen_global and Manager::once_global:

  1. Keep them as is, to know that it is global
  2. Renamed them to Manager::listen and Manager::once, and when used from App/AppHandle it will be global and when used from Window it will be specific to this window.

Also since I have changed emit to emit events to JS and Rust side and not only JS side, which should be the default IMO but should we provide another set of APIs to target either JS or Rust?

@lucasfernog
Copy link
Member

I think @chippers might help you decide since he designed most of these APIs :)

@amrbashir amrbashir marked this pull request as ready for review October 18, 2023 00:42
@amrbashir
Copy link
Member Author

I think this PR is ready to go, I have decided to keep Window::listen to be specific to window and Manager::listen_global to be global. I am not the biggest fan of having _global prefix but good enough for now.

@lucasfernog lucasfernog merged commit 93c8a77 into dev Oct 23, 2023
23 checks passed
@lucasfernog lucasfernog deleted the refactor/event-system-apis branch October 23, 2023 18:10
olivierlemasle added a commit to olivierlemasle/tauri that referenced this pull request Oct 25, 2023
Since tauri-apps#7996, `Window::emit` has been moved to `Manager::emit`,
but `tauri::Manager` is imported into scope only for
`cfg(desktop)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

2 participants