Fixes #2888: Use Dexter for requesting location permissions #2925

maskaravivek · 2019-04-24T18:15:12Z

Description (required)

ashishkumar468

Thanks for the PR, @maskaravivek. Works fine for me

neslihanturan

Thanks @maskaravivek , I have tested this. Tabs works fine but nearby doesn't load on API26 device.

neslihanturan · 2019-04-25T07:54:59Z

One more issue: if I send the app to background while nearby tab is selected, then disable the permission, when I come back to the app it does not request permission. Even more if I switch to contributions tab and come back to nearby tab, it still doesn't request for permission.

maskaravivek · 2019-04-25T08:10:45Z

Thanks @maskaravivek , I have tested this. Tabs works fine but nearby doesn't load on API26 device.

Is there any error in the logs?

neslihanturan · 2019-04-25T08:12:52Z

No, there is just an error about SettingsActivity which seems irrelevant.

neslihanturan · 2019-04-25T08:13:56Z

These are the logs I see after seeing loading screen for long time and switching back to contribution tab and come back to nearby tab:

2019-04-25 10:50:36.496 6366-6366/fr.free.nrw.commons.beta D/MainActivity: Contributions activity notifications menu item is visible
2019-04-25 10:50:38.478 1443-1492/? D/hwcomposer: hw_composer sent 50 syncs in 398s
2019-04-25 10:50:38.480 6366-6366/fr.free.nrw.commons.beta D/MainActivity: Nearby tab selected
2019-04-25 10:50:38.486 6366-6366/fr.free.nrw.commons.beta D/MainActivity: Contributions activity list sheet menu item is visible
2019-04-25 10:50:38.487 6366-6366/fr.free.nrw.commons.beta D/NearbyFragment: On nearby tab selected
2019-04-25 10:50:38.599 1537-1657/? D/AudioFlinger: mixer(0xa6b83480) throttle end: throttle time(5)

codecov-io · 2019-05-06T06:36:10Z

Codecov Report

Merging #2925 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2925      +/-   ##
=========================================
+ Coverage    3.69%   3.73%   +0.04%     
=========================================
  Files         249     249              
  Lines       12270   12135     -135     
  Branches     1090    1076      -14     
=========================================
  Hits          453     453              
+ Misses      11782   11647     -135     
  Partials       35      35

Impacted Files	Coverage Δ
...r/free/nrw/commons/contributions/MainActivity.java	`0% <ø> (ø)`	⬆️
...e/nrw/commons/location/LocationServiceManager.java	`0% <ø> (ø)`	⬆️
...ava/fr/free/nrw/commons/nearby/NearbyFragment.java	`0% <0%> (ø)`	⬆️
...ava/fr/free/nrw/commons/utils/PermissionUtils.java	`0% <0%> (ø)`	⬆️
...w/commons/contributions/ContributionsFragment.java	`0% <0%> (ø)`	⬆️
...fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.java	`0.84% <0%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67e43ef...e8d0c67. Read the comment docs.

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@maskaravivek you can click here to see the review status or cancel the code review job.

maskaravivek · 2019-06-04T18:28:38Z

@neslihanturan I have fixed the pending issues with this PR and its ready for review now. :)

PFA the attached video for various scenarios.

https://drive.google.com/file/d/1aCuegxzwD4NUY0f5xzyT_g0VQ1YkNuyh/view?usp=sharing

pullrequest

No issues found in the PR. Please consider the readability suggestions.

Reviewed with ❤️ by PullRequest

pullrequest · 2019-06-05T22:05:08Z

app/src/main/java/fr/free/nrw/commons/nearby/NearbyFragment.java

@@ -194,7 +195,7 @@ private void initBottomSheetBehaviour() {

            @Override
            public void onStateChanged(View bottomSheet, int newState) {


To let other collaborators know that newState is intentionally unused: rename to unusedNewState.

pullrequest · 2019-06-05T22:05:08Z

app/src/main/java/fr/free/nrw/commons/utils/PermissionUtils.java

@@ -120,6 +119,10 @@ public static void checkPermissionsAndPerformAction(Activity activity, String pe
                @Override
                public void onPermissionRationaleShouldBeShown(PermissionRequest permission,
                    PermissionToken token) {
+                    if (rationaleTitle == -1 && rationaleMessage == -1) {


It seems that you are trying to silently request permission here. The magic number of -1 is not clear from the caller's perspective.

Add function level comments in both checkPermissionsAndPerformAction that rationaleTitle and rationaleMessage can be invalid @StringRes and the meaning of -1, -1.

pullrequest · 2019-06-05T22:05:08Z

app/src/main/java/fr/free/nrw/commons/contributions/MainActivity.java

-         * @param isContributionsListFragment true when contribution fragment should be visible, false
-         *                                means user clicked to MediaDetails
-         */
-        private void updateContributionFragmentTabContent(boolean isContributionsListFragment) {


Is this change relevant to "Use Dexter for requesting location permissions"? If not, I think it would be better to be in a seperate PR.

ashishkumar468 approved these changes Apr 25, 2019

View reviewed changes

neslihanturan changed the title ~~Use dexter to ask for location permissions~~ Fixes #2888: First time ever tapping "Nearby": Tab does not show with dexter Apr 25, 2019

neslihanturan mentioned this pull request Apr 25, 2019

Fixes #2888 : First time ever tapping "Nearby": Tab does not show #2903

Closed

neslihanturan requested changes Apr 25, 2019

View reviewed changes

maskaravivek mentioned this pull request Apr 30, 2019

[WIP] changed confusing message about location permission(#2011) #2305

Closed

maskaravivek changed the title ~~Fixes #2888: First time ever tapping "Nearby": Tab does not show with dexter~~ Fixes #2888: Use Dexter for requesting location permissions Jun 3, 2019

pullrequest bot reviewed Jun 3, 2019

View reviewed changes

maskaravivek force-pushed the permissions branch from 7ebea53 to 283d308 Compare June 3, 2019 14:58

Use dexter to ask for location permissions

c19f855

maskaravivek force-pushed the permissions branch from 283d308 to c19f855 Compare June 4, 2019 17:40

pullrequest bot reviewed Jun 5, 2019

View reviewed changes

Addressed code review comments

e8d0c67

neslihanturan approved these changes Jun 8, 2019

View reviewed changes

neslihanturan merged commit 8971743 into commons-app:master Jun 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2888: Use Dexter for requesting location permissions #2925

Fixes #2888: Use Dexter for requesting location permissions #2925

maskaravivek commented Apr 24, 2019

ashishkumar468 left a comment

neslihanturan left a comment

neslihanturan commented Apr 25, 2019

maskaravivek commented Apr 25, 2019

neslihanturan commented Apr 25, 2019

neslihanturan commented Apr 25, 2019

codecov-io commented May 6, 2019 •

edited

Loading

pullrequest bot left a comment

maskaravivek commented Jun 4, 2019

pullrequest bot left a comment

pullrequest bot Jun 5, 2019

pullrequest bot Jun 5, 2019

pullrequest bot Jun 5, 2019

		@@ -194,7 +195,7 @@ private void initBottomSheetBehaviour() {

		@Override
		public void onStateChanged(View bottomSheet, int newState) {

Fixes #2888: Use Dexter for requesting location permissions #2925

Fixes #2888: Use Dexter for requesting location permissions #2925

Conversation

maskaravivek commented Apr 24, 2019

ashishkumar468 left a comment

Choose a reason for hiding this comment

neslihanturan left a comment

Choose a reason for hiding this comment

neslihanturan commented Apr 25, 2019

maskaravivek commented Apr 25, 2019

neslihanturan commented Apr 25, 2019

neslihanturan commented Apr 25, 2019

codecov-io commented May 6, 2019 • edited Loading

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

maskaravivek commented Jun 4, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

pullrequest bot Jun 5, 2019

Choose a reason for hiding this comment

codecov-io commented May 6, 2019 •

edited

Loading