search

parse-community / Parse-SDK-Flutter

The Dart/Flutter SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
575 stars 190 forks source link

feat: Add new new push notification interface `ParseNotification` for managing push notifications #949

Closed mbfakourii closed 1 year ago

mbfakourii commented 1 year ago

Pull Request

Issue

Closes: #936

Approach

In the new method, ParseNotification should be given to ParsePush

ParsePush.instance.initialize(
  firebaseMessaging,
  parseNotification: ParseNotification(onShowNotification: (value) {
    // show notification
  }),
);

Tasks

parse-github-assistant[bot] commented 1 year ago

I will reformat the title to use the proper commit message syntax.

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this pull request!

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: +0.27% :tada:

Comparison is base (aefc84e) 39.33% compared to head (b6146ec) 39.60%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #949 +/- ## ========================================== + Coverage 39.33% 39.60% +0.27% ========================================== Files 60 60 Lines 3356 3333 -23 ========================================== Hits 1320 1320 + Misses 2036 2013 -23 ``` | [Files Changed](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) | Coverage Δ | | |---|---|---| | [packages/flutter/lib/parse\_server\_sdk\_flutter.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZmx1dHRlci9saWIvcGFyc2Vfc2VydmVyX3Nka19mbHV0dGVyLmRhcnQ=) | `20.00% <ø> (ø)` | | | [...utter/lib/src/notification/parse\_notification.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZmx1dHRlci9saWIvc3JjL25vdGlmaWNhdGlvbi9wYXJzZV9ub3RpZmljYXRpb24uZGFydA==) | `0.00% <0.00%> (ø)` | | | [packages/flutter/lib/src/push/parse\_push.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZmx1dHRlci9saWIvc3JjL3B1c2gvcGFyc2VfcHVzaC5kYXJ0) | `0.00% <0.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mtrezza commented 1 year ago

@cool2apps Could you test this PR out, if it works for you?

mbfakourii commented 1 year ago

@cool2apps Could you test this PR out, if it works for you?

I think it is ready for merge, it is no longer dependent on another library

mbfakourii commented 1 year ago

Could you please change the PR title? It currently seems to describe the change, but it should describe the issue that is being fixed.

I changed it, I hope it's good, I didn't have a better idea for the title

mtrezza commented 1 year ago

The PR title must describe the issue, the solution of how it was fixed is an option addition. It's still not clear what this PR fixes. If it'S not "properly" displaying notifications, could we specify what that means? Or is it something else?

mbfakourii commented 1 year ago

The PR title must describe the issue, the solution of how it was fixed is an option addition. It's still not clear what this PR fixes. If it'S not "properly" displaying notifications, could we specify what that means? Or is it something else?

I changed it, what do you think?

mtrezza commented 1 year ago

Again, this describes the change, not the issue that this PR fixes.

mbfakourii commented 1 year ago

Again, this describes the change, not the issue that this PR fixes.

  • Is this PR fixing a bug, then what does it fix?
  • Is the PR adding a new or changing an existing feature and not fixing a bug, then what is that feature?

In this PR, we solved the problem of using the notification library overhead by removing it and replacing it with an interface. I think this PR solves a bug

mtrezza commented 1 year ago

If it was a bug we should be able to describe what's broken or not working, but that seems not possible. If it removes a overhead then maybe it's a performance improvement?

How about:

perf: Remove push notification library overhead by replacing it with a push notification interface

Just to confirm again, this PR does not require any changes in a developer's app code, right?

mbfakourii commented 1 year ago

If it was a bug we should be able to describe what's broken or not working, but that seems not possible. If it removes a overhead then maybe it's a performance improvement?

How about:

perf: Remove push notification library overhead by replacing it with a push notification interface

Just to confirm again, this PR does not require any changes in a developer's app code, right?

Changes are required on the developer side. I think this is a breaking changes

but since ParsePush is a new feature and this change is very small, I'm not sure there is a need for a MAJOR version increase.

mtrezza commented 1 year ago

We do not distinguish between "small" and "large" breaking changes. Any breaking change usually requires a major version increment, the exception being security fixes which are evaluated on a per-case basis.

but since ParsePush is a new feature

So then this is would be a feat PR that contains a breaking change. A feat is higher in priority than a perf change, but we can add the performance implication in the changelog entry.

Merging a breaking change as non-breaking in flutter is especially bad due to the de-facto standard of using range operators for dependency versions. That means they will auto-update and potentially break apps. We need to be extra careful because of that.

mbfakourii commented 1 year ago

We do not distinguish between "small" and "large" breaking changes. Any breaking change usually requires a major version increment, the exception being security fixes which are evaluated on a per-case basis.

but since ParsePush is a new feature

So then this is would be a feat PR that contains a breaking change. A feat is higher in priority than a perf change, but we can add the performance implication in the changelog entry.

Merging a breaking change as non-breaking in flutter is especially bad due to the de-facto standard of using range operators for dependency versions. That means they will auto-update and potentially break apps. We need to be extra careful because of that.

I got it, so it is a breaking change and feat added in title

mtrezza commented 1 year ago

I think the PR title could be improved; with "user interface" you are probably referring to the programming interface, not a UI.

How about this change log entry:

BREAKING CHANGES

  • The push notification library flutter_local_notifications is replaced with the new push notification interface ParseNotification (#949)

Features

  • Add new new push notification interface ParseNotification for managing push notifications (#949)

Feel free to add any info to the "breaking change" line that may help developers to cope with the braking change.

mbfakourii commented 1 year ago

I think the PR title could be improved; with "user interface" you are probably referring to the programming interface, not a UI.

How about this change log entry:

BREAKING CHANGES

  • The push notification library flutter_local_notifications is replaced with the new push notification interface ParseNotification (#949)

Features

  • Add new new push notification interface ParseNotification for managing push notifications (#949)

Feel free to add any info to the "breaking change" line that may help developers to cope with the braking change.

I changed it, what do you think?

mtrezza commented 1 year ago

We would need a changelog entry as well

mbfakourii commented 1 year ago

We would need a changelog entry as well

added

mtrezza commented 1 year ago

@mbfakourii I've edited the changelog; to better explain the format, when there's a breaking change, there is the BREAKING CHANGES section that can be more verbose and explain what is breaking, how the developer can transition and what to be careful about. Then there is still the Features section that has the shorter entry for the feature change. In other words, a PR always requires a base entry like Bug Fixes or Feature and if it's breaking it requires an additional entry in BREAKING CHANGES.