Fix permissions w/out moving them into separate files #1

alanscarpa · 2016-11-18T18:59:45Z

This is an attempt to fix issue: nickoneill#194
It is modeled after this solution used by a similar framework: https://github.com/iosphere/ISHPermissionKit/pull/80/files

In short, the problem is that the framework imports all libraries necessary for all permissions it supports (camera, location, bluetooth, etc), and any app using this library will get automatically rejected if they do not provide ALL plist descriptions - even for the permissions they are not requesting within their app.

The solution is to hack around with subspecs to define flags that will signal the pod which libraries to import.

The TODO found in this PR illustrates a potential problem with this solution and also the reason why a project using this pod won't build. Swift does not allow you to put #if #endif around switch cases. This is a documented bug: https://bugs.swift.org/browse/SR-2

A workaround for this, proposed by Ying is:

struct ThingDetail {
    var description = ""
    var permissions: String?
}

var thingDetailOne = ThingDetail(description: "one", permissions: "")
var thingDetailTwo = ThingDetail(description: "two", permissions: "")

enum Thing {
    case one
    #if ENABLE_TWO
    case two
    #endif

    var thingDetail: ThingDetail {
        if self == .one {
            return thingDetailOne
        }
        #if ENABLE_TWO
        if self == .two {
            return thingDetailTwo
        }
        #endif
        return ThingDetail()
    }
}

Another solution is to break each permission into it's own framework. This morning the main repo owner replied to me and said that this would be the right way to go.

Ying supports going the "ThingDetail" struct route. But after hearing from the repo owner, I think trying to break this into separate frameworks should be the route - as this would be the one he would like to merge in the end.

andrewdolce · 2016-11-21T19:08:29Z

PermissionScope/PermissionScope.swift

@@ -61,19 +85,6 @@ typealias resultsForConfigClosure     = ([PermissionResult]) -> Void
    public let contentView = UIView()

    // MARK: - Various lazy managers


Don't need this // MARK anymore.

changed in new pr

andrewdolce · 2016-11-21T19:23:47Z

PermissionScope/PermissionScope.swift

@@ -1003,7 +1075,8 @@ typealias resultsForConfigClosure     = ([PermissionResult]) -> Void
        onCancel = cancelled

        DispatchQueue.main.async {
-            while self.waitingForBluetooth || self.waitingForMotion { }
+            while
+                self.waitingForBluetooth || self.waitingForMotion { }


A few questions here:

Is this the preferred indentation? (Not necessarily opposed, just wondering why it was changed.)

Is this busy-waiting on the main thread? I realize this is existing code, but it still seems strange to me. Should we be concerned about this?

looks like that indent was my mistake.

Yea, it is strange looking at it now. Is it also saying that if waitingForBluetooth is true, execute the blank { } I wonder what the point of that is? The next line self.requiredAuthorized({ areAuthorized gets called regardless, right?

If I'm understanding it correctly, it will continue looping (and doing nothing) until both the waiting vars are false.

andrewdolce · 2016-11-21T19:29:32Z

PermissionScope/Structs.swift

@@ -22,6 +53,8 @@ import Foundation
    }

    public var description: String {
+        /* TODO: This will not compile when used in a project (unless project is asking ALL permissions) because any permission that is not used will have it's enum undefined due to the #ifendif statements around the enum definition above.
+        */


Since this doesn't compile, is the goal to address that issue before merging this PR? Or merge as-is? I usually think it's best to avoid merging into master with commits that don't compile, since that can cause headaches for things like git bisect.

Also do we have a workaround for this? The multiple if pattern that Ying suggested?

This PR will compile itself. But if you were to use this pod in a project, that project would not compile.

Currently working on the if pattern.

andrewdolce · 2016-11-21T19:39:30Z

PermissionScope.podspec

+    rem.pod_target_xcconfig  = feature_flags
+    rem.user_target_xcconfig = feature_flags
+  end
+


Cool setup. 👍

Have we tested yet with mixing/matching the subspecs to make sure that the flags are combined correctly?

I wonder if there's a terser, DRY-er way to define the subspecs without duplicating the stuff they have in common, but maybe that'd be too fancy.

Good point. I wonder if i can find something.

I have tested that installing pod PermissionScope/Motion sets the correct flag, if that's what you mean.

I mostly meant checking that we can build with 2 of the subspecs at the same time and get both flags, without them interfering with each other somehow.

Yea, I just installed multiple subspecs and all flags were set correctly.

alanscarpa force-pushed the alan/fixPermissions branch 2 times, most recently from 12c1bdf to 87b8502 Compare November 21, 2016 19:02

Organize permissions

8f92414

alanscarpa force-pushed the alan/fixPermissions branch from 87b8502 to 8f92414 Compare November 21, 2016 19:02

andrewdolce reviewed Nov 21, 2016

View reviewed changes

alanscarpa changed the title ~~Fix permissions~~ Fix permissions w/out moving them into separate files Nov 23, 2016

alanscarpa closed this Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix permissions w/out moving them into separate files #1

Fix permissions w/out moving them into separate files #1

alanscarpa commented Nov 18, 2016

andrewdolce Nov 21, 2016

alanscarpa Nov 21, 2016

andrewdolce Nov 21, 2016

alanscarpa Nov 21, 2016

andrewdolce Nov 21, 2016

andrewdolce Nov 21, 2016 •

edited

Loading

alanscarpa Nov 21, 2016

andrewdolce Nov 21, 2016

alanscarpa Nov 21, 2016

alanscarpa Nov 21, 2016

andrewdolce Nov 21, 2016

alanscarpa Nov 21, 2016

		@@ -61,19 +85,6 @@ typealias resultsForConfigClosure = ([PermissionResult]) -> Void
		public let contentView = UIView()

		// MARK: - Various lazy managers

Fix permissions w/out moving them into separate files #1

Fix permissions w/out moving them into separate files #1

Conversation

alanscarpa commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdolce Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdolce Nov 21, 2016 •

edited

Loading