Location Question Type #478

QuintilesRK · 2015-09-24T16:07:27Z

This pull request adds support for a new location question and answer type. It can be used as a single question step and/or in a form task. You may have multiple entries in the form task and while interacting with each individual form item, the map will hide or show depending on your current selection. The location question handles the permission request for accessing the devices current location, when using the current location button. It will reverse geocode the current location value to an address and fill it in the question answer field.

Includes: Location Question Location Question Answer Format Location Question Form Cell Location Survey Answer

* master: (114 commits) ResearchKit Project: add explicit 'ORK_LOG_LEVEL_WARNING=1' ORKImageSelectionView: resolve merge conflict ORKTest: fix column width not being updated after rotation ORKImageCaptureStepViewController: restore 'queue_' method prefix ORKTappingContentView: slight constraint refactor ORKSkin: hide 'ORKGetMetricForScreenType()' function; use 'ORKGetMetricForWindow()' instead Misc: remove extra newlines before 'else' Misc: remove additional space after '!' unary operator Misc: homegenize whitespace between consecutive closing and opening brackets ORKTaskViewController: whitespace Misc: homogenize for the more common 'cannot' instead of 'can not' in exception messages Misc: whitespace between '==' operator Misc: some additional constraint homogeneization of recently merged active tasks Misc: remove needless empty class extensions Misc: use 'has' instead of 'have' when refering to singular noun ORKFormItemCell: use VerticalMargin and HorizontalMargin const names Misc: remove space between '!' unary operator and operand Misc: use property syntax for accessing 'font', 'ascender', 'descender', and 'window' Misc: use property syntax for 'allValues' and 'allKeys' ORKDataLogger: use previous configuration file key for backwards compatibility ...

umerkhan-apple · 2015-09-26T01:11:31Z

ResearchKit/Common/ORKAnswerFormat.m

+
+#pragma mark - ORKLocationAnswerFormat
+
+@implementation ORKLocationAnswerFormat


There is a warning that init has not been implemented. To dismiss the warning, go to ORKAnswerFormat_Internal.h and remove line 67 (marked below).

rsanchezsaez · 2015-09-26T17:47:51Z

Also, although its layout looks ok, the location step logs unsatisfiable constraint warnings when shown. It'd be nice to fix those.

rsanchezsaez · 2015-09-26T18:13:36Z

In ORKCatalog, sort the Location Question alphabetically within its section.

When doing this, make sure you reorder all the related code in ORKCatalog (represented task type, description text, task-generating code, etc.) accordingly, because we're trying to keep the code in the same order as the items appear in the list, as detailed here.

umerkhan-apple · 2015-09-28T17:29:08Z

ResearchKit/Common/ORKAnswerFormat.m

+            isValid = false;
+        } else if (longitude.doubleValue < -180.0 || longitude.doubleValue > 180.0) {
+            isValid = false;
+        }


Change all the above isValid = false to isValid = NO.

I don't think we need to translate the coordinates into a string and then validated them here. We can just take the coordinates and validate them in isAnswerValid instead of validating them in isAnswerValidWithString:.

Marxon13 · 2015-10-19T12:25:43Z

@YuanZhu-apple The changes are ready for review.

YuanZhu-apple · 2015-10-19T17:13:02Z

Testing/ORKTest/ORKTest/ORKESerialization.m

+  ENTRY(ORKLocationAnswerFormat,
+        ^id(NSDictionary *dict, ORKESerializationPropertyGetter getter) {
+            ORKLocationAnswerFormat *format = [[ORKLocationAnswerFormat alloc] init];
+            format.useCurrentLocation = ((NSNumber *)GETPROP(dict, useCurrentLocation)).boolValue;


This line is unnecessary, since useCurrentLocation can be written after init.

@YuanZhu-apple Removed unnecessary setting of userCurrentLocation.

YuanZhu-apple · 2015-10-19T17:42:41Z

ResearchKit/Common/ORKLocationSelectionView.m

+    _mapViewHeightConstraint = [NSLayoutConstraint constraintWithItem:_mapView attribute:NSLayoutAttributeHeight relatedBy:NSLayoutRelationEqual toItem:nil attribute:NSLayoutAttributeNotAnAttribute multiplier:1.0 constant:ORKGetMetricForWindow(ORKScreenMetricLocationQuestionMapHeight, self.window)];
+    [constraints addObject:_mapViewHeightConstraint];
+
+    [NSLayoutConstraint activateConstraints:constraints];


showMapViewIfNecessary can be called again and again, more and more constraints are added?

Once this method is called, the mapView will have a super view, and the method will return on its second line. The map view is not an external property, so there is no way it can be removed from outside of the class, and it is never removed internally. Also once these constraints are set, they are never going to change since the map view is never going to be hidden once open. Do you want me to switch to using a boolean to keep track of wether or not this is called, or hold on to the constraints with an array and check its count?

Thanks for the explanation! It's good to keep it like this.

Marxon13 · 2015-10-20T12:41:53Z

@YuanZhu-apple Everything is ready for you to take another look at.

YuanZhu-apple · 2015-10-20T20:48:09Z

typedef enum {
   kCLErrorLocationUnknown  = 0,
   kCLErrorDenied,
   kCLErrorNetwork,
   kCLErrorHeadingFailure,
   kCLErrorRegionMonitoringDenied,
   kCLErrorRegionMonitoringFailure,
   kCLErrorRegionMonitoringSetupDelayed,
   kCLErrorRegionMonitoringResponseDelayed,
   kCLErrorGeocodeFoundNoResult,
   kCLErrorGeocodeFoundPartialResult,
   kCLErrorGeocodeCanceled,
   kCLErrorDeferredFailed,
   kCLErrorDeferredNotUpdatingLocation,
   kCLErrorDeferredAccuracyTooLow,
   kCLErrorDeferredDistanceFiltered,
   kCLErrorDeferredCanceled,
   kCLErrorRangingUnavailable,
   kCLErrorRangingFailure,
} CLError;

Here is a list of all the possible errors from core location. Now all the error codes were untranslated, for example, when the WiFi is disabled(no internet connection), the error alert shows

A more useful message should be displayed, like "Resolving address requires internet connection, please turn on your WiFi to answer this question. Skip this question if skip button is available, or come back to shi survey when you are back on internet".

I'm suggesting categorize the error codes into sub categories, and display different message to the user.

Marxon13 · 2015-10-21T13:01:30Z

@YuanZhu-apple Error handling has been updated. I've categorized the errors that would cause issues with the functionality of the location question. I've grouped other errors (like region monitoring) into an unknown error case. I'm debating whether or not those should even be displayed to the user, as it has nothing to do with the functionality of the question.

YuanZhu-apple · 2015-10-21T18:58:41Z

ResearchKit/Localized/en.lproj/Localizable.strings

+/* Alerts for invalid location coordinates */
+"LOCATION_QUESTION_PLACEHOLDER" = "Enter an address";
+"LOCATION_ERROR_TITLE" = "Failed to Resolve Specified Address";
+


Newly added strings are not here yet.

YuanZhu-apple · 2015-10-21T20:41:08Z

Generally I think there are two kinds of message can be displayed to user:

Internet is not available, ask user to "turn on WiFi / try next time / skip"
User's input cannot be resolved, ask user to correct their input
Please correct me if there are more messages can give guidance to user.

And for other types of errors, we need to handle them either internally within the view; or throw it out to taskViewController's delegate and let developer to handle.

…ot apply to the functionality of the selection view by silently absorbing the error.

Marxon13 · 2015-10-22T12:32:24Z

@YuanZhu-apple I added the localized strings I forgot, as well as internally absorbing the errors from the location manager that do not affect the functionality.

…nto LocationQuestion Conflicts: ResearchKit/Common/ORKResult.m

YuanZhu-apple · 2015-10-22T18:14:42Z

Testing/ORKTest/ORKTest/ORKESerialization.m

+            return [[ORKLocationAnswerFormat alloc] init];
+        },
+        (@{
+          PROPERTY(useCurrentLocation, NSNumber, NSObject, NO, nil, nil)


Change writeAfterInit from NO to YES, it will fix the unit test failure in ORKTest

@YuanZhu-apple Fixed issue causing tests to fail.

YuanZhu-apple · 2015-10-23T23:48:33Z

Great, good to go.

Location Question Type

Eric Rolf added 4 commits September 22, 2015 18:16

Initial commit for location question.

956549c

Includes: Location Question Location Question Answer Format Location Question Form Cell Location Survey Answer

Added Location Question to ORKTest.

7847c81

Updated Attributions.

be6a922

umerkhan-apple reviewed Sep 26, 2015
View reviewed changes

umerkhan-apple reviewed Sep 28, 2015
View reviewed changes

YuanZhu-apple reviewed Oct 19, 2015
View reviewed changes

Removed unnecessary lines.

5070517

YuanZhu-apple reviewed Oct 19, 2015
View reviewed changes

Marxon13 added 3 commits October 20, 2015 08:37

Pulled edge to edge presentation logic into init.

58453f5

Removed if statement left over from debugging.

0018387

Removed location question from form example.

cbf9d72

Updated error handling.

e7ca832

YuanZhu-apple reviewed Oct 21, 2015
View reviewed changes

Marxon13 added 3 commits October 22, 2015 08:24

Forgot to add localizable strings.

5e77684

Handle errors that can be returned by the CLLocationManager that do n…

224146f

…ot apply to the functionality of the selection view by silently absorbing the error.

Removed unused error message.

0a07f24

Marxon13 added 2 commits October 22, 2015 12:16

Merge branch 'master' of https://github.com/ResearchKit/ResearchKit i…

1f26d5c

…nto LocationQuestion Conflicts: ResearchKit/Common/ORKResult.m

Accidentally removed required import on merge.

dd42f47

YuanZhu-apple reviewed Oct 22, 2015
View reviewed changes

Fixed issue causing tests to fail.

1b5ea88

YuanZhu-apple added a commit that referenced this pull request Oct 23, 2015

Merge pull request #478 from QuintilesRK/LocationQuestion

160b7c5

Location Question Type

YuanZhu-apple merged commit 160b7c5 into ResearchKit:master Oct 23, 2015

YuanZhu-apple modified the milestone: Next Release Oct 28, 2015

YuanZhu-apple added the new feature label Oct 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Location Question Type #478

Location Question Type #478

QuintilesRK commented Sep 24, 2015

umerkhan-apple Sep 26, 2015

rsanchezsaez commented Sep 26, 2015

rsanchezsaez commented Sep 26, 2015

umerkhan-apple Sep 28, 2015

umerkhan-apple Sep 28, 2015

Marxon13 commented Oct 19, 2015

YuanZhu-apple Oct 19, 2015

Marxon13 Oct 19, 2015

YuanZhu-apple Oct 19, 2015

Marxon13 Oct 20, 2015

YuanZhu-apple Oct 20, 2015

Marxon13 commented Oct 20, 2015

YuanZhu-apple commented Oct 20, 2015

Marxon13 commented Oct 21, 2015

YuanZhu-apple Oct 21, 2015

YuanZhu-apple commented Oct 21, 2015

Marxon13 commented Oct 22, 2015

YuanZhu-apple Oct 22, 2015

Marxon13 Oct 22, 2015

YuanZhu-apple commented Oct 23, 2015


		#pragma mark - ORKLocationAnswerFormat

		@implementation ORKLocationAnswerFormat

Location Question Type #478

Location Question Type #478

Conversation

QuintilesRK commented Sep 24, 2015

Choose a reason for hiding this comment

rsanchezsaez commented Sep 26, 2015

rsanchezsaez commented Sep 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marxon13 commented Oct 19, 2015

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

Marxon13 commented Oct 20, 2015

YuanZhu-apple commented Oct 20, 2015

Marxon13 commented Oct 21, 2015

Choose a reason for hiding this comment

YuanZhu-apple commented Oct 21, 2015

Marxon13 commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuanZhu-apple commented Oct 23, 2015