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

Add UserNotifications Framework for iOS 10+ #90

Closed
wants to merge 11 commits into from
Closed

Add UserNotifications Framework for iOS 10+ #90

wants to merge 11 commits into from

Conversation

mrgrauel
Copy link

I added a permission for the new notification framework UserNotifications in iOS 10 to request permission with the new api.

  • add permission UserNotification
  • add configuration PERMISSION_USERNOTIFICATIONS
  • add permissionType userNotifications
  • removed the file private static variable _notification cause it's never read
  • did not change the requestedNotifications default cause it returns the same information

I tried to keep it in the structure you designed the framework.

@mrgrauel
Copy link
Author

On my local machine the tests do run. Does anybody has an idea why the checks fail?

@mrgrauel
Copy link
Author

@delba what's going on?

@pasevin
Copy link

pasevin commented Feb 5, 2017

Any chance this is going to be merged?

@@ -98,31 +116,22 @@ open class Permission: NSObject {
return Permission(type: .notifications(settings))
}()

/// Variable used to retain the notifications permission.
fileprivate static var _notifications: Permission?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is used to retain the permission as stated in the comment above.

/// The permission to send notifications.
open static func notifications(types: UIUserNotificationType, categories: Set<UIUserNotificationCategory>?) -> Permission {
let settings = UIUserNotificationSettings(types: types, categories: categories)
let permission = Permission(type: .notifications(settings))
_notifications = permission
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_notifications is used to retain the permission

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

}

/// The permission to send notifications.
open static func notifications(types: UIUserNotificationType) -> Permission {
let settings = UIUserNotificationSettings(types: types, categories: nil)
let permission = Permission(type: .notifications(settings))
_notifications = permission
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

}

/// The permission to send notifications.
open static func notifications(categories: Set<UIUserNotificationCategory>?) -> Permission {
let settings = UIUserNotificationSettings(types: [.badge, .sound, .alert], categories: categories)
let permission = Permission(type: .notifications(settings))
_notifications = permission
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

/// The permission to send notifications.
@available(iOS 10.0, *)
open static func userNotifications(options: UNAuthorizationOptions) -> Permission {
return Permission(type: .userNotifications(options))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to retain the permission with a private var like in the case of notifications (e.g: _userNotifications)

@@ -21,6 +21,9 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
//
#if PERMISSION_NOTIFICATIONS
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be PERMISSION_USERNOTIFICATIONS

@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = "Permission"
s.version = "2.0.3"
s.version = "2.1.0"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to change it just before pushing it (and it's dependent on the git tag)

@@ -64,6 +64,11 @@ Pod::Spec.new do |s|
no.pod_target_xcconfig = { "SWIFT_ACTIVE_COMPILATION_CONDITIONS" => "PERMISSION_NOTIFICATIONS" }
end

s.subspec 'UserNotifications' do |no|
no.dependency 'Permission/Core'
no.pod_target_xcconfig = { "SWIFT_ACTIVE_COMPILATION_CONDITIONS" => "PERMISSION_USERNOTIFICATIONS" }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename PERMISSION_USERNOTIFICATIONS to PERMISSION_USER_NOTIFICATIONS for the sake of consistency please?

import UserNotifications

internal extension Permission {
var statusUserNotifications: PermissionStatus {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the structure of the Motion permission (that also uses "synchronous" calls) please ?

@delba
Copy link
Owner

delba commented Mar 5, 2017

Hi @mrgrauel, thanks a lot for the PR! There are a just few changes to do before merging though.
Thanks!

@mrgrauel
Copy link
Author

mrgrauel commented Mar 6, 2017

Good Morning @delba, is it now as you want it?

Copy link
Author

@mrgrauel mrgrauel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

/// The permission to send notifications.
open static func notifications(types: UIUserNotificationType, categories: Set<UIUserNotificationCategory>?) -> Permission {
let settings = UIUserNotificationSettings(types: types, categories: categories)
let permission = Permission(type: .notifications(settings))
_notifications = permission
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@mrgrauel
Copy link
Author

mrgrauel commented Apr 5, 2017

@delba I changed everything. I dunno why it still says that a change request is open.

@mrgrauel
Copy link
Author

mrgrauel commented May 3, 2017

@delba Hello?!?!?!

@otymartin
Copy link

otymartin commented Jun 10, 2017

@delba Has this issue been fixed? I still get these xcode 10 deprecation warnings. It wont compile with target iOS 10

@rob5408
Copy link

rob5408 commented Jun 16, 2017

^^ @delba Sorry about that, I thought I was on my fork. I know it doesn't actually merge the PR, but why does GitHub even give me the option. :\

@zhuorantan
Copy link

@delba Please merge this PR

@pasevin
Copy link

pasevin commented Jul 27, 2017

Losing hope :/

@Jeehut Jeehut mentioned this pull request Sep 14, 2017
2 tasks
@Jeehut
Copy link

Jeehut commented Sep 14, 2017

Hey everybody, I've given up on waiting for @delba to answer and merge our PRs. Instead I've integrated this PR into my fork (which I created for #94). I've also created my own CocoaPods entry so we can continue using this framework and merging our improvements together. Feel free to star and send your PRs to my fork in the future. I will try to review them from time to time:
https://github.com/Dschee/Permission

To switch to my fork in your project simply replace pod 'Permission' with pod 'Dschee-Permission' if you are using CocoaPods or github "delba/Permission" with github "Dschee/Permission" if your are using Carthage. I've also made a new release: v2.1.0.

@otymartin
Copy link

@Dschee nice move 👍

@delba
Copy link
Owner

delba commented Nov 12, 2019

Hi @mrgrauel, the project has been updated to the latest Swift version and now uses UserNotifications (https://github.com/delba/Permission/blob/master/Source/Types/Notifications.swift).

@delba delba closed this Nov 12, 2019
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

7 participants