ecoji icon indicating copy to clipboard operation
ecoji copied to clipboard

Ecoji version 2

Open keith-turner opened this issue 2 years ago • 1 comments

After a long time, I think the work started by @robindiddams in #29 on Ecoji V2 is complete. I am creating this PR to merge that work in and once its merged, Ecoji V2 will be considered finalized. I plan to let the PR sit for at least 30 days and would welcome any review. If anyone is interested in looking at the changes and would like more time to review, just comment on this PR.

Ecoji V2 has an exciting new set of emojis based on feedback in #27 and #28. Ecoji v2 uses the Emoji 14 standard. To see what emojis were replaced between V1 and V2 look at :

https://github.com/keith-turner/ecoji/blob/ecojiv2/docs/emojis.md

For all the folks (@AntonMeep, @netvl, @abock, @Rayne, @mecforlove, @robindiddams) who created ecoji implementations in other languages in the past, if any of you have time to try to update to ecoji v2 I would like to hear of any questions or concerns you have when attempting that. Information on implementing ecoji V2 can be found at :

https://github.com/keith-turner/ecoji/blob/ecojiv2/docs/encoding.md

Information on testing a new implementation can be found at :

https://github.com/keith-turner/ecoji/tree/ecojiv2/test_scripts

keith-turner avatar Sep 04 '22 15:09 keith-turner

@dcow tagging you here in case you did not see #28 being tagged earlier. Your feedback was very useful for improving Ecoji V2.

keith-turner avatar Sep 10 '22 14:09 keith-turner

Hi,

I just finished implementing a Ecoji converter for BaseEx, which is a JavaScript library with many different base converters. Ecoji is now part of my 0.5.0-branch, the source code for Ecoji can be found here.

I just wanted to give some feedback for the version 2 implementation (I actually implemented both versions at the same time). I did not noticed much of a difference implementing version 1 vs version 2. It was a little challenging to accept both versions for decoding, also concatenated Ecoji strings (which is not part of any other converter in the library). But in other regards it worked pretty straight forward.

So, thanks for the great work. This was fun.

UmamiAppearance avatar Sep 28 '22 07:09 UmamiAppearance

I just finished implementing a Ecoji converter for BaseEx

@UmamiAppearance that sounds really neat. I have not had a chance to look at it yet, but I will in the next few days. I don't know javascript very well. A long time ago someone tried to create Ecoji javascript impl and when I reviewed it it did not handle binary data correctly. Curious if you have tried encoding and decoding random binary data.

Since there was no javascript version of ecoji when I created ecoji.io I resorted to compiling the Go code to wasm and calling that from javascript. That worked surprisingly well.

keith-turner avatar Oct 02 '22 19:10 keith-turner

You've got to be warned. I am using a generic method for base conversions, not the algorithm you use. Basically it calculates one integer according to the base (in this case a 2^10 int) which gets divided until the remainder is below or equal to the radix. This is what "super" calls at the end of en- and decoding are triggering. So, no reason to dig deeper unless you want to.

I resorted to compiling the Go code to wasm and calling that from javascript.

That is so cool. But you may consider to use BaseEx for this pupose 😁.

Edit: Yes, encoding binary data works pretty well. The default test I run via npm test is generating, en- and decoding binary data among other things.

UmamiAppearance avatar Oct 02 '22 22:10 UmamiAppearance

been a long time coming, excited to get the swift port up to date

also: thank you for keeping ✨ in the spec ❤️‍🔥

robindiddams avatar Oct 05 '22 18:10 robindiddams

It looks like I have to change my implementation to support concatenated Ecoji strings. What is the benefit or use case for concatenating Ecoji strings? ~~For instance, data/concat_v2_1.plaind can't be decoded currently on my end.~~

EDIT Okay, I've hacked a solution that is working with the CLI. The remaining question is now, whether functions working with strings instead of streams should also support concatenated Ecoji messages.

Rayne avatar Oct 06 '22 21:10 Rayne

EDIT Okay, I've hacked a solution that is working with the CLI. The remaining question is now, whether functions working with strings instead of streams should also support concatenated Ecoji messages.

@Rayne I did for the go version. i think its good to be consistent. Is your code pushed to GH?

keith-turner avatar Oct 09 '22 21:10 keith-turner

@Rayne I did for the go version. i think its good to be consistent. Is your code pushed to GH?

The working hack is not added. I will refactor the library to properly support concatenated messages.

Rayne avatar Oct 10 '22 20:10 Rayne

Hi,

I am almost done with my next release for BaseEx. I'd like to merge next week or so. It includes Ecoji, also with version 2. But it just doesn't feel right to to that before the original is available with version 2...

How is the progress?

Cheers.

UmamiAppearance avatar Nov 18 '22 10:11 UmamiAppearance

@UmamiAppearance I think this branch is in a good state to merge. I want to look over it one more time. I will do that on Nov 27 and merge it then if I don't see an serious problems. I was thinking of tweeting about Ecoji 2 when i do merge it, but who knows if twitter will exist by the end of next week.

keith-turner avatar Nov 18 '22 10:11 keith-turner

OK, great.

(I once had to develop a twitter like social media with Django. It was just an exercise, but maybe I can go online in case twitter breaks. It is called PhonyExpress™, a tweet is a whinny. You'll get used to it. ;)

UmamiAppearance avatar Nov 18 '22 11:11 UmamiAppearance

Ecoji V2 is live. Thanks everyone!

keith-turner avatar Nov 27 '22 21:11 keith-turner

Hi,

first congratulations!

I have an issue though. Your freshly inserted tests fail (or don't fail, when they should) on my side, this is due to a different handling of newlines. In the specs you write:

Decoding data ignores all new lines, including windows new lines. So decoding should ignore \n and \r\n

Well this is exactly what my decoder does, or to be more precise: it removes any kind of whitespace before dealing with the input. Now you introduced invalid or bad newlines, which is not ignoring newlines for my understanding. Can you explain the intention?

Best regards.

UmamiAppearance avatar Nov 28 '22 08:11 UmamiAppearance

Well this is exactly what my decoder does, or to be more precise: it removes any kind of whitespace before dealing with the input. Now you introduced invalid or bad newlines, which is not ignoring newlines for my understanding. Can you explain the intention?

My thinking was that if a \r is seen then a \n must be seen after it. That is how I wrote the go code, so I wrote test to cover that code. Maybe that's a bit too strict. I could certainly relax that and cut a 2.0.1. Relaxing that would greatly simplify the go code.

keith-turner avatar Nov 28 '22 22:11 keith-turner

Okay, now I get it.

I could certainly relax that and cut a 2.0.1. Relaxing that would greatly simplify the go code.

Makes sense in my opinion. I don't see a scenario, where a wrong (flipped windows) newline exists. Maybe by copying and pasting, but that doesn't hurt the decoding process, right?

UmamiAppearance avatar Nov 28 '22 22:11 UmamiAppearance

Makes sense in my opinion. I don't see a scenario, where a wrong (flipped windows) newline exists. Maybe by copying and pasting, but that doesn't hurt the decoding process, right?

Yeah, it will not hurt the correctness of decoding. I did some quick research when making the change, it seems most modern systems are either \n or \r\n. There was an ancient version of MacOs that only used \r it seems, but Mac switched away from that.

keith-turner avatar Nov 28 '22 23:11 keith-turner

@UmamiAppearance

I pushed a change to relax the new line handling and removed the test for the stricter handling

keith-turner avatar Nov 28 '22 23:11 keith-turner

That's great news, because it means no more work on my side. ;).

You'll find a pull request for the JavaScript version tomorrow.

UmamiAppearance avatar Nov 28 '22 23:11 UmamiAppearance

@dcow tagging you here in case you did not see #28 being tagged earlier. Your feedback was very useful for improving Ecoji V2.

Thanks for the hard work. I have seen this but, unfortunately, we abandoned ecoji for our product use case because it's simply not possible to have users type these things in in the way we were hoping (that was their feedback, at least, which we understand). If we revisited the idea, it would have to be with a reduced alphabet variant of ecoji (something like ecoji 256 or 128, perhaps, that's biased toward using unambiguous emoji that can be verbally communicated easily and show up readily accessibly across various common input methods like phone keyboards). If we do, I'd be happy to work on incorporating a reduced alphabet variant, but until then keep up the good work. The v2 improvements are much welcome in any case.

dcow avatar Dec 02 '22 23:12 dcow