Skip to content

fix: remove notification permission prompt#4302

Merged
lucasfernog merged 6 commits into
tauri-apps:devfrom
betamos:rm-notification-prompt
Jun 9, 2022
Merged

fix: remove notification permission prompt#4302
lucasfernog merged 6 commits into
tauri-apps:devfrom
betamos:rm-notification-prompt

Conversation

@betamos
Copy link
Copy Markdown
Contributor

@betamos betamos commented Jun 8, 2022

NOTE: This is a draft, so it doesn't yet have a change file and updated docs.

Highlighted a point below which could be important for 1.0 API compatibility.

Unresolved:

  • Approval of this whole approach :)
  • Whether this is ok w.r.t Apple guidelines. (See older MacOS versions below)
  • Whether to remove or deprecate the requestPermission and isPermissionGranted APIs.

Platform specifics:

  • Windows and Linux: permission prompts are not required nor common.
  • MacOS 10.15+: A first-use prompt is provided by the OS.
  • MacOS older versions: Notifications can be turned off in system settings.

Details:

  • removed the permission dialog entirely
  • removed the settings module, which only stored the notification preference:
  • the settings module used bincode, which makes no compatibility guarantees
    across rust versions, struct changes etc
  • removed the bincode dep
  • the public API is unchanged, but permission related endpoints return
    static responses instead.

closes #4244
closes #1252

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

Platform specifics:

- Windows and Linux: permission prompts are not required nor common.
- MacOS 10.15+: A first-use prompt is provided by the OS.
- MacOS older versions: Notifications can be turned off in system settings.

Details:

- removed the permission dialog entirely
- removed the settings module, which only stored the notification preference:
- the settings module used bincode, which makes no compatibility guarantees
  across rust versions, struct changes etc
- removed the bincode dep
- the public API is unchanged, but permission related endpoints return
  static responses instead.

closes tauri-apps#4244
closes tauri-apps#1252
@lorenzolewis
Copy link
Copy Markdown
Member

Don't remove or deprecate the requestPermission() and isPermissionGranted(), those are important for a developer to know the appropriate action to take. I think what is returned from those needs to be adapted (same with sendNotification()), but that's for another discussion.

@betamos
Copy link
Copy Markdown
Contributor Author

betamos commented Jun 9, 2022

Deprecation is probably not even a thing pre-1.0, I don't know why I even said that :)

Removing OTOH may, counter-intuitively, be a really good idea. It would leave a blank slate for precisely those API changes you're proposing. They would be easy to add later, but hard to change if they're cemented in 1.0. I doubt we have time to get those changes in pre-1.0. Heck, I'm even doubting we can get this PR in, even if people agree would agree with this approach.

In either case, leaving this PR as-is for now to solicit more feedback.

@lorenzolewis
Copy link
Copy Markdown
Member

To sum up what you currently have in this PR if I'm understanding correctly:

  1. Remove tauri::settings, this was only used as part of notifications to date and the implementation wasn't solid
  2. Remove logic that reads/writes from tauri::settings for notifications
  3. Remove dialog/request to grant notification permission
  4. Remove is_permission_granted()

For point 4, this is the only one we shouldn't remove. Right now it directly reads from settings, but I would temporarily just hard code this to return True until we can implement a proper OS-level check.

Keeping in mind that tauri::settings has only been used in this area so far, @lucasfernog what do you think of the code change contents? Do you think this is something we could merge prior to 1.0 mainly to resolve point 3 above?

@lucasfernog
Copy link
Copy Markdown
Member

I agree with you @lorenzolewis

@lorenzolewis
Copy link
Copy Markdown
Member

@betamos Do you think that gives you enough info to modify this and get it to a reviewable state with those changes? Only code change is NOT removing is_permission_granted() (and then anything else required for a review and merge)

@betamos
Copy link
Copy Markdown
Contributor Author

betamos commented Jun 9, 2022

Thanks for the feedback. Point 4 refers to an internal method that is no longer needed. The public method remains, there are no API changes in this PR.

Feel free to triple check me on this. There's a little bit of magic in the #[command_enum] macro.

Undrafting then :)

@betamos betamos marked this pull request as ready for review June 9, 2022 18:28
@betamos betamos requested a review from a team June 9, 2022 18:28
@betamos betamos requested a review from a team as a code owner June 9, 2022 19:31
Comment thread core/tauri/src/endpoints/notification.rs
@betamos betamos requested a review from a team June 9, 2022 20:35
@lucasfernog
Copy link
Copy Markdown
Member

Btw @betamos with that many amazing contributions you should look into setting up commit signing :)

@lucasfernog lucasfernog merged commit f482b09 into tauri-apps:dev Jun 9, 2022
@betamos
Copy link
Copy Markdown
Contributor Author

betamos commented Jun 9, 2022

you should look into setting up commit signing :)

  • "I have it setup already, what are you talking about?"
  • Wait, no, why?
  • Ok, I only set it up on my laptop, and I made these commits on my desktop.

Thanks for the reminder. Done now :)

And thanks for attending to this PR. Glad to have this in before 1.0.

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] Duplicate permission prompt for notifications on MacOS Notification's permission request should have i18n

3 participants