-
Notifications
You must be signed in to change notification settings - Fork 12
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 iOS device accessibility settings to device properties. #214
Add iOS device accessibility settings to device properties. #214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for taking on this task to help us understand the usage
If I understand the two properties correctly, do they correspond to the two controls in the settings below?
I was trying to verify the new properties in Tracks Live View but couldn't seem to access them probably due to a data backend issue p1661755847008199-slack-C03DKP1JP. The API requests seem to send the expected properties, so we can verify them in Tracks later.
Re Known issue:
I also saw Core Data error messages in Xcode like this:
CoreData: annotation: reason : The model used to open the store is incompatible with the one used to create the store
But I also checked Wormholy that application_opened
(the first event) is always tracked despite the migration errors. I didn't test the case with unsent events before the migration, but it seems like another previous PR that adds a new device information property didn't do anything about it either.
__block NSString *preferredContentSizeCategory; | ||
|
||
dispatch_sync(dispatch_get_main_queue(), ^{ | ||
preferredContentSizeCategory = UIApplication.sharedIfAvailable.preferredContentSizeCategory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used Objective-C for a long while 😅 do you know if this pauses the return statement below until this line is called when it's not on the main thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been a long time for me as well. 😅
do you know if this pauses the return statement below until this line is called when it's not on the main thread?
Yes, as we dispatching synchronously using dispatch_sync
, this will wait before executing the following return
statement.
|
||
return preferredContentSizeCategory; | ||
#else // Mac | ||
return @"Unknown"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem to match the behavior in the comments for this method?
This will be null for Mac OS.
return @"Unknown"; | |
return nil; // or NULL in objc? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Done in bbb876a
Oh, it looks like that. I didn't realise this before. 😄 I added the |
Related to - woocommerce/woocommerce-ios#7576
Changes
In order to understand the accessibility settings of our users, this PR adds the following properties to the
deviceProperties
dictionary of analytics events.Known issue
As this PR adds two properties to the
Transformable
deviceInfo
(a dictionary) property of the core data model, we will face errors when trying to load stores created using previous versions of the core data model.This is handled here by deleting the old store and creating a new one using the latest core data model.
I hope that we have to use a
ValueTransformer
to deal with theTransformable
deviceInfo
property of the core data model. I am not sure whether this issue was ignored because,Transformable
dictionary (deviceInfo
,customProperties
, anduserProperties
) type properties is not supposed to happen often.Please correct me if I am wrong about this.
Testing instructions
Follow the testing instructions from woocommerce/woocommerce-ios#7576