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

feat: add animated emoji support #450

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheOneWithTheBraid
Copy link
Collaborator

  • implement animated emoji support in both HTML and Linkify message type
  • fix some missing font glyphs
  • trim message input

@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch 2 times, most recently from 51bae14 to 65b1628 Compare July 21, 2023 06:19
@krille-chan
Copy link
Owner

Can you clean up the commit history and make sure that every commit does only one thing? For example why do you change style: const TextStyle(fontFamily: 'RobotoMono'), to style: const TextStyle(fontFamily: 'Roboto Mono'),? Does it fix something? There is nothing in the commit message about it and what has this to do with animated emoji support?

@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch from 65b1628 to b0bd0d0 Compare July 23, 2023 13:39
@TheOneWithTheBraid
Copy link
Collaborator Author

Can you clean up the commit history and make sure that every commit does only one thing? For example why do you change style: const TextStyle(fontFamily: 'RobotoMono'), to style: const TextStyle(fontFamily: 'Roboto Mono'),? Does it fix something? There is nothing in the commit message about it and what has this to do with animated emoji support?

Yeah, sorry, the commit history completely broke from rebasing and migrating onto the new upsream. These commits fix what's stated in the pubspec.yaml, which is at least about the noto font directly related to emojis. Since it'd feel weird to only fix it for one of the two font families, I fixedit for roboto too - though it's actually not related to the emoji support.

Sorry again, the commit history was re-written many times, since also the upstream commit history was re-written and during cherry-picking in the commits onto the new upstream history, most of the commit messages were somehow breaking again.

If you want, I can take the time to restore them, but that'd be quite lots of manual work.

@TheOneWithTheBraid
Copy link
Collaborator Author

  • would therefore rather prefer to squash the messy history

@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch 4 times, most recently from 4f82cf0 to 9debf2b Compare July 25, 2023 06:17
@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch 2 times, most recently from 91a9f56 to 5f65044 Compare August 8, 2023 10:01
@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch from 5f65044 to d038e6c Compare August 26, 2023 11:03
@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch from d038e6c to d2bbd15 Compare September 16, 2023 15:19
@TheOneWithTheBraid
Copy link
Collaborator Author

Poke @krille-chan : Do you have any update on this Pull Request ?

@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch from d2bbd15 to e00874b Compare September 22, 2023 05:38
@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch from e00874b to 733c7a0 Compare October 2, 2023 16:27
@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch 4 times, most recently from a32e654 to 8a3f886 Compare November 12, 2023 18:57
@krille-chan
Copy link
Owner

Poke @krille-chan : Do you have any update on this Pull Request ?

As long as it does bring so much more stuff to load for the initial start of the web app I unfortunately see no way to merge this. :-/ It could be possible by not shipping this in the web-version if it works to fallback to normal emojis there

@TheOneWithTheBraid
Copy link
Collaborator Author

Poke @krille-chan : Do you have any update on this Pull Request ?

As long as it does bring so much more stuff to load for the initial start of the web app I unfortunately see no way to merge this. :-/ It could be possible by not shipping this in the web-version if it works to fallback to normal emojis there

What do you mean by this ? There are no new assets added or changed. Do you have concerns about loading the dart_animated_emoji package ? I could simply deffer it and only load it on demand. Would you prefer that ?

- implement animated emoji support in both HTML and Linkify message type
- fix some missing font glyphs
- trim message input

Signed-off-by: The one with the braid <[email protected]>
@TheOneWithTheBraid TheOneWithTheBraid force-pushed the braid/animated-emoji-github branch from 8a3f886 to dc8d77b Compare November 17, 2023 15:50
@basings basings mentioned this pull request Jan 23, 2024
@Henry-Hiles Henry-Hiles mentioned this pull request Feb 7, 2024
@MagicRB
Copy link

MagicRB commented May 5, 2024

Any updates on this? Animated emojis would be nice

@krille-chan
Copy link
Owner

Any updates on this? Animated emojis would be nice

just had no time for this yet. My concerns are that this delays the start-up on web for too long or loads too much from google servers (while the web app already loads emojis from google servers unfortunately)

@TheOneWithTheBraid
Copy link
Collaborator Author

Yes, that was exactly the issue. Basically we'd need to decide : Do we want super slow startup and no stuff from google servers (initial load +5MB), slow startup and only fallback glyphs from google servers (+0.3MB startup) or everything from Google servers and fast startup. Either options seems like a regression. Though I actually removed all handling of that case into the #451 so that we don't need to find a solution for this issue before merging this.

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