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!(global-shortcut): better APIs DX #969

Merged
merged 13 commits into from
Mar 7, 2024

Conversation

amrbashir
Copy link
Member

ref: #965

@amrbashir amrbashir changed the title feat(global-shortcut): add GlobalShortcutExt::register_with_handler feat(global-shortcut): refactor for better APIs DX Feb 19, 2024
@amrbashir amrbashir changed the title feat(global-shortcut): refactor for better APIs DX refactor!(global-shortcut): better APIs DX Feb 19, 2024
.changes/global-shortcut-refactor.md Outdated Show resolved Hide resolved
Co-authored-by: Simon Hyll <hyllsimon@gmail.com>
@lucasfernog
Copy link
Member

Do we reall need the without_handler variants? can't users just define an empty closure and then the global listener handles the event?

@amrbashir
Copy link
Member Author

I am not happy with it either, the idea is to provide a method to register a shortcut without handler that is appealing to use, without using an empty closure, but a long method name is not good either, maybe just call it register2?

@lucasfernog
Copy link
Member

lucasfernog commented Feb 21, 2024

maybe register_without_handler keeps its name register and the new API is named on_shortcut? so it's clear you intend on providing a callback there

that's even a non breaking change I believe

@amrbashir
Copy link
Member Author

maybe register_without_handler keeps its name register and the new API is named on_shortcut? so it's clear you intend on providing a callback there

I am afraid that might confuse users because on_ methods throughout tauri, are used to register just the listener.

@amrbashir
Copy link
Member Author

Changed to take an Option instead

@lucasfernog
Copy link
Member

maybe register_without_handler keeps its name register and the new API is named on_shortcut? so it's clear you intend on providing a callback there

I am afraid that might confuse users because on_ methods throughout tauri, are used to register just the listener.

that's what the on_ here will do:

fn on_shortcut(&self, shortcut: &str, handler: impl Fn()) {}
fn register(&self, shortcut: &str) {}

@amrbashir
Copy link
Member Author

maybe I am overthinking it, let's roll with that and see user feedback

@amrbashir
Copy link
Member Author

that's what the on_ here will do:

fn on_shortcut(&self, shortcut: &str, handler: impl Fn()) {}
fn register(&self, shortcut: &str) {}

hmm if we go with that approach, there will be inconsistencies between the JS and Rust APIs

@lucasfernog
Copy link
Member

it's already inconsistent though 😂 JS is just different.. right now i wish we had function overloading in rust :D

@amrbashir
Copy link
Member Author

Alright, added that along with a few methods on the Builder that should make it more Rusty hopefully.

@lucasfernog lucasfernog merged commit 62dafda into v2 Mar 7, 2024
13 checks passed
@lucasfernog lucasfernog deleted the feat/global-shortcut/with-handler branch March 7, 2024 16:33
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.

the "tauri-plugin-global-shortcut" plugin never removes items when unregistering shortcuts.
3 participants