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

Lossless Transformations(Rotate feature) #5252

Merged
merged 30 commits into from
Oct 24, 2023

Conversation

shankarpriyank
Copy link
Contributor

Description (required)

Made simple changes to initiate the editing feature in the app, right now the UI can only facilitate rotating the images, the rotated imaged are written in the download directory, but how to propagate the images back to the uploads is something I am trying to figure rn, and I would appreciate any help with it.

Also cause I have merged #5220 in this branch so please ignore the changes that were made in that pr.

Please let me know what do you think about the changes, I know that a lot of improvements can be made but I am focusing on getting things running first, let me know if you want me to do anything differently

@shankarpriyank shankarpriyank marked this pull request as draft July 3, 2023 14:20
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Very nice, great start! :-)

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul Thank you, I am having a hard time trying to find a way to send the edited back to the upload screen.
Do you have any advice/ idea on how it might be done?

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul can you do a quick review here, I was not sure where to add the helper functions.
I am unable to upload any image right now, even from the master branch. So could not test the functionality.
I guess that now maybe that the rotated image will get uploaded(not sure though!), trying to figure out how to reflect the change in the preview of the image too.

@nicolas-raoul
Copy link
Member

Hi @shankarpriyank sorry for the delay!

I just uploaded this picture using this branch, rotating it and tapping "Save". Unfortunately the uploaded image is not rotated, as you can see, it is deeper than a preview issue. For testing you can just take pictures of streets or monuments ad rotate them first to an unnatural angle.

Thanks! :-)

@shankarpriyank
Copy link
Contributor Author

No worries @nicolas-raoul, thanks for testing it, I will try finding to fix this

@shankarpriyank
Copy link
Contributor Author

Hi @nicolas-raoul can you test this branch again?
I tested it with a bunch of images and it was working as intended.

@nicolas-raoul
Copy link
Member

I just uploaded https://commons.wikimedia.org/wiki/File:2007,_cliff_near_Rabat_in_Malta.jpg with your branch. The picture originally had the wrong orientation, but I corrected it using the Edit image button and now it looks great. :-)

One remark: The result image does not have EXIF anymore. Would you mind making sure that the EXIF is kept? It is vital metadata information of the picture, it can contain coordinates and exact date for instance, which are very important to use the picture in Wikipedia.

Thanks a lot!

@shankarpriyank
Copy link
Contributor Author

How are you checking the exif data? is there a handy tool i can use to test it ?

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul can you please test again, the exif data should be retained now

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Somehow I get Failed to rotate image now... I tried two different pictures, 3 times 90 degrees.

@nicolas-raoul
Copy link
Member

screen-20231007-225756.mp4

@shankarpriyank
Copy link
Contributor Author

That's happening most probably because the image's encoding is not supported by lljtran(mostly happens huffman encoding), can you please try with some other images.

@nicolas-raoul
Copy link
Member

The successful rotation I had mentioned at #5252 (comment) was with a picture taken by the same camera the same day, though, os I doubt it used a different encoding...

I just tried with a picture taken by Pixel 7 Pro's stock camera, the error message does not appear but the picture is not rotated either. :'-(

@shankarpriyank
Copy link
Contributor Author

Done @nicolas-raoul

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Sorry I just caught these.
Once the Flamingo PR is merged (now waiting for #5339), I will ask you to rebase (or pull), and will review the code once more just in case, thank you for your understanding. :-)





Copy link
Member

Choose a reason for hiding this comment

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

Ah, is all of this whitespace needed? :-)


val animator = ValueAnimator.ofFloat(0f, 1f).setDuration(1000L)


Copy link
Member

Choose a reason for hiding this comment

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

Single white lines is better within functions.

}
btn_save.setOnClickListener {
getRotatedImage()

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line before }

@shankarpriyank
Copy link
Contributor Author

Sorry I just caught these. Once the Flamingo PR is merged (now waiting for #5339), I will ask you to rebase (or pull), and will review the code once more just in case, thank you for your understanding. :-)

Hey! no worries at all.
Sorry for my formatting mistakes, I have fixed them now

} catch (Exception e) {
Timber.e(e);
}

Copy link
Member

Choose a reason for hiding this comment

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

Please remove empty lines here too.

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 line

@nicolas-raoul
Copy link
Member

Actually, would you mind:

  • From your same branch, creating a new pull request.
  • On the pull request creation webpage, select base:v4.2.0-release instead of base:main as the target branch.
  • Send the pull request.

This will allow us to review all of your changes clearly (not mixed with other Flamingo-related changes). Maybe we can even merge this into the v4.2.0-release branch.

@shankarpriyank
Copy link
Contributor Author

I made the pr, but in the first look it also seems to have unrelated changes.
I am a bit occupied for the next couples of days, will be having a closer look by monday

@nicolas-raoul
Copy link
Member

The main branch now supports Flamingo :-)

@nicolas-raoul
Copy link
Member

Would you mind rebasing or pulling from main, then checking the difference and possibly donig a bit of clean-up? Then ask for a review. :-)

# Conflicts:
#	app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java
#	app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.java
@shankarpriyank
Copy link
Contributor Author

shankarpriyank commented Oct 20, 2023

I pulled the changes from main, and now the pr looks quite clean to me.
There is some noise from extra formatting fixes, but I don't see any harm in them

@nicolas-raoul
Copy link
Member

Any idea why a unit test fails?

@shankarpriyank
Copy link
Contributor Author

Any idea why a unit test fails?

Nope, had a look at the logs, I could not really infer much from it apart from

Caused by: java.lang.IllegalStateException: Cannot process instrumented class fr/free/nrw/commons/di/DaggerCommonsApplicationComponent$60. Please supply original non-instrumented classes.

@nicolas-raoul
Copy link
Member

fr.free.nrw.commons.settings.SettingsActivityUnitTests > testSetTotalUploadCount FAILED
    java.lang.reflect.InvocationTargetException at SettingsActivityUnitTests.kt:77
        Caused by: java.lang.IllegalStateException at SettingsActivityUnitTests.kt:77

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul its fixed now

app/build.gradle Show resolved Hide resolved
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

minor

Intent intent = new Intent(getContext(), EditActivity.class);
intent.putExtra("image", uploadableFile.getFilePath().toString());
startActivityForResult(intent, REQUEST_CODE_FOR_EDIT_ACTIVITY);

Copy link
Member

Choose a reason for hiding this comment

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

remove 2 empty lines

@Override
public void onEditButtonClicked(int indexInViewFlipper){
view.showEditActivity(repository.getUploads().get(indexInViewFlipper));

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 empty line

}

init()

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 empty line

@OnClick(R.id.edit_image)
public void onEditButtonClicked() {
presenter.onEditButtonClicked(callback.getIndexInViewFlipper(this));

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 empty line

presenter.onEditButtonClicked(callback.getIndexInViewFlipper(this));

}

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 empty line

}

}

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 empty line

android:text="@string/menu_save_categories"
android:textColor="@android:color/white" />
</LinearLayout>

Copy link
Member

Choose a reason for hiding this comment

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

remove 2 empty lines

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul i guess all of them are fixed now

} catch (Exception e) {
Timber.e(e);
}

Copy link
Member

Choose a reason for hiding this comment

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

remove 1 line

@shankarpriyank
Copy link
Contributor Author

fixed

@nicolas-raoul nicolas-raoul merged commit 2ddb6b2 into commons-app:main Oct 24, 2023
1 check passed
@nicolas-raoul
Copy link
Member

Huge congratulations @shankarpriyank for adding this lossless rotation feature to the app, as well as setting the foundation for other lossless transforms.
Thanks to you, many picture uploads will have both keep their original quality and not occupy more disk space, while being in the right orientation to be used in Wikipedia articles and other places.
You truly help Wikipedia/etc run better!

@shankarpriyank
Copy link
Contributor Author

Thank you so much for your kind words!

@shankarpriyank shankarpriyank changed the title [WIP]Lossless Transformations(Rotate feature) Lossless Transformations(Rotate feature) Dec 30, 2023
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.

3 participants