search

wordpress-mobile / AztecEditor-Android

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

Update `minSdkVersion` (from `21` to `24`) #1017

Closed ParaskP7 closed 1 year ago

ParaskP7 commented 1 year ago

This PR upgrades FluxC to minSdkVersion to 24. This has been unblocked because:

As part of this minSdkVersion = 24 upgrade the below changes were also applied in order to fix any additional warnings that this change brought-up:

Warnings Resolution List:

  1. Resolve obsolete sdk int lint warnings.
  2. Resolve obsolete sdk int lint warnings for all xml files.

Test List:

  1. Resolve robolectric package parser exception due to config sdk 23.
  2. Document test failures due to expected actual assertion mismatch.
  3. Fix edit text to html related test failures.

Warning (⚠️): Please be very mindful of this fix test failures change and verify that this is not a breaking change of some sorts. See commit description for more info.

Test

  1. There is nothing much to test here.
  2. Verifying that all the CI checks are successful should be enough.
  3. However, if you want to be thorough about reviewing this change, you could:
    1. Quickly smoke test the example app and see if it is working as expected.
    2. Perform a quick check in gutenberg using this PR as a double-check. Cc @fluiddot

Review

@fluiddot as per the discussion on updating and testing gutenberg as well, this might be a good opportunity for you see what's coming to Aztec. As such, it might be good if we merge this change along with merging the equivalent minSdkVersion = 24 upgrade on gutenberg, that is, when that gets ready by you. PS: there is no time pressure here.

PS: Also, and based on this comment of yours, it might be better for your to perform a quick check in gutenberg using this PR instead, and not #1016 in order to save yourself some time, as this one includes that in it as well.

Merge instructions

Make sure strings will be translated:

ParaskP7 commented 1 year ago

👋 @fluiddot !

@ParaskP7 I finally managed to spare some time to check this today 🙂.

😃 You are the best, thank you for finding this spare time, now enjoy your AFK without thinking about it, we can back to that after your return! 🙇

I tested the Aztec version using the value 1017-29e499974e6e2c450112ce3dd74c86b033e78cd6 in Gutenberg and the build failed because the minSdkVersion in Gutenberg is 21 (reference). Therefore, we'd also have to bump the minSdkVersion there to 24 in order to include these changes.

Yea, that's absolutely expected. 👍

Apart from that, I did a quick check locally by manually changing the minSdkVersion to 24 and didn't find any issue when testing.

Thank for for bumping Gutenberg's minSdkVersion to 24 to test these changes, I am glad you didn't find any issues, at least at this stage. 🙇

However, I'd like to devote further time to this before confirming that everything is ok. As we discussed internally, I won't have the bandwidth to take a look until Nov 25th, 2022. I'll let you know when I have the results, thanks 🙇 !

This is totally fine, please enjoy your AFK. After you're back we can pick it up were we left it and continue from there, this is not urgent work. 💯

NOTE: Changes from this PR and https://github.com/wordpress-mobile/AztecEditor-Android/pull/1016 could be merged if it's blocking other projects, Aztec is versioned in Gutenberg so this won't impact until we bump the Aztec version there.

Thank you for saying that, but as these changes are not urgent, nor blocking other projects, at least as far as I am aware, I suggest we do it right and once, after everything is tested, as supposed to, and only then merge these changes to trunk, for both PRs. 🙏

fluiddot commented 1 year ago

@fluiddot as per the discussion on updating and testing gutenberg as well, this might be a good opportunity for you see what's coming to Aztec.

@ParaskP7 I'd like to note that I couldn't check what's coming to Aztec in Gutenberg. In relation to this, I'm wondering if we should create a GitHub release for releasing a new Aztec version with all the changes, wdyt?

As such, it might be good if we merge this change along with merging the equivalent minSdkVersion = 24 upgrade on gutenberg, that is, when that gets ready by you. PS: there is no time pressure here.

Good point, I'll prepare a PR next week to bump minSdkVersion to version 24 in Gutenberg in order to match the same version. Note that since we're not bumping the Aztec version in Gutenberg yet, it's safe to merge this PR. In a follow-up PR, once we release a new Aztec version, I'll bump the Aztec version there.

ParaskP7 commented 1 year ago

👋 @fluiddot !

Thank you so much for the review and testing, and on both, Aztec plus Gutenberg, you are awesome! 🙇 ❤️ 🥇

@ParaskP7 I'd like to note that I couldn't check what's coming to Aztec in Gutenberg. In relation to this, I'm wondering if we should create a GitHub release for releasing a new Aztec version with all the changes, wdyt?

Yes, I think this is a very good idea Carlos, as soon as we merge this PR, I'll go ahead and create a new Aztec release. 💯

Good point, I'll prepare a PR next week to bump minSdkVersion to version 24 in Gutenberg in order to match the same version.

Perfect, thank you Carlos! 🙇

Afterwards, and when that new version of Gutenberg get released, which would contain the minSdkVersion = 24 update and the Aztec update, I also recommend that on WPAndroid we then update both, the aztecVersion alongside the gutenbergMobileVersion this time. Wdyt? 🤔

Note that since we're not bumping the Aztec version in Gutenberg yet, it's safe to merge this PR. In a follow-up PR, once we release a new Aztec version, I'll bump the Aztec version there.

Perfecto, let me then merge this PR now and create a new Aztec version for you, I'll let you know when that is done! 🙏

fluiddot commented 1 year ago

Afterwards, and when that new version of Gutenberg get released, which would contain the minSdkVersion = 24 update and the Aztec update, I also recommend that on WPAndroid we then update both, the aztecVersion alongside the gutenbergMobileVersion this time. Wdyt? 🤔

Yes, usually when we upgrade the Aztec version in Gutenberg we also do it in WordPress-Android, so, all Aztec references point to the same version 👍 .

Perfecto, let me then merge this PR now and create a new Aztec version for you, I'll let you know when that is done! 🙏

Great, thank you @ParaskP7 !

ParaskP7 commented 1 year ago

Yes, usually when we upgrade the Aztec version in Gutenberg we also do it in WordPress-Android, so, all Aztec references point to the same version 👍 .

Coolio, you are the best! 🚀 🥇 🙇

Great, thank you @ParaskP7 !

Thank YOU @fluiddot and FYI the new release tag in now ready: v1.6.3

fluiddot commented 1 year ago

@ParaskP7 I'm afraid I won't have the bandwidth to update Aztec and the minSdkVersion in Gutenberg soon, not sure if that would block other work. If so, please let me know and I'll try to prioritize it. Otherwise, I estimate I could work on this at some point in the next two weeks.

ParaskP7 commented 1 year ago

👋 @fluiddot and thanks for the heads-up! ❤️

Please don't feel stressed about this update of minSdkVersion in Gutenberg, it is not blocking anything on our side, so do prioritize this update as per your schedule and availability. Given the current ongoing WP->JP migration, I would even recommend you forget about this and come back to it next month. 💯

FYI: The only blocker I am envisioning is on your side, in terms that if Gutenberg is not updated soon, next time you would want to use a newer version of Aztec, then this would not be possible and force you to first do the minSdkVersion update. But, I know you know that anyway. Apart from that, everything on our side is under control and this pending work on Gutenberg is not blocking us, so please don't worry about that, not at all. 👍

fluiddot commented 1 year ago

Great, thank you very much @ParaskP7 for elaborating on the potential blockers regarding not updating Gutenberg 🙇. I wouldn't like to postpone it too much, but I'm glad to know that it's not blocking other work.

FYI: The only blocker I am envisioning is on your side, in terms that if Gutenberg is not updated soon, next time you would want to use a newer version of Aztec, then this would not be possible and force you to first do the minSdkVersion update. But, I know you know that anyway. Apart from that, everything on our side is under control and this pending work on Gutenberg is not blocking us, so please don't worry about that, not at all. 👍

Good point, we'll note that.

fluiddot commented 1 year ago

To close the loop: Here are the PRs in Gutenberg to update minSdkVersion and bump Aztec dependency:

Once they are approved and merged, I'll create an alpha version of Gutenberg Mobile and update the Aztec version in WordPress-Android.

ParaskP7 commented 1 year ago

Great, thanks for the update and for working on that @fluiddot ! 🚀 ❤️ 🙇