Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement direct uploads for Nearby Map #1060

Merged

Conversation

misaochan
Copy link
Member

@misaochan misaochan commented Jan 12, 2018

I decided to submit this PR after getting the Nearby Map direct uploads for #252 sorted, and will work on the Nearby List direct uploads in a separate PR. This is aimed at allowing @neslihanturan to start work on updating the user's real-time position in the Nearby Map once this PR has been merged, while minimizing conflicts. I have tested this on an emulator running API 25.

What this PR does:

  • Hooks up the camera and gallery buttons in Nearby Map to their respective uploads
  • Checks and prompts for permissions as needed
  • Saves the title and desc of the Nearby item selected, and autofills the title and desc text fields in ShareActivity respectively
  • Applies the Fix issue with Nearby item descriptions #1034 fixes to this branch (as the Nearby code has changed drastically in this branch)

What this PR does NOT cover (and will thus need to be handled in our future PRs):

  • Hooking up camera/gallery buttons in Nearby List
  • Category suggestions based on Nearby item categories
  • Editing Wikidata P18 property
  • Fixing memory leaks

…loads' into nearby-uploads

# Conflicts:
#	app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java
@commons-app commons-app deleted a comment Jan 24, 2018
@commons-app commons-app deleted a comment Jan 24, 2018
@commons-app commons-app deleted a comment Jan 24, 2018
@misaochan
Copy link
Member Author

@psh @maskaravivek I've refactored the PR to use Dagger injections for saving the title/desc, please let me know if the injections are done correctly (I'm very new to Dagger, haha). I must say that using Dagger really cleans up all the shuffling of SharedPreferences between fragments and activities, which is very nice.

I've tested this and it works for me on API 25 and 21 emulators. But please let me know if you're still getting the NPEs @neslihanturan .

@neslihanturan
Copy link
Collaborator

Thanks @misaochan :)
I successfully uploaded 2 images, however after upload, while coming back to the NearbyMapFragment I get this error:
FATAL EXCEPTION: main Process: fr.free.nrw.commons.debug, PID: 22175 java.lang.IllegalStateException: Fragment NearbyMapFragment{4262a250} not attached to Activity at android.support.v4.app.Fragment.startActivityForResult(Fragment.java:1026) at android.support.v4.app.Fragment.startActivityForResult(Fragment.java:1017) at fr.free.nrw.commons.contributions.ContributionController.startGalleryPick(ContributionController.java:81) at fr.free.nrw.commons.nearby.DirectUpload.initiateGalleryUpload(DirectUpload.java:74) at fr.free.nrw.commons.nearby.NearbyMapFragment.lambda$passInfoToSheet$11$NearbyMapFragment(NearbyMapFragment.java:420) at fr.free.nrw.commons.nearby.NearbyMapFragment$$Lambda$10.onClick(Unknown Source) at android.view.View.performClick(View.java:4463) at android.view.View$PerformClick.run(View.java:18770) at android.os.Handler.handleCallback(Handler.java:808) at android.os.Handler.dispatchMessage(Handler.java:103) at android.os.Looper.loop(Looper.java:193) at android.app.ActivityThread.main(ActivityThread.java:5292) at java.lang.reflect.Method.invokeNative(Native Method) at java.lang.reflect.Method.invoke(Method.java:515) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:824) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:640) at dalvik.system.NativeStart.main(Native Method)

@misaochan
Copy link
Member Author

Hope that fixes it!

@commons-app commons-app deleted a comment Jan 24, 2018
@neslihanturan
Copy link
Collaborator

I couldn't reproduce the error. I think it is fixed and all uploads were successful:) Great job @misaochan . I will just wait @psh and @maskaravivek review the changes before merging.

@misaochan
Copy link
Member Author

Any thoughts on the Dagger additions? Would be good if we could merge this soon, so that @neslihanturan can start working on real-time user positions in the Nearby map with minimal conflicts.

Copy link
Member

@maskaravivek maskaravivek left a comment

Choose a reason for hiding this comment

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

Dagger usage looks perfectly fine to me. I would like to have it merged. :)

@neslihanturan neslihanturan merged commit 7a25f34 into commons-app:directNearbyUploads Feb 5, 2018
@misaochan misaochan deleted the nearby-uploads branch February 5, 2018 10:31
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.

4 participants