Closed yoobi closed 8 months ago
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.
The idea is to split out the library into:
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.
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)interface IEmojiDeserializer {
fun decodeFromStream(inputStream: InputStream): List<IEmoji>
}
import java.io.IOException
abstract class AbstractEmojiInitializer : Initializer
@Throws(IOException::class, SerializationException::class)
fun initEmojiData(
assetManager: AssetManager,
path: String = DEFAULT_PATH,
): List<IEmoji> {
return assetManager.open(path).use { inputStream ->
serializer.decodeFromStream(inputStream)
}
}
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)
?
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
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 thusunicode
,htmlDec
andhtmlHex
needs a refacto.I'm not sure if I understood correctly but you wish to make
:initializer
a new module withandroidx.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 withEmojiInitializer().onCreate(context)
?
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)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 methodinitializer
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 serializerinitializeProperties
into lazy field, the only issue is that the loop is being run twice, one for htmlDec
and one for htmlHex
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?
True a simple how to
in the README would be enough for users who wants to use runtime-initializer
Great, I'm happy with the PR and will suggest/ask some small questions on it ⭐
Hello, do you have an estimated date for 1.9.0 release ?
Hey @yoobi 😆 not going to lie but I had completely forgot to publish the release
Hahaha no problem :p
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
Oh right the readme, thanks for the release !
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
Damn, wonder how I missed this 😆
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
Yes definitely the :initializer
package isn't needed anymore and can be added by the user if they need it
@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
I couldn't pull the library, it failed for some reason
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
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
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: ...
Does the after_install
exists ? https://jitpack.io/docs/BUILDING/#custom-commands
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) https://github.com/AniTrend/android-emojify/pull/212 adding
Attempting to rebuild on aforementioned PR: https://jitpack.io/#AniTrend/android-emojify/hotfix~jitpack-pipeline-SNAPSHOT
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
I was able to download the package in my project 🎉
Great, I'll merge the PR and release 1.9.1
if there are no issues ⭐
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
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
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
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
Oh right, I copied/paste from the readme Thanks for everything :)
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