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

Dependency Updates - Main Batch - AndroidX Other #17565

Closed
5 tasks done
ParaskP7 opened this issue Nov 29, 2022 · 26 comments
Closed
5 tasks done

Dependency Updates - Main Batch - AndroidX Other #17565

ParaskP7 opened this issue Nov 29, 2022 · 26 comments

Comments

@ParaskP7
Copy link
Contributor

ParaskP7 commented Nov 29, 2022

Parent #17551

This issue is about updating all Main - AndroidX Other related dependencies for the whole project.

This Main - AndroidX Other batch contains the following 5 dependencies:

@peril-wordpress-mobile
Copy link

Fails
🚫

Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by 🚫 dangerJS

2 similar comments
@peril-wordpress-mobile
Copy link

Fails
🚫

Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

Fails
🚫

Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by 🚫 dangerJS

@ParaskP7 ParaskP7 mentioned this issue Nov 29, 2022
50 tasks
@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Nov 29, 2022

✅ ✅ The latest stable versions for both, the androidXLegacySupportCoreUiVersion and androidXLegacySupportV4Version are 1.0.0 (see releases). As such, these items are now marked as complete.

❗ However, we might take this opportunity and see if we can get rid of this legacy dependency altogether. No matter, the least we can do is to merge these two separate versions into one, the androidXLegacyVersion.

@ParaskP7
Copy link
Contributor Author

✅ The latest stable version of androidXMultidexVersion is 2.0.1 (see releases). As such, this item is now marked as complete.

❗ However, we might take this opportunity and get rid of this dependency and Multidex in general. Android 5.0 (API level 21) and higher uses a runtime called ART which natively supports loading multiple DEX files from APK files (see here). As such, we do not need the multidex library.

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Nov 29, 2022

❓ The androidxEmojiVersion is being used for the androidx.emoji:emoji (a.k.a emoji). However, there exists a newer emoji2 library (see here). Maybe we should take this opportunity and upgrade to emoji2 from emoji. 🤔

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Dec 5, 2022

✅ The dependency related to the androidXExifInterfaceVersion version got removed as part of this #17620 PR (see commit). As such, this item is now marked as complete.

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Dec 5, 2022

FYI: The dependency related to the androidXLegacyVersion version got removed as part of this #17620 PR (see commit). As such, this item is now completely unnecessary.

@ParaskP7
Copy link
Contributor Author

❗ However, we might take this opportunity and get rid of this dependency and Multidex in general. Android 5.0 (API level 21) and higher uses a runtime called ART which natively supports loading multiple DEX files from APK files (see here). As such, we do not need the multidex library.

FYI: Multidex got removed as part of this PR.

@ParaskP7 ParaskP7 self-assigned this Jan 24, 2023
@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Jan 25, 2023

👋 @mzorz @fluiddot !

❓ The androidxEmojiVersion is being used for the androidx.emoji:emoji (a.k.a emoji). However, there exists a newer emoji2 library (see here). Maybe we should take this opportunity and upgrade to emoji2 from emoji. 🤔

As part of the above effort of mine to try and update the current androidxEmojiVersion = '1.0.0' library to its newest version, preferably migrating to the androidx.emoji2:emoji2 and utilizing its 1.1.0 version (as the 1.2.0 requires compileSdkVersion = 33), I noticed a few things that I would like to discuss with both of you first:


@mzorz - Stories

  1. It is my understanding that the current emoji support configuration was added mainly due to the need of using that on Stories (see PR). I tried to follow this PR's test instructions and I immediately realized that the stickers button it refers to is not available. Thus, my question to you here. Did we have this stickers button in the past, as part of the Stories creation flow, but then decided to remove it and stop utilizing it? Looking at the stories-android codebase, I ended up into the content_composer.xml layout resource, then closer into its stickers_add_button and the fact that its visibility is gone. I also couldn't see it becoming visible anyhow programmatically. This made me think that we are no longer using or supporting this functionality on Stories. Can you verify or correct me on that? 🙏
  2. Looking at the latest documentation on supporting modern emoji I then realized that we no longer need to have any custom emoji configuration to make this work, having Appcompat pointing to 1.4.0 (see Updatable emoji support is enabled by default via the androidx.emoji2 library within important changes) or later would automatically do that for us (see Use AppCompat to support the latest emoji).
  3. Lucky that we are, today we merged this AndroidX Core PR, which amongst other changes, it also updated androidxAppcompatVersion to 1.4.2. I then, did a thorough testing on WPAndroid and confirmed that the below set of emojis were showing as expected (apart from 14.0):
    • ❌ 14.0: 🫠, 🫱🏼‍🫲🏿, 🫰🏽
    • ✅ 13.1: 😶‍🌫️, 🧔🏻‍♀️, 🧑🏿‍❤️‍🧑🏾
    • ✅ 13.0: 🥲, 🥷🏿, 🐻‍❄️
    • ✅ 12.1: 🧑🏻‍🦰, 🧑🏿‍🦯, 👩🏻‍🤝‍👩🏼
    • ✅ 12.0: 🦩, 🦻🏿, 👩🏼‍🤝‍👩🏻
  4. I know that you haven't touched anything Stories related, but does the above (2.) and (3.) points of mine make sense to you? If so, I would like to proceed on removing this custom emoji configuration that got added as part of this PR of yours and this way WPAndroid could become that much lighter, at least emoji wise. Also, I would like to do the same for the stories-android codebase afterwards at some point and update it to also utilize emoji2, plus any other updates necessary.

Question (❓): Does all the above make sense to you? Do you have any advise for me?


@fluiddot - Aztec and Gutenberg

  1. As part of my testing on Stories above, I did some testing on Posts too, both with the Block Editor and the Classic Editor alike. From my testing:
    • Classic Editor was working fine, emoji were showing as expected (apart from 14.0). I believe this is because of the fact that the Appcompat version of WPAndroid overrides Aztec's such transitive dependency (currently on 1.0.0 😱).
    • Block Editor wasn't working, emoji weren't showing, instead the app was displaying either tofu (☐) emojis or a combination of emojis that were supposed to be shown as one instead.
  2. As such, I recommend we invest some time on the Gutenberg side and update its Appcompat dependencies there to so as to have such emojis being displayed as expected.
  3. I also recommend we invest some time on the Aztec side too, as it too is very behind Appcompat wise, no matter if WPAndroid anyway overrides those and ends-up working fine. PS: I am not sure why that's not the case with Gutenberg too, but I am not a React Native expert, far from it, so I think there might be something there for us to look into... 🤔

Question (❓): Does all the above make sense to you? Do you have any advise for me?


PS: If you want to test it yourself you would need to launch WPAndroid on a device running Android 10 or lower. FYI: Personally, I used an Android 9 emulator (Pixel API 28).

@mzorz
Copy link
Contributor

mzorz commented Jan 25, 2023

Hi @ParaskP7 ! Thanks a ton for getting into the weeds of this and all the efforts to try and make this work 🙏

I believe everything you're saying is right. There is one thing to have in mind: IIRC we disabled the dedicated button for "stickers" (actually emoji) but the library should still support adding emoji when you add a text view. So what I would test would be to add some text with emoji, introduced with the keyboard itself (something like text combined with emoji).

From what you said above, it looks like you did test it and it worked, except for Android 14.0? (am I reading that correctly on point 3?) If that is the case then I guess we'd need to fix it for Android 14.0 at some point. Other than that, your steps described above make sense to me! (i.e. essentially "revert" this PR #12045 and fix anything that comes out of removing that code, hopefully not much).

Again, thanks for your efforts maintaining the codebase 🙌

@ParaskP7
Copy link
Contributor Author

👋 @mzorz and thank you for the reply! 🥇

Thanks a ton for getting into the weeds of this and all the efforts to try and make this work 🙏

😊 🤞

I believe everything you're saying is right. There is one thing to have in mind: IIRC we disabled the dedicated button for "stickers" (actually emoji) but the library should still support adding emoji when you add a text view.

💯

So what I would test would be to add some text with emoji, introduced with the keyboard itself (something like text combined with emoji).

Yea, I did that test and everything worked as expected. 👍

PS: Actually, the emojis weren't working with the current custom emoji support configuration, but they started to work fine when the Appcompat got updated to 1.4.2. 😅

From what you said above, it looks like you did test it and it worked, except for Android 14.0? (am I reading that correctly on point 3?) If that is the case then I guess we'd need to fix it for Android 14.0 at some point.

Actually, this 14.0 from the list of emojis above refers to the emoji version, not the Android version (see first table within Support modern emoji). Apologies for making you think it is the Android 14 version.

So, this ❌ on 14.0 and those 🫠, 🫱🏼‍🫲🏿, 🫰🏽 emojis that I tested meant that those emoji wheren't working on Android 10 or below, but they do work anyway, even without the Appcompat support on Android 11 or above. Also, all other emojis that come from lower emoji versions (13.1 and below) where working as expected, thus the ✅ there.

As such, I think this is minor and can be ignored. Also, the fact that our emoji supports improved anyway gives me the confidence that we are going towards the correct direction here, right? 🤓

Does the above make sense? 😊

Other than that, your steps described above make sense to me! (i.e. essentially "revert" this PR #12045 and fix anything that comes out of removing that code, hopefully not much).

Great, thank you so much for the confirmation on that, I'll now proceed into the "revert"! 🙇 🧹 🙏

Again, thanks for your efforts maintaining the codebase 🙌

❤️ Thank YOU! ❤️

@mzorz
Copy link
Contributor

mzorz commented Jan 26, 2023

Thanks for the detailed answer and the clarification on emoji version 14, not Android 14 😅 - my bad, should have read through the links too.

So, this ❌ on 14.0 and those 🫠, 🫱🏼‍🫲🏿, 🫰🏽 emojis that I tested meant that those emoji wheren't working on Android 10 or below, but they do work anyway, even without the Appcompat support on Android 11 or above. Also, all other emojis that come from lower emoji versions (13.1 and below) where working as expected, thus the ✅ there.

As such, I think this is minor and can be ignored. Also, the fact that our emoji supports improved anyway gives me the confidence that we are going towards the correct direction here, right? 🤓

This all sounds great to me! I agree this looks like the way to go 🙌

Great, thank you so much for the confirmation on that, I'll now proceed into the "revert"! 🙇 🧹 🙏

Awesome! Hope it's not a big problem :D 🙏

Thank you once more @ParaskP7 🙏 ❤️

@ParaskP7
Copy link
Contributor Author

Coolio, its a deal, once more thank you for helping and the confirmation on the "revert" @mzorz , you are awesome! 🚀 🙇 ❤️

@ParaskP7
Copy link
Contributor Author

❓ The androidxEmojiVersion is being used for the androidx.emoji:emoji (a.k.a emoji). However, there exists a newer emoji2 library (see here). Maybe we should take this opportunity and upgrade to emoji2 from emoji. 🤔

FYI: The custom emoji support got removed as part of this PR. CC @mzorz

@fluiddot
Copy link
Contributor

Hey @ParaskP7 👋 , thanks for the heads up and sharing all details 🙇 .

@fluiddot - Aztec and Gutenberg

  1. As part of my testing on Stories above, I did some testing on Posts too, both with the Block Editor and the Classic Editor alike. From my testing:

    • Classic Editor was working fine, emoji were showing as expected (apart from 14.0). I believe this is because of the fact that the Appcompat version of WPAndroid overrides Aztec's such transitive dependency (currently on 1.0.0 😱).

Great, good to know this is already covered 😅 .

  • Block Editor wasn't working, emoji weren't showing, instead the app was displaying either tofu (☐) emojis or a combination of emojis that were supposed to be shown as one instead.
  1. As such, I recommend we invest some time on the Gutenberg side and update its Appcompat dependencies there to so as to have such emojis being displayed as expected.

Oh, good catch! I wasn't aware of this issue in Gutenberg. I checked the references to androidx.appcompat:appcompat library in Gutenberg and found the following two pointing to 1.2.0:

I understand from your comments that this could be addressed simply by bumping the version to 1.4.2, is this accurate? If so, we can create a PR to test if there's any incompatibility and also to confirm the issue is solved.

@derekblank @geriux I was wondering if this library was involved when upgrading the React Native version, but I haven't found any references. Would you mind taking a look at this? Thanks!

  1. I also recommend we invest some time on the Aztec side too, as it too is very behind Appcompat wise, no matter if WPAndroid anyway overrides those and ends-up working fine. PS: I am not sure why that's not the case with Gutenberg too, but I am not a React Native expert, far from it, so I think there might be something there for us to look into... 🤔

I agree we should also update this in Aztec. In the case of Gutenberg, my gut feeling is that it won't be needed because we set the Appcompat version in the library that integrates it (react-native-aztec). But in case, it's needed we'd have to take a look.

Otherwise, I'd like to defer this to active contributors of the Aztec-Android repository. @ParaskP7 WDYT?
Thanks!

@ParaskP7
Copy link
Contributor Author

👋 @fluiddot and thanks so much for taking a look at this! 🙇 ❤️ 🙏

Great, good to know this is already covered 😅 .

Yeaaa... 😅 🤞

Oh, good catch! I wasn't aware of this issue in Gutenberg. I checked the references to androidx.appcompat:appcompat library in Gutenberg and found the following two pointing to 1.2.0:

Thanks for checking! 🥇

I understand from your comments that this could be addressed simply by bumping the version to 1.4.2, is this accurate? If so, we can create a PR to test if there's any incompatibility and also to confirm the issue is solved.

Yea, I hope that would do it as I can't think out of the top of my head why the Classic Editor can displays those emojis fine while the Block Editor can't... 🤔 🤞

@derekblank @geriux I was wondering if this library was involved when WordPress/gutenberg#45235, but I haven't found any references. Would you mind taking a look at this? Thanks!

🙏

I agree we should also update this in Aztec. In the case of Gutenberg, my gut feeling is that it won't be needed because we set the Appcompat version in the library that integrates it (react-native-aztec). But in case, it's needed we'd have to take a look.

👍

Otherwise, I'd like to defer this to active contributors of the Aztec-Android repository. @ParaskP7 WDYT?

That makes sense, yes, but who do you think those active contributors might be? 🤔

@geriux
Copy link
Member

geriux commented Jan 26, 2023

Block Editor wasn't working, emoji weren't showing, instead the app was displaying either tofu (☐) emojis or a combination of emojis that were supposed to be shown as one instead.

Hey @ParaskP7 with the message above did you mean with emojis above 13.0 from your list?

I checked on the editor and I can see the others correctly <= 13.0

@derekblank @geriux I was wondering if this library was involved when WordPress/gutenberg#45235, but I haven't found any references. Would you mind taking a look at this? Thanks!

There weren't any updates for this library, I guess we could update it in the references you mentioned.

Yea, I hope that would do it as I can't think out of the top of my head why the Classic Editor can displays those emojis fine while the Block Editor can't... 🤔 🤞

Well they're different apps, I guess unless we force all appcompat versions we'll have different behaviors between apps Aztec/React Native Editor

So just to confirm, are the next steps to update these references react-native-aztec, react-native-editor to the latest appcompat version?

@ParaskP7
Copy link
Contributor Author

👋 @geriux and thanks for helping with this! 🙇 ❤️ 🙏

Hey @ParaskP7 with the message above did you mean with emojis above 13.0 from your list?

I checked on the editor and I can see the others correctly <= 13.0

With my testing on a Pixel API 28 emulator device (Android 9), what I am seeing is that even emojis on 12.0 are not being displayed as expected.

What device did you use? 🤔

Well they're different apps, I guess unless we force all appcompat versions we'll have different behaviors between apps Aztec/React Native Editor

👍

So just to confirm, are the next steps to update these references react-native-aztec, react-native-editor to the latest appcompat version?

I am not sure which references exactly you would need to update, and within which modules, but make sure to update Appcompat to 1.4.2 as the 1.4.0 version started including this automatic support for emojis (see docs).

PS: You won't be able to update it to any latest version anyway due to the below compileSdkVersion constraints:

  • The 1.5.1 version update needs compileSdkVersion = 32 (Android 12L)
  • The latest 1.6.0 version update needs compileSdkVersion = 33 (Android 13)

Also, you updating Appcompat alone, while still keeping some other legacy dependency like androidx.legacy:legacy-support-v4, or having some other androidx dependencies on outdated version might require you to update those too. This is just a heads-up, it might end-up not being the case, but it might also end-up being the case... 🤷 🤞 🍀

Does this help?

@fluiddot
Copy link
Contributor

Otherwise, I'd like to defer this to active contributors of the Aztec-Android repository. @ParaskP7 WDYT?

That makes sense, yes, but who do you think those active contributors might be? 🤔

@ParaskP7 Oh, I haven't thought about anyone specific to be honest. Maybe we could ping the contributors of the last releases in a GitHub issue, and see if anyone has the bandwidth to tackle it.

@ParaskP7
Copy link
Contributor Author

I haven't thought about anyone specific to be honest.

👍

Maybe we could ping the contributors of the last releases in a GitHub issue, and see if anyone has the bandwidth to tackle it.

Yes @fluiddot , I was actually thinking the same, but from what I am seeing I doubt that anyone has any bandwidth to tackle such tech debt. I am not trying to be pessimistic here, please don't get me wrong, I am just seeing everyone being on full capacity lately so I am always twice as thoughtful before asking for additional support on tech debt. 😊

@ParaskP7
Copy link
Contributor Author

FYI: @fluiddot @geriux I've just closed this issue as this batch of updates are now done on the WPAndroid side. However, feel free to continue the discussions about Aztec and Gutenberg here. Or, if you like, we could move these discussions somewhere else, maybe a dedicated issue per library. 😊

@geriux
Copy link
Member

geriux commented Jan 27, 2023

FYI: @fluiddot @geriux I've just closed this issue as this batch of updates are now done on the WPAndroid side. However, feel free to continue the discussions about Aztec and Gutenberg here. Or, if you like, we could move these discussions somewhere else, maybe a dedicated issue per library. 😊

Sounds good, I'm currently checking the Gutenberg emoji issues and I will report back in a new issue 👍

@geriux
Copy link
Member

geriux commented Jan 30, 2023

To close the loop, I created an issue with my findings after some investigations.

@ParaskP7
Copy link
Contributor Author

Great, thank you for creating that issue @geriux , and of course, for sharing your findings! 💯 🙇 ❤️

Question (❓): Reading the issue I understand that the issue with emojis not displaying persists even after updating Appcompat to 1.4.2, right? 🤔

@geriux
Copy link
Member

geriux commented Jan 30, 2023

Question (❓): Reading the issue I understand that the issue with emojis not displaying persists even after updating Appcompat to 1.4.2, right? 🤔

That's correct, after updating to 1.4.2 the issue persists 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants