Skip to content

Add UserNotifications Framework for iOS 10+ - #90

Closed
mrosberghaus wants to merge 11 commits into
delba:masterfrom
mrosberghaus:master
Closed

Add UserNotifications Framework for iOS 10+#90
mrosberghaus wants to merge 11 commits into
delba:masterfrom
mrosberghaus:master

Conversation

@mrosberghaus

Copy link
Copy Markdown

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.

@mrosberghaus

Copy link
Copy Markdown
Author

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

@mrosberghaus

Copy link
Copy Markdown
Author

@delba what's going on?

@pasevin

pasevin commented Feb 5, 2017

Copy link
Copy Markdown

Any chance this is going to be merged?

Comment thread Source/Permission.swift
}()

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

Copy link
Copy Markdown
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.

Comment thread Source/Permission.swift
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
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

Comment thread Source/Permission.swift
open static func notifications(types: UIUserNotificationType) -> Permission {
let settings = UIUserNotificationSettings(types: types, categories: nil)
let permission = Permission(type: .notifications(settings))
_notifications = permission

Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed

Comment thread Source/Permission.swift
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
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Again

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed

Comment thread Source/Permission.swift Outdated
/// The permission to send notifications.
@available(iOS 10.0, *)
open static func userNotifications(options: UNAuthorizationOptions) -> Permission {
return Permission(type: .userNotifications(options))

Copy link
Copy Markdown
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)

Comment thread Source/PermissionType.swift Outdated
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
//
#if PERMISSION_NOTIFICATIONS

Copy link
Copy Markdown
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

Comment thread Permission.podspec Outdated
Pod::Spec.new do |s|
s.name = "Permission"
s.version = "2.0.3"
s.version = "2.1.0"

Copy link
Copy Markdown
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)

Comment thread Permission.podspec Outdated

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

Copy link
Copy Markdown
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
Copy Markdown
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

delba commented Mar 5, 2017

Copy link
Copy Markdown
Owner

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

@mrosberghaus

Copy link
Copy Markdown
Author

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

@mrosberghaus mrosberghaus left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed

Comment thread Source/Permission.swift
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

@mrosberghaus

Copy link
Copy Markdown
Author

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

@mrosberghaus

Copy link
Copy Markdown
Author

@delba Hello?!?!?!

@otymartin

otymartin commented Jun 10, 2017

Copy link
Copy Markdown

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

@rob5408

rob5408 commented Jun 16, 2017

Copy link
Copy Markdown

^^ @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
Copy Markdown

@delba Please merge this PR

@pasevin

pasevin commented Jul 27, 2017

Copy link
Copy Markdown

Losing hope :/

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

Jeehut commented Sep 14, 2017

Copy link
Copy Markdown

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
Copy Markdown

@Dschee nice move 👍

@delba

delba commented Nov 12, 2019

Copy link
Copy Markdown
Owner

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.

7 participants