Fix #5711: Incorrect rotation logic and re-edit bug#6642
Conversation
|
I've tried that beta see video: small_Screen_Recording_20260204_014959_Commons.mp4can you reproduce it @Kota-Jagadeesh ? (also if I rotate and save, it also displays differently in |
|
Additionally, I noticed it seems that on every rotate/save the app saves a new image in my
Footnotes
|
|
Sorry for extending scope, but would you mind creating unit tests for various rotations with all JPGs at https://github.com/recurser/exif-orientation-examples ? EXIF rotation is a tricky topic, so getting a clear+testable definition of how it is supposed to work would help a lot now and in the future. |
Screenrecorder-2026-02-04-21-01-35-136.mp4@mnalis I tested with this branch from android studio, but couldn't reproduce the issue. It works fine for me and i also dowwnloaded the Could you please some more information regarding hwo you reproduced the issue ? |
even i thought of this issue& even though the current main branch behaves this way, we definitely shouldnn't be polluting the user's downloads folder like this wwith the temporary edits. i will update the pr to save these intermediate files to the app's internal |
@nicolas-raoul yeah sure, i'll add the unit testt class that iterates through the standard exif orientation examples to verify that the rotation logic holds up against all of the edge cases. |
|
@mnalis @nicolas-raoul I have updated the code with the fix for the images to store in the cache rather than the downloads. I tested the changes and everyhtinng worked fine for me and here is the video clip of that : Screenrecorder-2026-02-04-23-33-55-689.mp4Here, i uploaded a picture #5711 (comment) and saved with rotations and without rotations, before this fix: all the pics are been saved in the downloads dir, after the fix: now the images are not being saved in the downloads anymore :) |
Looking forward to it! Would you mind doing it in this PR? Thanks! 🙂 |
|
@nicolas-raoul Updated the PR with all the changes, PLease review it :) |
It is still incorrect for me, even with latest changes. Here is a detailed procedure for reproducing the issue:
Here is the video how it looks for me: small_SVID_20260209_030103_1.mp4Can you reproduce the issue there @Kota-Jagadeesh? Or does the step (10) differ for you, if you use exactly that image I provided ? Perhaps you had an old copy of the image which was saved so it was modified and behaves differently? And in which environment (phone/android version) that happens for you? Because I've tried on 2 quite different devices (Samsung Galaxy S23+ on Android 15, and Huawei P30Pro on Android 10) and the bug is exactly the same on both with that specific picture freshly extracted from |
|
(BTW here is a copy of that
Thanks, I can confirm that multiple junk copies in |
Sorry for my oversight. I was able to reproduce the issue. I thought I was using the same image you provided, but I ended up using a duplicate image which was saved while editing the image, and that made the bug seem to be solved. I understand where the problem is and will fix it and update the PR shortly. @mnalis, thank you for testing! 🙂 |
|
✅ Generated APK variants! |
Yeah, that PR fixes one of the issue mentioned in the comments of this PR, so I had to use the fixes keyword. Sorry, for that :) |
|
✅ Generated APK variants! |
|
@nicolas-raoul Should i proceed with this ? 🙂 |
|
Can you leave the unit test in place and just disable it? And mention it in that unit test in the new issue. Thanks! |
|
✅ Generated APK variants! |
@nicolas-raoul i've added the |
|
✅ Generated APK variants! |
nicolas-raoul
left a comment
There was a problem hiding this comment.
Then I think it will be good to merge. Thanks for your patience! :-)
| val gDiff = abs(gExp - gNorm) | ||
| val bDiff = abs(bExp - bNorm) | ||
|
|
||
| if (rDiff > tolerance || gDiff > tolerance || bDiff > tolerance) { |
There was a problem hiding this comment.
Can you please remove the concept of tolerance from this pull request?
It complexifies the code and is not necessary since the editor must be lossless.
Thanks!
There was a problem hiding this comment.
@nicolas-raoul Made the required changes, once you are free, can you please look into it and let me me know if there are any changes needed 🙂
|
✅ Generated APK variants! |
|
✅ Generated APK variants! |
|
@nicolas-raoul Any update on this PR? 🙂 |
|
✅ Generated APK variants! |
nicolas-raoul
left a comment
There was a problem hiding this comment.
Rotation seems to work well in my tests, including one test where I actually uploaded the picture:
https://commons.wikimedia.org/wiki/File:Orix_Nagoya_Nishiki_Building.jpg
As mentioned the next step will be #6659 but the code here is using jpegtran so I am optimistic.
Thanks for the great work!
@nicolas-raoul @mnalis Thank you for the in-depth review and for correcting me whenever I’m wrong ❤️ |
|
Thank you for your patience! |
Description (required)
Fixes #5711
This PR addresses the "Incorrect rotation" where the images were being rotated 90 degrees upon opening the "Edit Image" screen or saving without edits.
Changes made:
fixed "save without edit" bug: Updated
TransformImageImpl.ktto stop defaulting to a 90-degree rotation when the requested rotation is 0. It now correctly applies no rotation (OPT_DEFAULTS).fixed the re-edit workflow: Updated
UploadMediaDetailFragment.ktto check if an edited file path exists (mediaUri) before launching the editor. This ensures that clicking "Edit" a second time loads the current state of the image instead of reverting to the original file.Tests performed (required)
Tested ProdDebug on Redmi Note 13 Pro with API level 35 (Android 15).
Screenshots (for UI changes only)
COMMONS.REC1.mp4