Skip to content

Feature/permission settings#199

Merged
thias15 merged 32 commits into
ob-f:masterfrom
dhruv2295:feature/permissionSettings
Jun 2, 2021
Merged

Feature/permission settings#199
thias15 merged 32 commits into
ob-f:masterfrom
dhruv2295:feature/permissionSettings

Conversation

@dhruv2295

Copy link
Copy Markdown
Contributor

No description provided.

if (camera.isChecked()) startInstalledAppDetailsActivity(requireActivity());
else PermissionUtils.requestCameraPermission(requireActivity());
else {
if (PermissionUtils.shouldShowRational(requireActivity(), Manifest.permission.CAMERA))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a constant for that.

Comment thread android/app/build.gradle Outdated
apply plugin: 'com.android.application'
apply plugin: 'de.undercouch.download'
apply plugin: 'com.google.firebase.crashlytics'
apply plugin: 'kotlin-android'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this needed for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

integrated some kotlin code, but then decided against it, so not really needed. i'll remove

== PackageManager.PERMISSION_GRANTED) {
setupCamera();
} else if (shouldShowRequestPermissionRationale(Constants.PERMISSION_CAMERA)) {
PermissionUtils.showCameraPermissionsControllerToast(requireActivity());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should show a toast for the camera permission, not for all the controller permissions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  public static void showCameraPermissionsControllerToast(Activity activity) {
    Toast.makeText(
            activity.getApplicationContext(),
            R.string.camera_permission_denied + " " + R.string.permission_reason_stream_video,
            Toast.LENGTH_LONG)
        .show();
  }

this is the definition of the function. just one toast.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but since it comes from camera fragment, we don't know if the permission was needed for the controller or something else. As a matter of fact it is probably used for previewing the camera feed isn't it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, so you want to change the message? if so, can you share some content?

if (isGranted) {
setupCamera();
} else {
PermissionUtils.showCameraPermissionsControllerToast(requireActivity());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it CameraPermissionsController?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because toast message is same

PermissionUtils.showCameraPermissionsControllerToast(requireActivity());
}
break;
case PERMISSION_STORAGE:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed in controller fragment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a master permission handler, to be accessed anywhere in app extending this class

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm talking about the storage permission. The controller does not need storage permission afaik.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was using it as a global thing, so that if some other screen needs it, it could be used. but it's not used right now, so i've removed it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand if it's a general function, but here it is just for the ControllerFragment. It would be nice, if it was not required to have this function in each fragment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so i've removed it

Comment thread android/app/src/main/java/org/openbot/common/ControlsFragment.java Outdated
@thias15 thias15 linked an issue May 28, 2021 that may be closed by this pull request
@thias15 thias15 merged commit 9d0f6e4 into ob-f:master Jun 2, 2021
@dhruv2295 dhruv2295 deleted the feature/permissionSettings branch September 13, 2021 07:10
lacykaltgr pushed a commit to satinavrobotics/SatiBot that referenced this pull request Jun 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean up permission handling

2 participants