Ask for permission only when needed #272

PerrineAguiar · 2017-05-09T13:27:42Z

Fixes #270.

mcopejans · 2017-05-11T08:12:33Z

...reamer/app/src/main/java/eu/intermodalics/tango_ros_streamer/activities/RunningActivity.java

+                getTangoPermission(EXTRA_VALUE_ADF, REQUEST_CODE_ADF_PERMISSION);
+            if (!mParameterNode.getStringParam(getString(R.string.dataset_datasets_path_key)).isEmpty() &&
+                    !mParameterNode.getStringParam(getString(R.string.dataset_uuid_key)).isEmpty())
+                getTangoPermission(EXTRA_VALUE_DATASET, REQUEST_CODE_DATASET_PERMISSION);


Isn't it required to get Tango permissions before connecting to Tango (SERVICE_CONNECTED)?

The documentation is not really clear about it but I think not, they are required only for specific Tango API calls like: TangoService_getAreaDescriptionUUIDList, TangoService_getAreaDescriptionMetadata, TangoService_saveAreaDescription or TangoService_Experimental_getDatasetUUIDs.

What we could also do is, each time we do one of this call from the C++ code, somehow trigger this intent in the Java code. I think it could be possible using a ros topic.

Tango Connect returns TANGO_NO_DATASET_PERMISSION if permissions are not granted for recording a dataset BEFORE connecting.

See: https://github.com/googlesamples/tango-examples-c/blob/master/tango_client_api/include/tango_client_api.h#L76

The documentation is very clear: https://github.com/googlesamples/tango-examples-c/blob/master/tango_client_api/include/tango_client_api.h#L676

…ure/ask_from_permission_only_when_needed Conflicts: TangoRosStreamer/app/src/main/java/eu/intermodalics/tango_ros_streamer/activities/RunningActivity.java

…ure/ask_from_permission_only_when_needed

PerrineAguiar · 2017-05-19T15:33:57Z

@mcopejans Thanks for your comment. I changed the code to trigger the permission intent from c++ using the tango status topic, ptal.

mcopejans · 2017-05-29T14:15:09Z

tango_ros_common/tango_ros_native/src/tango_ros_node.cpp

+  UpdateAndPublishTangoStatus(TangoStatus::NEED_TO_REQUEST_ADF_PERMISSION);
+  while (tango_status_ != TangoStatus::ADF_PERMISSION_REQUEST_ANSWERED) {
+    ros::Duration(0.1).sleep();
+  }


Is it possible to implement this using a service call? Sleeping does not look like an elegant solution. Why did you choose not to request permissions before connecting on the Java side.

I chose to trigger permission request from the C++ side because this is where we know exactly when and which permission we need. For example, if the user reconnect to Tango using the ros command line tool, the Java side does not know that a service call to /tango/connect has been made.
Here we are using the /tango/status topic to trigger the permission request from C++ to Java, and know when the request has been answered.

Is abusing the /tango/status the best way to do this? Why not using a ros service call between the java side (server) and the c++ side (client)?

…ure/ask_from_permission_only_when_needed Conflicts: TangoRosStreamer/app/src/main/java/eu/intermodalics/tango_ros_streamer/activities/RunningActivity.java tango_ros_common/tango_ros_common/src/main/java/eu/intermodalics/tango_ros_common/ParameterNode.java

PerrineAguiar · 2017-06-01T12:50:21Z

@mcopejans @smits Thanks for your comments.
I implemented a service to request permissions as suggested, ptal.

mcopejans

The code looks good, but I added one comment to improve the app flow. For this feature, I think it is useful to have a schema in mind that illustrates what happens when each permission is granted vs. not granted.

mcopejans · 2017-06-02T09:59:31Z

tango_ros_common/tango_ros_native/src/tango_ros_node.cpp

+    if (srv.response.granted) {
+      LOG(INFO) << "Dataset permission has been requested and granted by the user.";
+    } else {
+      LOG(WARNING) << "Dataset permission has been requested but not granted by the user.";


If I understand correctly, a permission that was not granted will lead to a connection failure? I would resend the request in this case or force the user to go back to the settings screen. The app cannot continue in any other way.

I think it will indeed fail to connect if we try to load/record dataset while dataset premission is not granted. I would like to test it but I did not find a proper way to revoke the dataset permission.

…verNode api

PerrineAguiar · 2017-06-08T16:16:59Z

@mcopejans Thanks for the review.

I think the schema for ADF and dataset permission is as follow:

ADF permission is needed only when saving an ADF or accessing its metadata. In our case it means that if the user does not grant ADF permission, he will not be able to save a map or see the list of available maps.
Dataset permission is needed when loading/recording dataset. If the user does not grant dataset permission while loading or recording a dataset, connection to Tango will fail.

mcopejans · 2017-07-31T15:18:47Z

Jenkins failed with the following error, is this expected:

ros_common/tango_ros_native/include/tango_ros_native/tango_ros_node.h:43:50: fatal error: tango_ros_messages/RequestPermission.h: No such file or directory
 #include <tango_ros_messages/RequestPermission.h>

mcopejans · 2017-07-31T15:19:33Z

test this please

PerrineAguiar · 2017-07-31T15:51:52Z

@mcopejans Yes, I think one line is missing here.
It should be:

RequestPermission
RequestPermissionRequest
RequestPermissionResponse

mcopejans · 2017-08-01T08:07:31Z

Yay, one point for Jenkins :)

meyerj · 2018-02-20T17:31:52Z

retest this please

Ask for permission only when needed

917fbe9

PerrineAguiar added the in progress label May 9, 2017

PerrineAguiar requested a review from mcopejans May 9, 2017 13:28

mcopejans reviewed May 11, 2017

View reviewed changes

Perrine Aguiar added 3 commits May 11, 2017 11:27

Merge branch 'master' of github.com:Intermodalics/tango_ros into feat…

3ea1f08

…ure/ask_from_permission_only_when_needed Conflicts: TangoRosStreamer/app/src/main/java/eu/intermodalics/tango_ros_streamer/activities/RunningActivity.java

Ask permissions from c++ code

781f0ce

Merge branch 'master' of github.com:Intermodalics/tango_ros into feat…

dfba9c7

…ure/ask_from_permission_only_when_needed

mcopejans suggested changes May 29, 2017

View reviewed changes

Perrine Aguiar added 5 commits June 1, 2017 12:53

Request permission via a service call

5eb8305

Rename tango service client node

ffd5df5

Undo renaming and add new service server class instead

e92a371

Clean up code

fee85db

mcopejans approved these changes Jun 2, 2017

View reviewed changes

Perrine Aguiar added 3 commits June 8, 2017 15:22

No need to request ADF permission at connection time

784f772

Apply recent changes of TangoServiceClientNode api to TangoServiceSer…

e235a09

…verNode api

Correct error when setting default datasets path

cdbdb0a

Add missing header

8d36998

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ask for permission only when needed #272

Ask for permission only when needed #272

PerrineAguiar commented May 9, 2017

mcopejans May 11, 2017

PerrineAguiar May 11, 2017

mcopejans May 11, 2017 •

edited

Loading

PerrineAguiar commented May 19, 2017

mcopejans May 29, 2017

PerrineAguiar May 30, 2017

smits May 30, 2017

PerrineAguiar commented Jun 1, 2017 •

edited

Loading

mcopejans left a comment

mcopejans Jun 2, 2017

PerrineAguiar Jun 8, 2017

PerrineAguiar commented Jun 8, 2017

mcopejans commented Jul 31, 2017

mcopejans commented Jul 31, 2017

PerrineAguiar commented Jul 31, 2017

mcopejans commented Aug 1, 2017

meyerj commented Feb 20, 2018

Ask for permission only when needed #272

Are you sure you want to change the base?

Ask for permission only when needed #272

Conversation

PerrineAguiar commented May 9, 2017

mcopejans May 11, 2017

Choose a reason for hiding this comment

PerrineAguiar May 11, 2017

Choose a reason for hiding this comment

mcopejans May 11, 2017 • edited Loading

Choose a reason for hiding this comment

PerrineAguiar commented May 19, 2017

mcopejans May 29, 2017

Choose a reason for hiding this comment

PerrineAguiar May 30, 2017

Choose a reason for hiding this comment

smits May 30, 2017

Choose a reason for hiding this comment

PerrineAguiar commented Jun 1, 2017 • edited Loading

mcopejans left a comment

Choose a reason for hiding this comment

mcopejans Jun 2, 2017

Choose a reason for hiding this comment

PerrineAguiar Jun 8, 2017

Choose a reason for hiding this comment

PerrineAguiar commented Jun 8, 2017

mcopejans commented Jul 31, 2017

mcopejans commented Jul 31, 2017

PerrineAguiar commented Jul 31, 2017

mcopejans commented Aug 1, 2017

meyerj commented Feb 20, 2018

mcopejans May 11, 2017 •

edited

Loading

PerrineAguiar commented Jun 1, 2017 •

edited

Loading