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

Patch text emojis to include utf8 word joiners #1035

Closed
wants to merge 1 commit into from

Conversation

Simon-Laux
Copy link
Member

To prevent them from being affected by line breaks.
I made a javascript script to make this patching process easier: https://gist.github.com/Simon-Laux/794ada18754eca1db8301d30c127e2a3

To prevent them from being affected by line breaks.
I made a javascript script to make this patching process easier: https://gist.github.com/Simon-Laux/794ada18754eca1db8301d30c127e2a3
@Simon-Laux
Copy link
Member Author

It shouldn't break anything, but keep in mind I did this edit in the web editor so I couldn't test if it still compiles.

@r10s
Copy link
Member

r10s commented Sep 25, 2019

tbh, i am a bit sceptical, i've seen too many problems with unicode specials, BOM and whatnot.

this stuff is processed through so many instances - i can easily imagine a smily is shown in some mua as :▊-▊) then. exchanging linebreak display problems to other display problems.

but it is not only display-wise: we support android down to version 4, and there are already lots of problems with unicode, i would not call for more troubles. quote form dc_wrapper.c: on KitKat a simple "SMILING FACE WITH SMILING EYES" (U+1F60A, UTF-8 F0 9F 98 8A) will let the app crash ... and this is only one example.

i do not say that we can never do that, but this would need more testing and evaluating.

but: unicode.org says to this character: confuse: none

@r10s r10s closed this Sep 25, 2019
@r10s
Copy link
Member

r10s commented Sep 25, 2019

one thing to add: i assume this is introduced on desktop? maybe we can evaluate if problems arise there and reopen the pr at a later point.

@Simon-Laux
Copy link
Member Author

we don't have text emojis in desktop yet.
https://en.wikipedia.org/wiki/Word_joiner is there since unicode 3, way before emojis were added (unicode 6)

@r10s
Copy link
Member

r10s commented Sep 25, 2019

well, old standards are no reason for things not to fail.

i think these word-joiners solve a pretty tiny problem with the potential to raise much larger ones.

might also be that there is no problem at all, though.

as said, maybe we can go for them later on, but then this would need some more exploration. and maybe not start on android with that, where older versions are known to have huge unicode problems.

@r10s r10s reopened this Sep 25, 2019
Copy link
Member

@r10s r10s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe closing was a bit hash. i just want to make sure this does not gets merged before further exploration, at least with some larger mua and old android versions.

but i do no see this as a high priority.

@cyBerta
Copy link
Contributor

cyBerta commented Oct 15, 2019

I 'll test that on Android 4.4 now

@cyBerta
Copy link
Contributor

cyBerta commented Oct 15, 2019

Hm, it seems not to break anything on an old device, but is also has no visible effect in comparison to the former implementation

@cyBerta
Copy link
Contributor

cyBerta commented Oct 15, 2019

linebreak2

s/int between/in between/

@r10s
Copy link
Member

r10s commented Oct 15, 2019

k, i think we close that for now. also talked with @Jikstra about the need and possible side-effects - as mentioned, they may be more on the other machines on other muas etc.

i think this is nothing we should target just now.

@r10s r10s closed this Oct 15, 2019
@Simon-Laux
Copy link
Member Author

Simon-Laux commented Oct 16, 2019

They are people that want to have it, another solution could be to adjust the input field, so that you can see that you're text emoji would break before sending it.

Also what can we do about muas that don't support Unicode?
Having the wordjoiner looks hacky, but I believe it's the right solution. Also its in unicode since way before emojis were added.

but is also has no visible effect

@cyBerta how should it break with no spaces in between? We could add zero width spaces in front to solve that I would Imagine...

Also the emoji you tested doesn't need the wordjoiners because it's glued together anyway a real example would be ☜(゚ヮ゚☜) which would break. I admit the wordjoiners could be used less to be more efficient (don't use them between chars that are glued together anyway).

see https://support.delta.chat/t/dont-brake-text-based-emoticons/374 for the forum discussion and why some folks want that.

CC @adbenitez

@r10s
Copy link
Member

r10s commented Oct 16, 2019

They are people that want to have it

sure, but this is only one perspective.

only precaution, of course, however, there are some serious concerns mentioned above wrt maintainability and potential problems with other messengers and clients, based upon known utf-8 problems in the past.

we have to do at least some testing before merging this pr. not with delta, but with other mua,.

and in delta-android, there are tons of android-utf-8 problems, just now at #1065
for this reason, i am not sure of. android is the best system to play around with this stuff (i know, desktop does not have text-emojis at all :)

as mentioned, we can figure that out these things at some points, but, tbh, i do not want to dive into that just now.

@r10s
Copy link
Member

r10s commented Oct 16, 2019

just did a quick duckduckgo'in:

this is a typical issue i am thinking about: SamsungInternet/support#3 - Samsung, Android 6 - here in the browsers, but this may easily cause problems somewhere else.

a5781b68-f391-11e6-8c0c-e94d227f8ebd

@adbenitez adbenitez deleted the Simon-Laux-patch-1 branch October 10, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants