wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Update kotlin and other libraries and fix the code #982

Closed planarvoid closed 1 year ago

planarvoid commented 2 years ago

In this PR I'm updating the core libraries and fixing code after the changes. I don't think there are any breaking changes except of what I fixed. I've run all the tests and tested the app as well and I think it works well. Let me know if You'd like me to split the PR into multiple PRs.

Test

  1. Make sure the app works as expected.

Review

@danilo04

Make sure strings will be translated:

danilo04 commented 1 year ago

Hi @planarvoid, thanks for the changes.

I think it can be a little risky upgrading to Android 31 since we are not sure other projects that depend on Aztec (e.g. mobile Gutenberg) will play well with the change. Let me know what do you think?

ParaskP7 commented 1 year ago

👋 @planarvoid @danilo04 !

Thank you both on working on this PR.

I think it can be a little risky upgrading to Android 31 since we are not sure other projects that depend on Aztec (e.g. mobile Gutenberg) will play well with the change. Let me know what do you think?

FYI: @AjeshRPai and @fluiddot recently helped with the targetSdkVersion = 31 upgrade (see here).

I am not sure why compileSdkVersion wasn't upgraded to 31 as well (currently 28). 🤔 If @AjeshRPai and @fluiddot is okay with that too, I am voting in favor of this upgrade, maybe as a separate upgrade to all other changes, so that it becomes easier for @AjeshRPai and @fluiddot to review and test the changes. 👍

PS: Also, I think now is a good time to extract all those references of 28 to an ext variable, just like it is already done for commonMinSdkVersion and commonTargetSdkVersion.

fluiddot commented 1 year ago

I think it can be a little risky upgrading to Android 31 since we are not sure other projects that depend on Aztec (e.g. mobile Gutenberg) will play well with the change. Let me know what do you think?

@danilo04 good point, I'd like to note that we already upgraded compile and target sdk version to Android API 31 in Gutenberg (PR reference). Hence, I don't foresee potential conflicts due to this change. In any case, I'd advocate to do a double-check just in case.

I am not sure why compileSdkVersion wasn't upgraded to 31 as well (currently 28). 🤔

@ParaskP7 good point, I'm not sure either why the compileSdkVersion wasn't upgraded 🤔, maybe it wasn't necessary for the purpose of upgrading Android 12?

If @AjeshRPai and @fluiddot is okay with that too, I am voting in favor of this upgrade, maybe as a separate upgrade to all other changes, so that it becomes easier for @AjeshRPai and @fluiddot to review and test the changes. 👍

@planarvoid Yep, I'm also voting in favor of the upgrade. Let me know when the PR is ready to be tested, and I'll spare some time to test it in Gutenberg to confirm there's no breakage with the changes.

danilo04 commented 1 year ago

Hi @ParaskP7 @fluiddot. Good to know Gutenberg upgraded to Android 31 🎉 ! I think we just have to resolve the conflicts and also generate a new release build from trunk since we have some other changes merged, so @fluiddot can test those changes as well.

@planarvoid let me know if you have time to make the changes.

ParaskP7 commented 1 year ago

👋 @danilo04 @fluiddot !

@danilo04 good point, I'd like to note that we already upgraded compile and target sdk version to Android API 31 in Gutenberg (https://github.com/WordPress/gutenberg/pull/44610). Hence, I don't foresee potential conflicts due to this change. In any case, I'd advocate to do a double-check just in case.

🥇

@ParaskP7 good point, I'm not sure either why the compileSdkVersion wasn't upgraded 🤔, maybe it wasn't necessary for the purpose of upgrading Android 12?

Yeap, I am not sure as well, maybe it was an oversight. 🤔 Cc @AjeshRPai

PS: To my knowledge and although it is (kind of) okay to have compileSdkVersion being of a lesser value than that of targetSdkVersion, it is (highly) recommended that compileSdkVersion is equal or higher than targetSdkVersion.

@planarvoid Yep, I'm also voting in favor of the upgrade. Let me know when the PR is ready to be tested, and I'll spare some time to test it in Gutenberg to confirm there's no breakage with the changes.

🥇

I think we just have to resolve the conflicts and also generate a new release build from trunk since we have some other changes merged, so @fluiddot can test those changes as well.

Since this PR is also about Kotlin, I would recommend to do the compileSdkVersion = 31 upgrade on a separate PR and then continue with this PR (if need be). Wdyt? 🤔 Cc @danilo04 @planarvoid

AjeshRPai commented 1 year ago

Yeap, I am not sure as well, maybe it was an oversight.

Yep, it was. During the migration, I missed this. My Bad.

PS: To my knowledge and although it is (kind of) okay to have compileSdkVersion being of a lesser value than that of targetSdkVersion, it is (highly) recommended that compileSdkVersion is equal or higher than targetSdkVersion.

Yeah, I completely agree.

PS: Also, I think now is a good time to extract all those references of 28 to an ext variable, just like it is already done for commonMinSdkVersion and commonTargetSdkVersion.

I also vote for this suggestion as it would make it easier in the future to manage the versions.

ParaskP7 commented 1 year ago

👋 @planarvoid !

I guess this PR can be now closed and we could (potentially) create another separate PR to update compileSdkVersion to 31, right? 🤔

Are there any other major changes in this PR that weren't already included within these below PRs already, that is, apart from the compileSdkVersion and other dependency updates I am seeing in the diff?

ParaskP7 commented 1 year ago

👋 @planarvoid @danilo04 !

As per my comment above, let's consider closing this PR for now. Wdyt? 🤔

danilo04 commented 1 year ago

We agree @ParaskP7. Closing the PR.