burhanrashid52 / PhotoEditor

A Photo Editor library with simple, easy support for image editing using paints,text,filters,emoji and Sticker like stories.
MIT License
4.13k stars 991 forks source link

Creating setShadow to TextView (to be used from code) #370

Closed anderbytes closed 3 years ago

anderbytes commented 3 years ago

I wasn't able to test it as I didn't succeed on linking my Android Studio Kotlin project to locally implement the PhotoEditor Intellij project as a dependency on my Android app (sadly), but I'd like to propose this change as it seems simple (as others that may contribute in the future) related to TextStyling

Please test it or fix it if I forgot anything ;-)

burhanrashid52 commented 3 years ago

Please test the changes before creating a PR. This will avoid going back and forth if something get broken. Also please add the tests for those changes to make sure someone doesn't break it in future.

anderbytes commented 3 years ago

I will try again to link the local project to my main project where I call the PhotoEditor class, so I can do some local testing. But as I don't "use" your sample editor, I will only do some testing with the methods (old and new).

What do you mean with "add those tests" ? Something related to Intellij JUNIT plugin? I'm pretty novice in this so I've never really used "test feature" of the IDE.

If I ever manage to do it right, I'll be more than happy to continue collaborating with what I can, even if my main language being Kotlin

burhanrashid52 commented 3 years ago

I will try again to link the local project to my main project where I call the PhotoEditor class, so I can do some local testing. But as I don't "use" your sample editor, I will only do some testing with the methods (old and new).

If you can import as module and it should work.

What do you mean with "add those tests" ? Something related to Intellij JUNIT plugin? I'm pretty novice in this so I've never really used "test feature" of the IDE.

Yes, Junit test or UI test to make sure the the text changes are applied to the textview

If I ever manage to do it right, I'll be more than happy to continue collaborating with what I can, even if my main language being Kotlin

Thank you :)

anderbytes commented 3 years ago

@burhanrashid52 thanks for the module import tip.

I did some testing here in my app and it worked perfectly! but I am clueless about using JUnit, even after seeing your PinchTestHelper in "src/androidTest", I can't figure how to properly test this new TextView method. As I mentioned... never ever worked with automated tests before.

Would you please create a test about a textview modification so I can learn it? You could even do one related to a feature that your app already does (ex: TextView font size change), and I'd understand it and create my own test right after, related to Shadow.

If everything goes well, I'll be soon adding more improvements, I could even be commiting a few more changes to TextView before you change from 1.1.4 to 1.1.5

burhanrashid52 commented 3 years ago

UI test for text is tricky. You can post the screenshot here to give an idea how it looks. Because the only way I see is to test using TextStyleBuilderTest and check weather it's setting property on TextView properly.

anderbytes commented 3 years ago

Uploaded here some screenshots of my testing.

UI test for text is tricky. You can post the screenshot here to give an idea how it looks. Because the only way I see is to test using TextStyleBuilderTest and check weather it's setting property on TextView properly.

blackShadow - code blackShadow - normal blackShadow - zoom noShadow - code noShadow - normal noShadow - zoom redShadow - code redShadow - normal redShadow - zoom

anderbytes commented 3 years ago

ps: radius in the "red" test was actually 16F (greater than black one), I just took the screenshot of the code before changing that from 7F.

anderbytes commented 3 years ago

Hi @burhanrashid52 something went wrong ? I'm seeing some error above, but it's too generic and says something about a DEX file , which I don't know about.

I could remove the "version bumps" I did on the PR, would it help?

burhanrashid52 commented 3 years ago

It failing because of 64K method limit.

Task :app:mergeDexDebugAndroidTest FAILED
ERROR:: D8: Cannot fit requested classes in a single dex file (# methods: 90699 > 65536)
com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives: 
The number of method references in a .dex file cannot exceed 64K.

Learn how to resolve this issue at https://developer.android.com/tools/building/multidex.html

anderbytes commented 3 years ago

It failing because of 64K method limit.

Task :app:mergeDexDebugAndroidTest FAILED
ERROR:: D8: Cannot fit requested classes in a single dex file (# methods: 90699 > 65536)
com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives: 
The number of method references in a .dex file cannot exceed 64K.

Learn how to resolve this issue at https://developer.android.com/tools/building/multidex.html

It failing because of 64K method limit.

Task :app:mergeDexDebugAndroidTest FAILED
ERROR:: D8: Cannot fit requested classes in a single dex file (# methods: 90699 > 65536)
com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives: 
The number of method references in a .dex file cannot exceed 64K.

Learn how to resolve this issue at https://developer.android.com/tools/building/multidex.html

Is that "excess" related to the little modification I made?? That can't be real. This it getting real real hard you know. I'm justing adding a new "withTextShadow" property.

Also... I could build it normally here locally, don't know why this error happened here.

burhanrashid52 commented 3 years ago

Updating the libraries build.gradle might in increase the methods count. Can you revert it back and check in your local ?

anderbytes commented 3 years ago

Just reverted the latest commit and re-inserted the actual TextShadow code.

If still too many methods, I could also try to revert the first commit (https://github.com/burhanrashid52/PhotoEditor/pull/370/commits/d20ed83d3056180795a879a076e8ac0297f08c08) , that also had some version bumps.

burhanrashid52 commented 3 years ago

LGTM