Remove current location retrieval from upload process entirely #1644

misaochan · 2018-06-19T11:24:23Z

Temporary fix for #1599 . We are concerned that people who enabled the "automatically get current location" setting prior to the subtext change may not realize that their location is being added to the image's location template. This removes current location retrieval entirely from the upload process, and removes the Setting as well.

Where possible, I have left methods intact so that in the future if we decide to reimplement this (retrieving current location solely for category suggestions but not adding it to the template), we will not need to rewrite code. However, I have removed most of the method calls, and methods that would affect the user (e.g. the snackbar requesting location permissions).

As an added bonus, this should fix #1515 since the LocationListener will not be registered in ShareActivity.

Tested on Nexus S emulator running API 25 on betaDebug. To test:

Make sure GPS is on and your image has no geotagged coordinates
Upload that image
Check your uploaded image and make sure no location template was added to it

…nto 2.8-release-fork

codecov-io · 2018-06-19T11:51:13Z

Codecov Report

Merging #1644 into 2.8-release will increase coverage by 0.03%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##           2.8-release   #1644      +/-   ##
==============================================
+ Coverage         3.87%    3.9%   +0.03%     
==============================================
  Files              150     150              
  Lines             7571    7501      -70     
  Branches           710     703       -7     
==============================================
  Hits               293     293              
+ Misses            7261    7191      -70     
  Partials            17      17

Impacted Files	Coverage Δ
...java/fr/free/nrw/commons/upload/ShareActivity.java	`0% <ø> (ø)`	⬆️
...java/fr/free/nrw/commons/upload/FileProcessor.java	`0% <0%> (ø)`	⬆️
.../java/fr/free/nrw/commons/upload/GPSExtractor.java	`0% <0%> (ø)`	⬆️
...free/nrw/commons/upload/MultipleShareActivity.java	`0% <0%> (ø)`	⬆️

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 18b0517...b5161d6. Read the comment docs.

whym · 2018-06-20T09:32:17Z

Tested on real device (API 24) and it worked fine. An upload with no EXIF location info has no location template and an upload with EXIF location ifo has one.

whym · 2018-06-20T09:35:55Z

app/src/main/res/xml/preferences.xml

-        android:title="@string/preference_category_location">
-
-        <SwitchPreference
-            android:key="allowGps"


What do you think of the idea of leaving some trace of the key string "allowGps" so that future developers will not accidentally reuse it? (e.g. "// key 'allowGps' was used before - do not reuse it unless you revive the same feature.")

Sounds like a good plan, but where should we mention this?

I was thinking we could do that in preferences.xml in place of the removed code. Or maybe somewhere in the wiki pages?

Makes sense to me - added a comment there. :) Are we all good to merge?

whym

(I made a suggestion but it doesn't mean I'm disapproving this even if the suggestion is not applied.)

sivaraam · 2018-06-21T11:18:05Z

Where possible, I have left methods intact so that in the future if we decide to reimplement this (retrieving current location solely for category suggestions but not adding it to the template), we will not need to rewrite code

IMHO, it's better to remove functions that aren't used in the app as it would confuse the person who would be reading the related code in the future. Not many would expect dead code to be held back intentionally. It might also improve code hygiene. You could always get them back with a git revert when you do decide to re-use them rather than rewriting it (rewriting might be a better alternative if the code is very old).

Just for the sake of an example and nothing more, the Wikipedia app recently had a feature called "Offline library" which was actively developed until a month back or so. Since they discovered potential blockers for the feature they're shelving the feature for now. For that removed the relevant code completely rather than keeping it back.

nicolas-raoul · 2018-06-21T12:49:34Z

I also generally believe that unused code should be removed, to make the code smaller.

misaochan · 2018-06-21T12:57:21Z

Thanks for the feedback guys. Will do that soon. :)

misaochan · 2018-06-23T10:34:04Z

Removed unused code. :)

neslihanturan · 2018-06-28T16:37:12Z

@misaochan I have tested and get this error on beggining of app:

FATAL EXCEPTION: main Process: fr.free.nrw.commons.debug, PID: 8641 java.lang.RuntimeException: Unable to start activity ComponentInfo{fr.free.nrw.commons.debug/fr.free.nrw.commons.contributions.ContributionsActivity}: android.database.sqlite.SQLiteException: Can't downgrade database from version 7 to 6 at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2416) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476) at android.app.ActivityThread.-wrap11(ActivityThread.java) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344) at android.os.Handler.dispatchMessage(Handler.java:102) at android.os.Looper.loop(Looper.java:148) at android.app.ActivityThread.main(ActivityThread.java:5417) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616) Caused by: android.database.sqlite.SQLiteException: Can't downgrade database from version 7 to 6 at android.database.sqlite.SQLiteOpenHelper.onDowngrade(SQLiteOpenHelper.java:360) at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:254) at android.database.sqlite.SQLiteOpenHelper.getReadableDatabase(SQLiteOpenHelper.java:187) at fr.free.nrw.commons.contributions.ContributionsContentProvider.query(ContributionsContentProvider.java:52) at android.content.ContentProvider.query(ContentProvider.java:1017) at android.content.ContentProvider$Transport.query(ContentProvider.java:238) at android.content.ContentProviderClient.query(ContentProviderClient.java:136) at android.content.ContentProviderClient.query(ContentProviderClient.java:118) at fr.free.nrw.commons.contributions.ContributionDao.loadAllContributions(ContributionDao.java:50) at fr.free.nrw.commons.contributions.ContributionsActivity.onAuthCookieAcquired(ContributionsActivity.java:116) at fr.free.nrw.commons.auth.AuthenticatedActivity.requestAuthToken(AuthenticatedActivity.java:26) at fr.free.nrw.commons.contributions.ContributionsActivity.onCreate(ContributionsActivity.java:140) at android.app.Activity.performCreate(Activity.java:6237) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1107) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2369) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476) at android.app.ActivityThread.-wrap11(ActivityThread.java) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344) at android.os.Handler.dispatchMessage(Handler.java:102) at android.os.Looper.loop(Looper.java:148) at android.app.ActivityThread.main(ActivityThread.java:5417) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616) 06-28 19:35:25.542 1645-1973/system_process W/ActivityManager: Force finishing activity fr.free.nrw.commons.debug/fr.free.nrw.commons.contributions.ContributionsActivity

neslihanturan · 2018-06-28T16:39:16Z

Re-installing the app solves the crash.

misaochan · 2018-06-28T17:16:50Z

Hmm. It is still a legit error, but doesn't appear to be related to this PR - I haven't modified any code in the Contributions package at all. Should we create a new issue for it? Might be a bit difficult to reproduce though, haha.

neslihanturan · 2018-06-28T19:04:00Z

If you have an old installation on an emulator, can you try to pull your changes there and test? I think the error will be appeared then.

I didn't experienced this error on other branches.

misaochan · 2018-06-30T16:29:59Z

@neslihanturan I did as you described and could not reproduce the error. I do think it's very unlikely to be caused by this PR as none of the classes in the stack trace were modified. I would suggest merging if it otherwise works for you, and if the issue reoccurs when we test the branch, we can fix it. Either way, I don't think the fix should be in this PR, but rather a separate one.

Edit: It seems according to https://stackoverflow.com/questions/24053786/cant-downgrade-database-from-version-2-to-1-even-after-fresh-install-and-re-run that the cause could be code on the previous version of the app erroneously incrementing the DB version. So that explains why I wasn't able to reproduce it, since I would likely have used a different "old installation" from yours. Which branch were you testing prior to this one, do you remember?

whym · 2018-07-06T09:32:34Z

It appears that the DB version was changed in the 'browse' branch from 6 to 7: ae09445#diff-1c0d8106634cdf0a1f9d0087c46c3e9d So I think we are safe to ignore it in this particular PR.

The DB downgrade issue is tracked in #1589.

whym · 2018-07-06T09:33:51Z

I tested it again with a betaRelease build and it still worked fine for me. :)

misaochan · 2018-07-07T15:14:44Z

Thanks for testing again, @whym . :) Would anyone like to go ahead and merge?

maskaravivek · 2018-07-07T16:37:12Z

app/src/main/res/xml/preferences.xml

-            android:summary="@string/allow_gps_summary" />
-
-    </PreferenceCategory>
+    <!-- The key 'allowGps' was used before and has since been removed based on the discussion at #1599.


Nice that you added this comment here. :)

misaochan added 8 commits June 12, 2018 19:59

Merge remote-tracking branch 'refs/remotes/commons-app/2.8-release' i…

a2b1f5b

…nto 2.8-release-fork

Merge remote-tracking branch 'refs/remotes/commons-app/2.8-release' i…

5be4d17

…nto 2.8-release-fork

Always return false for gpsPreferenceEnabled()

0b0c01d

Always return null if image has no EXIF data

371580b

Add TODO

e8925f5

Do not display Snackbar to get location permissions

fa238b9

No need to unregisterLocationManager() as it should never be registered

801fea6

Remove "automatically get current location" Setting entirely

b5c74c3

misaochan requested review from whym and maskaravivek June 19, 2018 11:29

This was referenced Jun 19, 2018

Items needed for v2.8 release #1478

Closed

Update "automatically get current location" setting text to inform that files will be geotagged with it #1599

Closed

whym reviewed Jun 20, 2018

View reviewed changes

whym approved these changes Jun 20, 2018

View reviewed changes

Added comment in place of allowGps key

97d63ce

misaochan added 10 commits June 23, 2018 19:53

Remove unused methods in GPSExtractor

df7a944

Remove unnecessary context var

927ceb9

Remove context in other constructor

0fd77a1

Fix other GPSExtractor vars, logs and imports

7d3e3ae

Remove LocationListener from GPSExtractor

0b58e28

Remove boolean useGPS entirely

fe3dda1

Remove prefs var in GPSExtractor

be5c054

Optimize FileProcessor

4a5257c

Remove snackbar from ShareActivity

4340d65

Tidy onRequestPermissionsResult

798bcba

misaochan added 2 commits June 23, 2018 20:11

Optimize imports in ShareActivity

3ab3b02

Remove location constant from ShareActivity

b5161d6

ilgazer mentioned this pull request Jul 2, 2018

Implementation of in-app EXIF modification #1686

Open

maskaravivek reviewed Jul 7, 2018

View reviewed changes

maskaravivek merged commit 2d831c0 into commons-app:2.8-release Jul 7, 2018

misaochan deleted the fix-automatic-location branch July 8, 2018 17:58

nicolas-raoul mentioned this pull request Aug 3, 2018

Nearby edits didn't edit Wikidata items #1601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove current location retrieval from upload process entirely #1644

Remove current location retrieval from upload process entirely #1644

misaochan commented Jun 19, 2018

codecov-io commented Jun 19, 2018 •

edited

Loading

whym commented Jun 20, 2018

whym Jun 20, 2018

misaochan Jun 20, 2018

whym Jun 20, 2018

misaochan Jun 21, 2018

whym left a comment

sivaraam commented Jun 21, 2018 •

edited

Loading

nicolas-raoul commented Jun 21, 2018

misaochan commented Jun 21, 2018

misaochan commented Jun 23, 2018

neslihanturan commented Jun 28, 2018

neslihanturan commented Jun 28, 2018

misaochan commented Jun 28, 2018

neslihanturan commented Jun 28, 2018 •

edited

Loading

misaochan commented Jun 30, 2018 •

edited

Loading

whym commented Jul 6, 2018

whym commented Jul 6, 2018

misaochan commented Jul 7, 2018

maskaravivek Jul 7, 2018

Remove current location retrieval from upload process entirely #1644

Remove current location retrieval from upload process entirely #1644

Conversation

misaochan commented Jun 19, 2018

codecov-io commented Jun 19, 2018 • edited Loading

Codecov Report

whym commented Jun 20, 2018

whym Jun 20, 2018

Choose a reason for hiding this comment

misaochan Jun 20, 2018

Choose a reason for hiding this comment

whym Jun 20, 2018

Choose a reason for hiding this comment

misaochan Jun 21, 2018

Choose a reason for hiding this comment

whym left a comment

Choose a reason for hiding this comment

sivaraam commented Jun 21, 2018 • edited Loading

nicolas-raoul commented Jun 21, 2018

misaochan commented Jun 21, 2018

misaochan commented Jun 23, 2018

neslihanturan commented Jun 28, 2018

neslihanturan commented Jun 28, 2018

misaochan commented Jun 28, 2018

neslihanturan commented Jun 28, 2018 • edited Loading

misaochan commented Jun 30, 2018 • edited Loading

whym commented Jul 6, 2018

whym commented Jul 6, 2018

misaochan commented Jul 7, 2018

maskaravivek Jul 7, 2018

Choose a reason for hiding this comment

codecov-io commented Jun 19, 2018 •

edited

Loading

sivaraam commented Jun 21, 2018 •

edited

Loading

neslihanturan commented Jun 28, 2018 •

edited

Loading

misaochan commented Jun 30, 2018 •

edited

Loading