Custom emotes

[nmscode]

Checklist

• [ ] Tests written for new code (and old code if feasible)
• [x] Linter and other CI checks pass
• [ ] Sign-off given on the changes (see CONTRIBUTING.md)

Spec proposal Maybe could be added to labs as a temporary measure until the team starts working on it? Needs some additions (encryption/decryption of the images). I tested by building it.

Can be accessed on https://emotes.operation.chat/ Type: enhancement More details in this comment: https://github.com/vector-im/element-meta/issues/339#issuecomment-1213520411

---

Here's what your changelog entry will look like:

✨ Features

• Custom emotes (#9240). Contributed by @nmscode.

[robintown]

Thank you for contributing! You appear to have somehow commited the entire contents of the react-sdk as a brand new repository - we will be unable to review this until that is fixed. We also require all changes to be signed off, per the checklist.

It's also important to note that there are other PRs adding custom emoji functionality to the app, such as https://github.com/matrix-org/matrix-react-sdk/pull/8087, that this would likely conflict with. Additionally, any changes to the Matrix spec, such as a hypothetical m.room.emotes event type as is used here, need to go through the spec proposal process first. There are already several spec proposals for custom emoji out there, such as MSC2545, though the approach to custom emoji as a whole has not had much attention from the spec core team yet, and is very much subject to change.

[nmscode]

I apologize for the issue with the repository. I have rebased it and I think that particular issue is fixed now.

[nmscode]

Thank you for the contribution, however this doesn't appear to be in a reviewable state at the moment: there's merge conflict chunks, incorrect indentation, linter errors, etc which all need to be addressed before we can take a closer look at this.

Thank you for pointing those issues out. I have fixed the linting issues as well as some other issues. It seems to pass all the tests except the i18n. I added some phrases such as "Upload Emote" to the strings which might be related to this issue. I think the rest of the code is mostly formatted properly now.

[t3chguy]

@nmscode as per the docs, run yarn i18n to re-generate the internationalisation files

[nmscode]

@nmscode as per the docs, run yarn i18n to re-generate the internationalisation files

I have tried this with and without my changes to en_EN.json but it still does not pass the i18n test.

[nmscode]

I reran it again and it is fine now.

[nmscode]

@robintown @turt2live Here is the spec change proposal https://github.com/matrix-org/matrix-spec-proposals/pull/3892

[z411]

I tried it but it's not compatible with the already existing emotes in existing channels, mostly because other clients are using this proposal to implement their emote packs (as mentioned earlier):

https://github.com/matrix-org/matrix-spec-proposals/pull/2545

[JiffyCat]

I've done some code format/style cleanup for @nmscode PR. It should make reviewing easier when looking at the diffs.

@nmscode is working on resolving the last blocker for dangerouslySetInnerHTML but this PR should be ready for another review.

[nmscode]

Thank you @JiffyCat for the formatting/style fixes.

Some points from the last review I wanted to address:

To alleviate concerns about conflicting MSCs I have added a compatibility option which makes the current implementation compatible with MSC2545. Thank you @z411 for helping test compatibility mode and identifying bugs with it. For the copyright comments, several files already in develop have the same incorrect copyright, which is also why my files had the incorrect one since I copied those. I was not entirely sure how to update this. I think it would make more sense to fix all the copyrights in a separate PR. For the dangerouslySetInnerHTML issue I added some comments about it to explain it's use in that context better The other points have been fixed in the code

I agree that this feature should go to labs first as there is still work/testing to be done. I think having it in labs would allow those who are eagerly waiting for this feature to have basic custom emote functionality while still allowing the dev team to work on it at their own pace. If additional code is needed to make it a labs feature I can assist with that.

[t3chguy]

@nmscode your code doesn't build nor pass the linters and tests.

[nmscode]

@nmscode your code doesn't build nor pass the linters and tests.

Sorry about that. The issues have been fixed. It should build and pass most linters/tests.

[t3chguy]

@nmscode you still have a lot of failures, https://github.com/matrix-org/matrix-react-sdk/actions/runs/5273469521/jobs/9541914942?pr=9240 points out you're using maps wrong. Maps don't use [key] accesses.

[JiffyCat]

Linters and tests all pass now.

[t3chguy]

@nmscode CI is still very unhappy with this change

[nmscode]

@t3chguy fixed errors with strict type checker

[t3chguy]

CI won't run due to merge conflicts

[nmscode]

fixed conflict and merged

[t3chguy]

You're certainly getting there, Sonar is complaining about the lack of test coverage for the changes

[nmscode]

How do I check coverage locally for my changes when I am writing tests for them? I tried yarn test --coverage it gave me the report for the entire repo. The --collectCoverageFrom option also did not seem to work.

[t3chguy]

@nmscode seems like you can specify a from ref https://stackoverflow.com/a/68510498

[JiffyCat]

Running yarn jest --coverage --changedSince=upstream/develop gives us:

=============================== Coverage summary ===============================
Statements   : 63.42% ( 2272/3582 )
Branches     : 54.67% ( 1286/2352 )
Functions    : 67.4% ( 397/589 )
Lines        : 64.6% ( 2183/3379 )

However, we want what Sonar does for coverage report for new code (24.3% Coverage on New Code (is less than 80%)). Is there a way to emulate that? Otherwise, it's really hard to see our progress with tests.

[t3chguy]

Doubt it, what Sonar does is compare the LCOV reports between the base ref and head ref, maybe there's some diff utility but no idea about that. Your best bet is to open the sonar report, look at the lines its complaining about, and write tests which exercise them.

[nmscode]

@t3chguy I added some tests to increase coverage

[JiffyCat]

@t3chguy Would you mind removing the blocked label from our PR? We have slowed down the rate of our commits and will not need as many CI runs. We are only pushing to check sonar coverage at this point.

[t3chguy]

That doesn't affect the CI beyond the single blocking job, that's just the Github repo's security settings which apply to all new contributors unfortunately

[JiffyCat]

@t3chguy I think there's an issue with the Jest CI. Jest passes locally with the current changeset. Can you rerun the failed Jest job?

[t3chguy]

@JiffyCat yes it is a flaky test as listed in https://github.com/vector-im/element-web/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3AZ-Flaky-Test

[nmscode]

@t3chguy I pushed the hopefully final tests needed to pass sonar

[nmscode]

@t3chguy I was not aware of the new check. Pushed a fix it should pass now