-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix issue #5814 - Rotated pictures shown as failed to upload, actually uploaded successfully #5899
base: main
Are you sure you want to change the base?
Conversation
…ase access, the access failed, causing a crash at the end of the upload, which made normal testing impossible. The UploadWorker has been fixed to ensure that database access occurs on the correct thread.
…ure to obtain storage read and write permissions. On my phone, this bug manifests as a crash when attempting to store the rotated image (I tested this on both my emulator and my real phone; Fix 1 addresses the emulator, and Fix 2 is for the real phone). The issue described in the report could not be reproduced, and I suspect this is due to the storage permission problem manifesting differently on different emulators/phones. In any case, the two situations mentioned in the issue no longer occur. I added code to request permissions in the `getRotatedImage` function of `EditActivity.kt` to resolve this problem.
Personally, I think the issue described was caused by not obtaining read-write permissions, which led to a specific situation where the program may have skipped the permission check during the testing environment at the time. This could have caused the upload to succeed but still show as failed. I was able to partially reproduce the bug: I believe that in the testing environment when the issue was raised, the program forcefully bypassed the error for some reason, which led to the second bug described in the issue. This error should have caused the crash. |
@@ -239,6 +245,12 @@ class EditActivity : AppCompatActivity() { | |||
* as a result, and finishes the current activity. | |||
*/ | |||
fun getRotatedImage() { | |||
//Get Permission to saccess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
//Get Permission to saccess | ||
if (ContextCompat.checkSelfPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE) | ||
!= PackageManager.PERMISSION_GRANTED) { | ||
ActivityCompat.requestPermissions(this, arrayOf(Manifest.permission.WRITE_EXTERNAL_STORAGE), 7747) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding a comment explaining what 7747 is, or maybe use constants rather than integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok ! Actually it's just a randomly choice code, cause it should be avoided to be repeat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the documentation correctly, onRequestPermissionsResult has to use the same integer? Do we have a onRequestPermissionsResult method somewhere that should use this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently not, but I'm not sure if anyone will use it in the future.
This code(Integer) is a necessary parameter for requestPermissions to run.
Since there have been no instances of request failures in my testing environment so far, there isn't a situation that handle request failures about it in onRequestPermissionsResult (It always support to work)
I am not sure what the program should do in the event of a request failure. If only add something like this:
Toast.makeText(context, "Fail to request permission for access picture", Toast.LENGTH_SHORT).show()
When it fail is fine, then I can add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a failure check/toast.
Would you mind making the integer final static as seen at https://github.com/raulhaag/MiMangaNu/blob/master/app%2Fsrc%2Fmain%2Fjava%2Far%2Frulosoft%2Fmimanganu%2FMainActivity.java#L48 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will do it this way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
Would you mind solving the conflict? :-) |
The code from Main solved the same problem and used a better solution, so I adopted the code from Main |
Change it to final var |
Actually I somehow do not observe the issue on the |
area:EditActivity.kt, UploadWorker.kt
Fixes #5814
Issue page #5814
Added permission handling to allow the program to properly access the rotated image.
Additionally fixed a bug: the database was being accessed on the wrong thread, causing the program to crash.
I cannot confirm that this issue is fully resolved, as I was unable to reproduce the issue's description on my test devices. I still need the original reporter of the issue to test it again. (Currently, none of the problems described in the issue occur on my test devices, so I believe all possible fixes have been made.)