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(core): add windows 7 notification support #4491

Merged
merged 6 commits into from
Jun 28, 2022

Conversation

lucasfernog
Copy link
Member

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

@lucasfernog lucasfernog requested a review from a team as a code owner June 27, 2022 21:19
@JonasKruckenberg
Copy link
Contributor

JonasKruckenberg commented Jun 27, 2022

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.

Edit: Without knowing too much about the notifications feature, why can't this be a check inside show? That calls out to the notify_win7 method?

EditEdit: I'm stupid and should pay attention to what's happening on discord

@lucasfernog
Copy link
Member Author

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.

@lucasfernog
Copy link
Member Author

lucasfernog commented Jun 27, 2022

I could also introduce a windows7 or compatibility Cargo feature that changes the show method signature.

@JonasKruckenberg
Copy link
Contributor

JonasKruckenberg commented Jun 27, 2022

I could also introduce a windows7 or compatibility Cargo feature that changes the show method signature.

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 windows7 (or windows7_compat) though, we might have more specialized bug fixes for win7 in the future

@amrbashir
Copy link
Member

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.

@lucasfernog
Copy link
Member Author

I've added the feature flag but kept the two different names for better documentation. I also put a deprecation warning on show() in case windows7-compat is enabled so you don't accidentally use it.

@lucasfernog lucasfernog requested a review from a team as a code owner June 28, 2022 01:12
@lucasfernog
Copy link
Member Author

Done. Tomorrow I'll add the Windows 7 section in the distribution guide (webview install + notifications).

@lucasfernog lucasfernog merged commit 57039fb into dev Jun 28, 2022
@lucasfernog lucasfernog deleted the feat/windows7-notifications branch June 28, 2022 12:59
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

3 participants