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

Option to create parser-specific Emoji object #189

Closed
yoobi opened this issue Feb 26, 2024 · 33 comments · Fixed by #190
Closed

Option to create parser-specific Emoji object #189

yoobi opened this issue Feb 26, 2024 · 33 comments · Fixed by #190
Labels

Comments

@yoobi
Copy link
Contributor

yoobi commented Feb 26, 2024

Description of Bug

Follow up of #179, after implementing the version 1.8.0 in my project we still need to add kotlinx-serialization

Reproduction Steps

  1. Implement new version into project
  2. use customInitializer
  3. emoji won't load
@yoobi yoobi added the bug label Feb 26, 2024
@wax911 wax911 linked a pull request Feb 27, 2024 that will close this issue
3 tasks
@wax911
Copy link
Member

wax911 commented Feb 27, 2024

I believe this is a perfect opportunity to provide artifacts for serialization variants for the sole reason of simplification. Here's what I have in mind from a high level, we can discuss the implementation detail.

android-emojify

The idea is to split out the library into:

  • core: Purely functional parts of emojify
  • contract: Model as interface with default implementation for deserialization to avoid having to rely on AbstractEmoji and serializer.decodeFromStream(inputStream).map { it.toEmoji() }

This approach is slightly less efficient (.map) as you perform two passes of the model constructing it & iterating through each model to create an emoji, rather than the original implementation which does this in one pass.

In the original implementation decodeFromStream has a time complexity of O(n), where n is the size of the input stream, the overall time complexity for the first case remains O(n) because it directly deserializes the data from the input stream into a list.

In your case decodeFromStream utilises .map , after deserialization, there's an additional map operation applied to each element, which has a time complexity of O(m), where m is the number of elements in the list. Thus, the overall time complexity for the second case would be O(n + m), considering both deserialization and mapping.

  • deserializer: Essentially we implement the IEmoji interface into concrete implementation which will look very similar to Emoji just with the correct annotations (this way consumers only use what they need and can create their own if need be)
  • initializer: Also optional, and should work on the basis of the contracts and shouldn't care about which deserializer is used. e.g.
interface IEmojiDeserializer {
    fun decodeFromStream(inputStream: InputStream): List<IEmoji>
}
import java.io.IOException

abstract class AbstractEmojiInitializer : Initializer<EmojiManager> {
    abstract val serializer: IEmojiDeserializer
    
    @Throws(IOException::class, SerializationException::class)
    fun initEmojiData(
        assetManager: AssetManager,
        path: String = DEFAULT_PATH,
    ): List<IEmoji> {
        return assetManager.open(path).use { inputStream ->
            serializer.decodeFromStream(inputStream)
        }
    }

@yoobi
Copy link
Contributor Author

yoobi commented Feb 27, 2024

Spliting into artificats(:serializer-jackson, :serializer-moshi, :serializer-gson etc... ) would be make total sense.

This approach is slightly less efficient (.map) as you perform two passes of the model constructing it & iterating through each model to create an emoji, rather than the original implementation which does this in one pass.

I've had to do it that way because Emoji uses init { initializeProperties() } and when parser creates object it is null thus unicode, htmlDec and htmlHex needs a refacto.

I'm not sure if I understood correctly but you wish to make :initializer a new module with androidx.startup that :serializer could depend on if they wanted to ? And if user doesn't want to use :initializer there should be a way to create the object with EmojiInitializer().onCreate(context) ?

@yoobi
Copy link
Contributor Author

yoobi commented Feb 28, 2024

Hello, I've begin the change on local but I'm facing issue.

I've put AbstractEmojiInitializer into the :contract module but AbstractEmojiInitializer depends on EmojiManager which in turn depends on most of utility function from emojify.parser package, by moving it those 2 (EmojiManager and functions from parser) the :emojify module is almost empty and not holding logic anymore.

Wouldn't it make more sense to create a contract package inside emojify and let the :serializer-xxx depends on it ? If you have other ideas do tell me :p

I can also push what I've done so far so you can take a closer look (note that it is not working nor compiling properly due to the issue mentioned earlier)


EDIT: I think I found a nice way to do it while keeping the :contract, :emojify and :serializer-xxx. I'll push it soon

@wax911
Copy link
Member

wax911 commented Feb 28, 2024

Spliting into artificats(:serializer-jackson, :serializer-moshi, :serializer-gson etc... ) would be make total sense.

This approach is slightly less efficient (.map) as you perform two passes of the model constructing it & iterating through each model to create an emoji, rather than the original implementation which does this in one pass.

I've had to do it that way because Emoji uses init { initializeProperties() } and when parser creates object it is null thus unicode, htmlDec and htmlHex needs a refacto.

I'm not sure if I understood correctly but you wish to make :initializer a new module with androidx.startup that :serializer could depend on if they wanted to ? And if user doesn't want to use :initializer there should be a way to create the object with EmojiInitializer().onCreate(context) ?

  • I apologize for not clarifying the initializer module, the idea was to have initializer for each serializer-* lib, which would depend on all the other deps, this would make it completely optional and allow us to move it out of core (this might be overkill so would love to hear what your thoughts are around this approach)
  • Regarding the issue with parser creates object is null would be expected because since the implementation of AbstractEmoji would need a way to trigger initializeProperties on it's init { } so ideally you could move initializeProperties into AbstractEmoji and the implemenation class would call the protected method

@yoobi
Copy link
Contributor Author

yoobi commented Feb 28, 2024

  • Creating a new module for initializer might be a bit tricky as it needs to access EmojiManager which is in core and that would break the dependency inversion. I'll try to think of something once we are done migrating serializer
  • In my last PR I've managed to refacto the initializeProperties into lazy field, the only issue is that the loop is being run twice, one for htmlDec and one for htmlHex

@wax911
Copy link
Member

wax911 commented Feb 29, 2024

You raise a great point, I honestly think we could just ommit initializer entirely, a usage guide for v2.0.0 would probably suffice since in reality if we're going with customizability then rather let a person how they want to handle initialization?

@yoobi
Copy link
Contributor Author

yoobi commented Feb 29, 2024

True a simple how to in the README would be enough for users who wants to use runtime-initializer

@wax911
Copy link
Member

wax911 commented Feb 29, 2024

Great, I'm happy with the PR and will suggest/ask some small questions on it ⭐

@yoobi
Copy link
Contributor Author

yoobi commented Apr 24, 2024

Hello, do you have an estimated date for 1.9.0 release ?

@wax911
Copy link
Member

wax911 commented Apr 24, 2024

Hey @yoobi 😆 not going to lie but I had completely forgot to publish the release

@yoobi
Copy link
Contributor Author

yoobi commented Apr 25, 2024

Hahaha no problem :p

@wax911
Copy link
Member

wax911 commented Apr 27, 2024

I wanted to update the README.md to reflect the current state of the project, but I guess it shouldn't hold back the release

@yoobi
Copy link
Contributor Author

yoobi commented Apr 28, 2024

Oh right the readme, thanks for the release !

@yoobi
Copy link
Contributor Author

yoobi commented Apr 30, 2024

Hello, it's me again haha, looks like there was an issue in the release https://jitpack.io/com/github/anitrend/android-emojify/1.9.0/build.log

@wax911
Copy link
Member

wax911 commented Apr 30, 2024

Damn, wonder how I missed this 😆

@wax911
Copy link
Member

wax911 commented Apr 30, 2024

I think we can get rid of the :initializer module, at this point it's just an abstraction so it make with a mandatory property, we can update the README.md we'll be able to test the version without releasing a tag for jitpack @yoobi

@yoobi
Copy link
Contributor Author

yoobi commented May 1, 2024

Yes definitely the :initializer package isn't needed anymore and can be added by the user if they need it

@wax911
Copy link
Member

wax911 commented May 3, 2024

@yoobi try the latest artifact using the commit has as a version e.g.
implementation 'com.github.anitrend:android-emojify:1ff7613'

which correlates to 1ff7613 and #205

If all is fine I'll publish the tag for v1.9.1

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

I couldn't pull the library, it failed for some reason

Screenshot 2024-05-03 at 12 27 50

@wax911
Copy link
Member

wax911 commented May 3, 2024

I see jitpack hadn't built it yet 😆 I also provided a short commit hash than the actual. Try with 69882b15ad instead, jitpack will probably take a few minutes to build and publish to it's local repository https://jitpack.io/#AniTrend/android-emojify/69882b15ad

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

I think you're getting closer to the resolution of this

Running after_install command:
/script/buildit.sh: line 118: ${$AFTER_INSTALL_CMD}: bad substitution
Build tool exit code: 1
2024-05-03T12:17:36.10950606Z
Exit code: 1

@wax911
Copy link
Member

wax911 commented May 3, 2024

Not entirely sure what this is but I think it's complaining about https://github.com/AniTrend/android-emojify/blob/develop/jitpack.yml#L10 on after_install: ...

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

Does the after_install exists ? https://jitpack.io/docs/BUILDING/#custom-commands

@wax911
Copy link
Member

wax911 commented May 3, 2024

That's a good question, don't know where I got it from 😄 but regardless I might not need it, since that command that gets executed to copy the emoji.json into the test directory and jitpack shouldn't need to run unit tests (hopefully) #212 adding

@wax911
Copy link
Member

wax911 commented May 3, 2024

Attempting to rebuild on aforementioned PR: https://jitpack.io/#AniTrend/android-emojify/hotfix~jitpack-pipeline-SNAPSHOT

@wax911
Copy link
Member

wax911 commented May 3, 2024

Alright, replace the version with hotfix~jitpack-pipeline-SNAPSHOT and let me know if you encounter any issues. It seems like it builds successfully now https://jitpack.io/com/github/AniTrend/android-emojify/hotfix~jitpack-pipeline-a3ea763f7e-1/build.log

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

I was able to download the package in my project 🎉

@wax911
Copy link
Member

wax911 commented May 3, 2024

Great, I'll merge the PR and release 1.9.1 if there are no issues ⭐

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

It's me again... looks like there is an issue with the subpackages, Could not GET 'https://jitpack.io/com/github/anitrend/serializer-moshi/1.9.1/serializer-moshi-1.9.1.pom'. Received status code 401 from server: Unauthorized

@wax911
Copy link
Member

wax911 commented May 3, 2024

That URL doesn't look right 🤔 where did that url from because what you're looking for shouldn't exist e.g. https://jitpack.io/com/github/anitrend/android-emojify/moshi/1.9.1/moshi-1.9.1.pom <-- point to the packages published under com.github.anitrend:package:artifact where as what you posted would look for the equivalent to com.github.anitrend.serializer-moshi

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

I used com.github.anitrend:serializer-moshi:1.9.1 and the url to download the pom https://jitpack.io/com/github/anitrend/serializer-moshi/1.9.1/serializer-moshi-1.9.1.pom and it returns a 401 response

@wax911
Copy link
Member

wax911 commented May 3, 2024

That wouldn't resolve though because within the anitrend org you have multiple repos, so what you should be using is com.github.anitrend:android-emojify:moshi:1.9.1 (not sure why jitpack didn't group the serializer into a group), anyways you can confirm that the exact artifact id is on jitpack itself -> https://jitpack.io/#anitrend/android-emojify/1.9.1
image

@yoobi
Copy link
Contributor Author

yoobi commented May 3, 2024

Oh right, I copied/paste from the readme
Thanks for everything :)

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

Successfully merging a pull request may close this issue.

2 participants