Add option to configure Tracks to store its DB in Application Support instead of Documents by mokagio · Pull Request #147 · Automattic/Automattic-Tracks-iOS

mokagio · 2020-09-23T05:34:39Z

See #146.

~~This is just a test to verify my internal non-sandboxed macOS app works fine when using Application Support.~~

Update: we decided to merge this change, but in a way that wouldn't change the behavior of Simplenote macOS.

To test these changes:

I added unit tests 🙂, although CI is failing 🙃, but not because of my tests 🙂
Checkout this PR in Simplenote macOS Update Tracks to version with support for non-sandboxed apps simplenote-macos#683

mokagio

I refined the implementation to avoid changing the way existing consumers of the library store the DB.

@jleandroperez @jkmassel can I ask you for a new round of review?

mokagio · 2020-10-01T02:23:29Z

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.h

+@property (nonnull, nonatomic, strong) NSManagedObjectContext *managedObjectContext;
+@property (nonnull, nonatomic, strong) NSManagedObjectModel *managedObjectModel;
+@property (nonnull, nonatomic, strong) NSPersistentStoreCoordinator *persistentStoreCoordinator;


I had to add the annotation here too to avoid warnings. It makes the diff a bit noisier but it's good to have it anyways.

The implementation of all those getters makes sure they are not nil, or rather, fails if they are, so nonnullable seemed appropriate.

Yup that's the perfect approach Gio!

mokagio · 2020-10-01T02:26:14Z

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.h

+/// the app is running and where to save the store.
+///
+/// The `super` `init` method defaults to `false` for backward compatibility.
+- (nonnull instancetype)initWithSandboxedMode:(BOOL)sandboxed NS_DESIGNATED_INITIALIZER;


I wonder whether the parameter should be an enum instead:

_ = TracksContextManager(storeLocation: .applicationSupport) // or _ = TracksContextManager(storeLocation: .documents)

I'm on the fence and might sit overnight. Regardless, because the changes doesn't affect existent consumers thanks to the init override below, I think it's okay to merge this as it is and then I can follow up with a refactor if we think it's worth it.

To be entirely honest, I would keep the initializer simple, and just switch to the Application Support folder.

Something in the lines of:

If a Documents folder already exists, use it

Otherwise, check if a folder in Applications Support exists, and use it

If none of the above, create a new folder in Applications Support

WYDT?

I'm ok with either approach. I like how pretty the enum is, but I like how much work @jleandroperez's isn't 😄

mokagio · 2020-10-01T02:26:58Z

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.m

+
+- (instancetype)initWithSandboxedMode:(BOOL)sandboxed {
+    self = [super init];
+    if (!self) { return nil; }


This should never happen so I defined the initializer as nonnullable. In Swift land, that would be the case.

Standard ObjC, exactly!

jleandroperez

@mokagio sir!! Few changes suggested:

Initializer fix (would prevent leaking memory)
ApplicationSupport API simplification (not required, not a blocker either!!)

As per the initializer detail, 10000% up to you!

Taking that parameter isn't a problem at all
It can be solved without specifying if we're sandboxed or not, as well. Yet another alternative, is to just always use the Application Support path, and migrate legacy databases, if needed

I would take the easiest path. As usual 😄

jleandroperez · 2020-10-01T18:04:13Z

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.m

 @implementation TracksContextManager

+- (instancetype)init {
+    return [[TracksContextManager alloc] initWithSandboxedMode:true];


This initializer returning a new instance, rather than the one being initialized.

You would typically instantiate a Context Manager like this:

TracksContextManager *manager = [[TracksContextManager alloc] init];

alloc reserves memory for the object

init is expected to initialize the instance

In this case, init is alloc'ating more memory, and returning yet another instance.

Thank you for this! Gosh 😳 I'm really rusty at writing Objective-C 🤦

I felt like there was something wrong with that, but it compiled and the tests passed so I shrugged it off.

Interestingly, Xcode also gives me a warning about it

It either came after I wrote the code and moved on or after I added the NS_DESIGNATED_INITIALIZER annotation. Regardless, I shouldn't have missed it... 😳

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.m

jleandroperez · 2020-10-01T18:12:41Z

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.m

+        abort();
+    }
+
+    return applicationSupportURL;


You can proooobably simplify this API as follows:

NSURL *appSupportURL = [[[NSFileManager defaultManager] URLsForDirectory:NSApplicationSupportDirectory inDomains:NSUserDomainMask] lastObject];

Ref. here

(The Application Support path is expected to always be there!)

mokagio · 2020-10-02T00:58:39Z

Thanks @jleandroperez for the great review!

Regarding the init, I like the idea of keeping it simple and eventually migrate everything to Application Support.

I haven't tried, but I think that even to just check whether a file exists in the Documents folder from a non-sandboxed app, macOS would request the user for permission. If that's the case, then we'd be back to square one, because the real aim of this change is avoid that behavior in non-sandboxed apps.

I really like the migration idea, but I think it needs more experimenting and might be best for a dedicated PR.

@jleandroperez

Thanks @jleandroperez! See #147 (comment)

@jleandroperez

Thanks @jleandroperez See #147 (comment)

jleandroperez

@mokagio One tiny comment sent away (apologies I missed that!!).

Other than that, please when ready!!

jleandroperez · 2020-10-02T13:44:29Z

Automattic-Tracks-iOS/Model/Core Data/TracksContextManager.m

+        return [self applicationSupportURLForContainerApp];
+    } else {
+        return [self applicationDocumentsDirectory];
+    }


@mokagio Is it possible this logic is inverted? (sandboxed > Documents Directory?).

Otherwise non sandboxed apps would still trigger the Documents Folder alert perhaps?

You are totally right!

I didn't notice because the tests were inverted, too 🤦‍♂️

@jleandroperez

Thanks @jleandroperez! See #147 (comment)

@jleandroperez

Thanks @jleandroperez See #147 (comment)

mokagio · 2020-10-05T09:27:37Z

😫 I feel like I'm in a deadlock... There's some inconsistent issue with the tests, which doesn't happen with the Xcode 12, but I can't upgrade to Xcode 12 because the pod validation fails.

Inconsistent as in this:

@jleandroperez

Thanks @jleandroperez! See #147 (comment)

@jleandroperez

Thanks @jleandroperez See #147 (comment)

@jleandroperez

Thanks @jleandroperez! See #147 (comment)

@jleandroperez

Thanks @jleandroperez See #147 (comment)

@jleandroperez

Thanks @jleandroperez! See #147 (comment)

@jleandroperez

Thanks @jleandroperez See #147 (comment)

This will allow us to add the conditional logic for the sandbox vs non-sandbox location later on.

We'll need this later to make the custom init with the sandbox mode flag non-nullable.

@jleandroperez

Thanks @jleandroperez! See #147 (comment)

@jleandroperez

Thanks @jleandroperez See #147 (comment)

@jleandroperez

This should have always been the case, but somehow I got confused and didn't realize my mistake. Thanks @jleandroperez for picking this up! Goes to show that tests are only good when written properly! https://github.com/Automattic/Automattic-Tracks-iOS/pull/147/files/3e38df8087e7b38a76b45acc5e89527746b4145b#r498830462

jleandroperez

sir!!

jleandroperez · 2020-10-09T01:08:33Z

For the record, Cocoapods had issues finding the Sodium-Fork framework. Workaround involved running bundle exec pod repo update

Thank you Gio!!

mokagio force-pushed the store-data-in-application-support branch from 09c9afb to cedd0a5 Compare September 23, 2020 05:35

mokagio mentioned this pull request Sep 23, 2020

Where to store SQLite DB in non-sandboxed apps? #146

Closed

mokagio marked this pull request as ready for review September 30, 2020 23:47

mokagio requested review from jkmassel and jleandroperez September 30, 2020 23:47

mokagio commented Oct 1, 2020

View reviewed changes

mokagio mentioned this pull request Oct 1, 2020

Update Tracks to version with support for non-sandboxed apps Automattic/simplenote-macos#683

Closed

mokagio changed the title ~~Store Tracks.sqlite store in Application Support instead of Documents~~ Add option to configure Tracks to store its DB in Application Support instead of Documents Oct 1, 2020

jleandroperez suggested changes Oct 1, 2020

View reviewed changes

mokagio requested a review from jleandroperez October 2, 2020 00:58

mokagio added a commit that referenced this pull request Oct 2, 2020

Call self in TracksContextManager convenience initializer

8c035b0

Thanks @jleandroperez! See #147 (comment)

mokagio added a commit that referenced this pull request Oct 2, 2020

Simplify code to get URL of Application Support folder

3e38df8

Thanks @jleandroperez See #147 (comment)

jleandroperez approved these changes Oct 2, 2020

View reviewed changes

mokagio force-pushed the store-data-in-application-support branch from fcfec20 to aeb61db Compare October 5, 2020 04:19

mokagio added a commit that referenced this pull request Oct 5, 2020

Call self in TracksContextManager convenience initializer

87d0522

Thanks @jleandroperez! See #147 (comment)

mokagio added a commit that referenced this pull request Oct 5, 2020

Simplify code to get URL of Application Support folder

5f0330e

Thanks @jleandroperez See #147 (comment)

mokagio added a commit that referenced this pull request Oct 8, 2020

Call self in TracksContextManager convenience initializer

40d2e9f

Thanks @jleandroperez! See #147 (comment)

mokagio added a commit that referenced this pull request Oct 8, 2020

Simplify code to get URL of Application Support folder

a75bfbd

Thanks @jleandroperez See #147 (comment)

mokagio force-pushed the store-data-in-application-support branch from aeb61db to 9eb9300 Compare October 8, 2020 05:47

mokagio added a commit that referenced this pull request Oct 8, 2020

Call self in TracksContextManager convenience initializer

ee51c10

Thanks @jleandroperez! See #147 (comment)

mokagio added a commit that referenced this pull request Oct 8, 2020

Simplify code to get URL of Application Support folder

ce12329

Thanks @jleandroperez See #147 (comment)

mokagio force-pushed the store-data-in-application-support branch from 9eb9300 to a074e1f Compare October 8, 2020 08:34

mokagio added a commit that referenced this pull request Oct 8, 2020

Call self in TracksContextManager convenience initializer

df19c46

Thanks @jleandroperez! See #147 (comment)

mokagio added a commit that referenced this pull request Oct 8, 2020

Simplify code to get URL of Application Support folder

ac7ca74

Thanks @jleandroperez See #147 (comment)

mokagio force-pushed the store-data-in-application-support branch from a074e1f to 61a05f6 Compare October 8, 2020 08:43

mokagio added 3 commits October 9, 2020 11:35

Add logic to get URL for Application Support folder

5466b8b

Store Tracks.sqlite store in Application Support instead of Documents

f7ad4a9

Add test for TracksContexManager store URL location

eed19a8

mokagio added 6 commits October 9, 2020 11:35

Refactor: extract context manager store URL definition

d8da20f

This will allow us to add the conditional logic for the sandbox vs non-sandbox location later on.

Add nullability annotation to TracksContextManager properties

03a742d

We'll need this later to make the custom init with the sandbox mode flag non-nullable.

Allow TracksContextManager consumers to configure the store location

3f981b8

Call self in TracksContextManager convenience initializer

547374e

Thanks @jleandroperez! See #147 (comment)

Simplify code to get URL of Application Support folder

4c36c39

Thanks @jleandroperez See #147 (comment)

mokagio force-pushed the store-data-in-application-support branch from 61a05f6 to 9d50fde Compare October 9, 2020 00:36

jleandroperez approved these changes Oct 9, 2020

View reviewed changes

mokagio merged commit 6cf038e into develop Oct 9, 2020

mokagio deleted the store-data-in-application-support branch October 9, 2020 01:26

jkmassel mentioned this pull request Aug 26, 2021

Add support for Swift Package Manager #186

Merged

Conversation

mokagio commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Oct 2, 2020

Uh oh!

jleandroperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Oct 5, 2020

Uh oh!

jleandroperez left a comment

Choose a reason for hiding this comment

Uh oh!

jleandroperez commented Oct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mokagio commented Sep 23, 2020 •

edited

Loading