Skip to content

Fixes #1545 Top crashes on Play Store for v2.7.1 [A and E] #1672

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

Conversation

neslihanturan
Copy link
Collaborator

Description

Fixes A and E from #1545 (I hope). It saves file which is being uploaded to a temp location, and uses Uri of that file as reference during upload process, instead of using Uri that content provider supported.

Tests performed (required)

Tested on API 23, beta debug. Tested actions: share photo from another application, share multiple photos from another application and contributing via contributions list.

misaochan and others added 30 commits May 2, 2018 20:17
It feels like the template for reporting bugs is a bit redundant - we have summary, steps to reproduce, observed behaviour AND expected behaviour, which all are very similar. I think that by paring it down to the essentials, people will be less likely to skip steps.
…efresh if fetching fails" (commons-app#1609)

* Revert "Modify allow_gps_summary string (commons-app#1608)"

This reverts commit 3b6c94c.

* Revert "Fix issue commons-app#1332 : Contribution Image auto/manual refresh if fetching fails"

This reverts commit 5aba745.
* Versioning for v2.7.2

* Update changelog.md
* Fixfor#1479

* Issue fixed

* IssueSolved
* Added Basic Pic Of the Day App Widget

* Added Java Lib for XML to JSON

* Added missing json library from xml to json

* Undone formatting

* Consolidate the networking libraries - drop volley in favor of OkHttp

* Extracted a few networking related items into a new Dagger module and finished the process of mocking the main component for tests.

* Refactoring to extract GpsCategoryModel and ensure single-responsibility-principle is maintained in CategoryApi.

* Updated PicOfDayAppWidget class to parse HTML

* fixed featured image back bug

* Localisation updates from https://translatewiki.net.

* Javadocs added

* Add option to set image as wallpaper (commons-app#1535)

* Add option to set image as wallpaper

* Added java docs

* Toast message on setting the wallpaper successfully

* Localisation updates from https://translatewiki.net.

* Add dependencies to com.android.support.test.rules and runner

Needed for ActivityTestRule used in SettingsActivityTest

* Added Basic Pic Of the Day App Widget

* Added Java Lib for XML to JSON

* Added missing json library from xml to json

* Undone formatting

* Updated PicOfDayAppWidget class to parse HTML
…l save the file to a internal path and it will be deleted by another method after upload process is done.
@neslihanturan
Copy link
Collaborator Author

Hi all, currently camera bug is solved bud I couldn't find the reason behind undeletable reference to previously uploaded file.

On you first upload everything is fine. Our file will be uploaded gets saved to ExternalStorage/1_tmp . Then we delete that file. File.delete() returns true. File becomes invisible on file system (manually checked). On our second upload, we save our new file to ExternalStorage/1_tmp on Share Activity background and behind progress bar on ContributionsListFragment we somehow see previously uploaded file (that we deleted already).

If we don't delete files and continue as 1_tmp, 2_tmp... right images becomes visible during upload process. So our problem is deleting and overriding to same file name. It somehow preserves a reference to old file.

Did you ever experienced such situation?

Uri uri = ContributionUtils.saveFileBeingUploadedTemporarily(getActivity(), data.getData());
controller.handleImagePicked(requestCode, uri, false, null);
if (requestCode == ContributionController.SELECT_FROM_CAMERA) {
controller.handleImagePicked(requestCode, null, false, null);
Copy link
Member

Choose a reason for hiding this comment

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

@neslihanturan Why is the uri being passed as null for camera clicks?

Copy link
Member

Choose a reason for hiding this comment

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

Also, please update the handleImagePicked method signature annotating this param as @Nullable :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is null because, data coming from camera intent is already null. Instead, photo gets saved to a directory and its path will be get from intent extras on subsequent steps.

@@ -96,7 +95,7 @@ public void cleanup() {
* @param decimalCoords the coordinates in decimal. (e.g. "37.51136|-77.602615")
* @param onComplete the progress tracker
*/
public void startUpload(String title, Uri mediaUri, String description, String mimeType, String source, String decimalCoords, String wikiDataEntityId, ContributionUploadProgress onComplete) {
public void startUpload(String title, Uri contentProviderUri, Uri mediaUri, String description, String mimeType, String source, String decimalCoords, String wikiDataEntityId, ContributionUploadProgress onComplete) {
Copy link
Member

Choose a reason for hiding this comment

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

Please update the javadocs to reflect the change

@@ -148,7 +148,9 @@ public void startUpload(final Contribution contribution, final ContributionUploa
protected Contribution doInBackground(Void... voids /* stare into you */) {
long length;
ContentResolver contentResolver = context.getContentResolver();
try {
/*try {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain by adding javadocs to why this block has been commented out?

* @param contentProviderUri
*/
public static void removeTemporaryFile(Context context, Uri tempFileUri, Uri contentProviderUri) {
//TODO: do I have to notify file system about deletion?
Copy link
Member

Choose a reason for hiding this comment

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

Am not sure about notifying to the file system but our contributionDao should be updating the table if it has a corresponding tuple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only delete temp file here, after contribution is uploaded. If we delete contribution from contributionDao, then our successful contribution will be removed from contributions table. Then it won't be visible on contribution list.

@@ -279,6 +280,9 @@ public void onCreate(Bundle savedInstanceState) {
contribution = savedInstanceState.getParcelable("contribution");
}

//receiveImageIntent();
Copy link
Member

Choose a reason for hiding this comment

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

Can you add javadocs to explain why this has been commented out?

@misaochan
Copy link
Member

Pinging @whym and @psh just in case they know a handy solution to the wrong reference. :)

@neslihanturan
Copy link
Collaborator Author

Here is the demo, I tried to display what is going wrong: https://mega.nz/#!bM4xDJ6K!C2Dd4cfNM5DzJVcnrCT2GspeopqUxLV_z4oBb-5rwbs

@neslihanturan
Copy link
Collaborator Author

Help still wanted. This problem postpones our release. I hope some of you experienced such issue before:(

@neslihanturan neslihanturan changed the base branch from master to 2.8-release July 12, 2018 15:30
@codecov-io
Copy link

codecov-io commented Jul 12, 2018

Codecov Report

Merging #1672 into 2.8-release will increase coverage by <.01%.
The diff coverage is 4.06%.

Impacted file tree graph

@@              Coverage Diff               @@
##           2.8-release   #1672      +/-   ##
==============================================
+ Coverage         3.89%    3.9%   +<.01%     
==============================================
  Files              151     154       +3     
  Lines             7521    7630     +109     
  Branches           705     721      +16     
==============================================
+ Hits               293     298       +5     
- Misses            7211    7314     +103     
- Partials            17      18       +1
Impacted Files Coverage Δ
...c/main/java/fr/free/nrw/commons/AboutActivity.java 0% <ø> (ø) ⬆️
...ain/java/fr/free/nrw/commons/utils/ImageUtils.java 0% <ø> (ø) ⬆️
...ain/java/fr/free/nrw/commons/upload/FileUtils.java 3.22% <ø> (ø) ⬆️
...ree/nrw/commons/contributions/ContributionDao.java 0% <ø> (ø) ⬆️
...fr/free/nrw/commons/media/MediaDetailFragment.java 0% <ø> (ø) ⬆️
...ee/nrw/commons/media/MediaDetailPagerFragment.java 0% <ø> (ø) ⬆️
...rw/commons/upload/DetectUnwantedPicturesAsync.java 0% <ø> (ø) ⬆️
.../free/nrw/commons/di/CommonsApplicationModule.java 50% <ø> (ø) ⬆️
...ava/fr/free/nrw/commons/nearby/NearbyActivity.java 0% <ø> (ø) ⬆️
...e/nrw/commons/category/CategoryImagesActivity.java 0% <ø> (ø) ⬆️
... and 22 more

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 2d831c0...bbb9a87. Read the comment docs.

@maskaravivek
Copy link
Member

@neslihanturan Is help still wanted or were you able to figure it out with the random file name approach?

@neslihanturan
Copy link
Collaborator Author

@maskaravivek , no everything should be fine now. Solved 🎆 💃 . But Let me test all upload scenarios in 3 hours, than it will be ready for your tests.

@neslihanturan
Copy link
Collaborator Author

Ready for test! @maskaravivek

@misaochan
Copy link
Member

Hi @neslihanturan , I tried testing this but unfortunately I didn't get very far! :(

  1. Could you rebase onto 2.8-release branch, please? It seems to have many unrelated file changes, perhaps from master?

  2. Tested on API 24 emulator. Immediately upon loading, the app crashes with the logs below. This happens for both betaDebug and prodDebug, even after uninstalling and doing a fresh install.

07-16 17:46:07.425 29450-29450/fr.free.nrw.commons.debug E/AndroidRuntime: FATAL EXCEPTION: main
    Process: fr.free.nrw.commons.debug, PID: 29450
    java.lang.RuntimeException: Unable to create application fr.free.nrw.commons.CommonsApplication: java.lang.NullPointerException: Attempt to get length of null array
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:5364)
        at android.app.ActivityThread.-wrap2(ActivityThread.java)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1528)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:154)
        at android.app.ActivityThread.main(ActivityThread.java:6077)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:866)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:756)
     Caused by: java.lang.NullPointerException: Attempt to get length of null array
        at fr.free.nrw.commons.utils.ContributionUtils.emptyTemporaryDirectory(ContributionUtils.java:83)
        at fr.free.nrw.commons.CommonsApplication.onCreate(CommonsApplication.java:85)
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1024)
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:5361)
        at android.app.ActivityThread.-wrap2(ActivityThread.java) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1528) 
        at android.os.Handler.dispatchMessage(Handler.java:102) 
        at android.os.Looper.loop(Looper.java:154) 
        at android.app.ActivityThread.main(ActivityThread.java:6077) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:866) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:756) 

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.