-
-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): add windows 7 notification support #4491
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
Conversation
I don't like that it's not an opt-in thing :/ because many people will not make use of this method. But I think we have too many features as-is, so I'm content.
EditEdit: I'm stupid and should pay attention to what's happening on discord |
Basically the problem is that we need the event loop to be running and the notification's show method to be called in the main thread. The current show API cannot do that. |
I could also introduce a |
Hmmm, I mean on paper that is the most elegant solution IMO. But we already have so many features and this would just be one more thing to document 🤔 So I'm fine with whatever you decide. If you go the feature route, name it |
I am in favor of adding this behind a feature flag. Most tauri apps won't care about win7 and those who want to support, will need to pay the penalty of having to include another crate. |
I've added the feature flag but kept the two different names for better documentation. I also put a deprecation warning on |
c88329b
to
8b306d7
Compare
Done. Tomorrow I'll add the Windows 7 section in the distribution guide (webview install + notifications). |
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___
)Other information