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

Support unicode emojis and remove emojify.js #11032

Merged
merged 99 commits into from
Apr 28, 2020
Merged

Conversation

mrsdizzie
Copy link
Member

@mrsdizzie mrsdizzie commented Apr 9, 2020

This PR replaces all use of emojify.js and adds unicode emoji support to various areas of gitea.

This works in a few ways:

First it adds emoji parsing support into gitea itself. This allows us to

  • Render emojis from valid alias (:smile:)
  • Detect unicode emojis and let us put them in their own class with proper aria-labels and styling
  • Easily allow for custom "emoji"
  • Support all emoji rendering and features without javascript
  • Uses plain unicode and lets the system render in appropriate emoji font
  • Doesn't leave us relying on external sources for updates/fixes/features

That same list of emoji is also used to create a json file which replaces the part of emojify.js that populates the emoji search tribute.

For custom "emoji" it uses a pretty simple scheme of just looking for /emojis/img/name.png where name is something a user has put in the "allowed reactions" setting we already have. The gitea reaction that was previously hard coded into a forked copy of emojify.js is included and works as a custom reaction under this method.

The emoji data sourced here is from https://github.com/github/gemoji which is the gem library Github uses for their emoji rendering (and a data source for other sites). So we should be able to easily render any emoji and :alias: that Github can, removing any errors from migrated content. They also update it as well, so we can sync when there are new unicode emoji lists released.

I've included a slimmed down and slightly modified forked copy of https://github.com/knq/emoji to make up our own emoji module. The code is pretty straight forward and again allows us to have a lot of flexibility in what happens.

I had seen a few comments about performance in some of the other threads if we render this ourselves, but there doesn't seem to be any issue here. In a test it can parse, convert, and render 1,000 emojis inside of a large markdown table in about 100ms on my laptop (which is many more emojis than will ever be in any normal issue). This also prevents any flickering and other weirdness from using javascript to render some things while using go for others.

Includes the optional download of a fallback emoji webfont for small number of users who don't already have one installed.

Fixes: #9182
Fixes: #8974
Fixes: #8953
Fixes: #6628
Fixes: #5130

@silverwind
Copy link
Member

silverwind commented Apr 9, 2020

This sounds like a clearly better alternative then #11031. Maybe you want to port the Tribute extraction from my PR to yours, but it's also fine if I do that later. Your data source seems fine and more maintained than mine, albeit it's a bit smaller (35kB vs 40kB).

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 9, 2020
@mrsdizzie
Copy link
Member Author

I wanted to update our copy of tribute and was actually going to ask you to help with that after this -- I noticed some bugs with ours (like you can't delete) that seem to be fixed in newer versions. I'll look at how your PR does that (I don't like sticking all of that in footer template anyway)

@silverwind
Copy link
Member

silverwind commented Apr 9, 2020

Suggestions:

  • extract tribute to its own file in web_src/features/tribute.js similar to my PR.
  • import map from './path/to/json' to load the data directly into the webpack bundle instead of ajax.
  • maybe use my version of the tribute values function:
    values(query, cb) {
      const matches = [];
      for (const name of allNames) {
        if (name.startsWith(query)) {
          matches.push(name);
          if (matches.length > 5) break;
        }
      }
      cb(matches);
    },

Difference here is .startsWith which I find more useful than indexOf which can match too much on short strings.

@mrsdizzie
Copy link
Member Author

Thank you I am not familiar with javascript at all! I've added you as a collaborator if you want to push those changes, if not I can try to get to them in a bit

@silverwind
Copy link
Member

I'll have a look at extracting tribute tomorrow. I guess I would also move emoji.json to a better location, maybe the assets directory. My data source for reference.

@6543
Copy link
Member

6543 commented Apr 9, 2020

looks like emoji.json exist twice ? public/emoji.json and public/emoji/emoji.json

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Apr 10, 2020
@guillep2k guillep2k added the topic/ui Change the appearance of the Gitea UI label Apr 10, 2020
@silverwind
Copy link
Member

silverwind commented Apr 10, 2020

Rebased to fix issue references in orginal commit message. Maybe you can update original PR message with those Fixes lines so GitHub wil reference them correctly.

@lafriks
Copy link
Member

lafriks commented Apr 10, 2020

I think it's missing aliases for :), ;) etc

@silverwind
Copy link
Member

silverwind commented Apr 10, 2020

@lafriks those were previously disabled corresponding to this. +1 and -1 are covered by this set, btw.

@mrsdizzie
Copy link
Member Author

I think it's missing aliases for :), ;) etc

Those are emoticons which are technically different and often people want to use them as is as a plain text alternative to an emoji. I purposely didn't add support for those for that reason.

If you wanted to add a smile you can start typing :smile or insert the emoji normally

build/generate-emoji.go Outdated Show resolved Hide resolved
web_src/less/_base.less Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #11032 into master will increase coverage by 0.06%.
The diff coverage is 92.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11032      +/-   ##
==========================================
+ Coverage   43.48%   43.54%   +0.06%     
==========================================
  Files         597      598       +1     
  Lines       84708    84840     +132     
==========================================
+ Hits        36837    36947     +110     
- Misses      43335    43349      +14     
- Partials     4536     4544       +8     
Impacted Files Coverage Δ
modules/templates/helper.go 42.29% <50.00%> (+0.09%) ⬆️
modules/emoji/emoji.go 82.92% <82.92%> (ø)
modules/markup/html.go 81.88% <100.00%> (+2.28%) ⬆️
modules/markup/sanitizer.go 92.85% <100.00%> (+0.35%) ⬆️
modules/queue/workerpool.go 54.80% <0.00%> (-5.70%) ⬇️
services/pull/check.go 50.00% <0.00%> (-5.49%) ⬇️
modules/process/manager.go 74.69% <0.00%> (-3.62%) ⬇️
services/pull/patch.go 62.93% <0.00%> (-2.80%) ⬇️
modules/git/command.go 86.95% <0.00%> (-2.61%) ⬇️
services/pull/temp_repo.go 29.05% <0.00%> (-2.57%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7430221...4cf129f. Read the comment docs.

@mrsdizzie
Copy link
Member Author

looks like emoji.json exist twice ? public/emoji.json and public/emoji/emoji.json

fixed

@silverwind
Copy link
Member

silverwind commented Apr 10, 2020

So this 394 kB (ungzipped) emoji.json seems a bit on the heavy side for frontend inclusion. Would you mind outputting a lightweight name to unicode mapping format for the frontent (pretty much like https://unpkg.com/emojilib@2.4.0/simplemap.json)?

@mrsdizzie
Copy link
Member Author

So this 394 kB (ungzipped) seems a bit on the heavy side for frontend inclusion. Would you mind building a lightweight name to unicode mapping format for the frontent (pretty much like https://unpkg.com/emojilib@2.4.0/simplemap.json)?

Sure I'll make something like that that, but for my own knowledge: It is 35kb gzipped, which I think anybody who cares about kb of size should enabled -- unless I'm misunderstanding and the size issue is related to something else about performance? Does it have some noticeable effect after it is loaded? I'm just not familiar with that area at all.

@silverwind
Copy link
Member

Yeah, it gzips well, but the reduced version is 40kb without and 12kB with gzip.

It's not a big deal but I'd still prefer the smaller version because every byte matters, especially for slow connections. With a active service worker (the default), the content should only be downloaded once every 24h at most, so the price is not paid every page load for the common case.

Do you forsee that we will need the additional data on the frontend? If so, keep it.

@mrsdizzie
Copy link
Member Author

Yeah, it gzips well, but the reduced version is 40kb without and 12kB with gzip.

It's not a big deal but I'd still prefer the smaller version because every byte matters, especially for slow connections. With a active service worker (the default), the content should only be downloaded once every 24h at most, so the price is not paid every page load for the common case.

Do you forsee that we will need the additional data on the frontend? If so, keep it.

In thinking about it I don't see us actually doing anything with most of the fields in general (category, tags, unicode and ios version, skin_tones bool). So good suggestion I've just removed those.

It is now 27kb gzipped. It would be 18kb without the aliases but I'd rather have them to search and keep the json and what go uses in sync than save the additional 9kb.

Since it loads after the page has loaded it shouldn't visually effect or slow rendering on a slow connection either (I tried on the chrome 3g simulator and says it takes 750ms to load in the background after the page is rendered).

custom/conf/app.ini.sample Outdated Show resolved Hide resolved
modules/markup/html.go Outdated Show resolved Hide resolved
templates/repo/issue/view_content/add_reaction.tmpl Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Apr 11, 2020

@mrsdizzie we realy need a fallback:

Screenshot at 2020-04-11 02-44-04
(Crome latest - on linux)

@mrsdizzie
Copy link
Member Author

You don't have a good emoji font on your system?

@6543
Copy link
Member

6543 commented Apr 11, 2020

You don't have a good emoji font on your system?

I dont think we should require that - and no I didn't had any thoughts in installing a font for emojis jet

@6543
Copy link
Member

6543 commented Apr 11, 2020

custom emoji not renderd on review comment:

Screenshot at 2020-04-11 03-04-24

mrsdizzie added a commit to mrsdizzie/gitea that referenced this pull request Apr 28, 2020
…14 in git env

As seen in trouble shooting go-gitea#11032 the new feature of Go 1.14 is causing several second delays in startup in certain situations. Debugging shows it spending several seconds handling SIGURG commands during init:

```
6922:04:51.984234 trace init() ./modules/queue/unique_queue_wrapped.go
remote: ) = 69 <0.000012>
remote: [pid 15984] 22:04:51 write(1, "\ttime taken: 236.761\302\265s\n\n", 25    time taken: 236.761µs
remote:
remote: ) = 25 <0.000011>
remote: [pid 15984] 22:04:51 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
remote: [pid 15984] 22:04:52 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
remote: [pid 15984] 22:04:52 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
```

This causes up to 20 seconds added to a push in some cases as it happens for each call of the gitea hook command. This is likely the cause of go-gitea#10661 as well and would start to effect users once we release 1.12 which would be the first release compiled with Go 1.14. I suspect this is just a slight issue with the upstream implementatation as there have been a few very similar bugs fixed and reported:

 golang/go#37741
 golang/go#37942

We should revisit this in the future and see if a newer version of Go has solved it, but for now disable this option in the environment that gitea hook runs in to avoid it.
techknowlogick pushed a commit that referenced this pull request Apr 28, 2020
…14 in git env (#11237)

As seen in trouble shooting #11032 the new feature of Go 1.14 is causing several second delays in startup in certain situations. Debugging shows it spending several seconds handling SIGURG commands during init:

```
6922:04:51.984234 trace init() ./modules/queue/unique_queue_wrapped.go
remote: ) = 69 <0.000012>
remote: [pid 15984] 22:04:51 write(1, "\ttime taken: 236.761\302\265s\n\n", 25    time taken: 236.761µs
remote:
remote: ) = 25 <0.000011>
remote: [pid 15984] 22:04:51 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
remote: [pid 15984] 22:04:52 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
remote: [pid 15984] 22:04:52 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
```

This causes up to 20 seconds added to a push in some cases as it happens for each call of the gitea hook command. This is likely the cause of #10661 as well and would start to effect users once we release 1.12 which would be the first release compiled with Go 1.14. I suspect this is just a slight issue with the upstream implementatation as there have been a few very similar bugs fixed and reported:

 golang/go#37741
 golang/go#37942

We should revisit this in the future and see if a newer version of Go has solved it, but for now disable this option in the environment that gitea hook runs in to avoid it.
@guillep2k guillep2k removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Apr 28, 2020
@guillep2k
Copy link
Member

Ping LG-TM!

@guillep2k guillep2k merged commit 4563eb8 into go-gitea:master Apr 28, 2020
@lafriks lafriks added the type/enhancement An improvement of existing functionality label May 17, 2020
@lafriks lafriks added this to the 1.12.0 milestone May 17, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
…14 in git env (go-gitea#11237)

As seen in trouble shooting go-gitea#11032 the new feature of Go 1.14 is causing several second delays in startup in certain situations. Debugging shows it spending several seconds handling SIGURG commands during init:

```
6922:04:51.984234 trace init() ./modules/queue/unique_queue_wrapped.go
remote: ) = 69 <0.000012>
remote: [pid 15984] 22:04:51 write(1, "\ttime taken: 236.761\302\265s\n\n", 25    time taken: 236.761µs
remote:
remote: ) = 25 <0.000011>
remote: [pid 15984] 22:04:51 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
remote: [pid 15984] 22:04:52 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
remote: [pid 15984] 22:04:52 --- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=15984, si_uid=0} ---
```

This causes up to 20 seconds added to a push in some cases as it happens for each call of the gitea hook command. This is likely the cause of go-gitea#10661 as well and would start to effect users once we release 1.12 which would be the first release compiled with Go 1.14. I suspect this is just a slight issue with the upstream implementatation as there have been a few very similar bugs fixed and reported:

 golang/go#37741
 golang/go#37942

We should revisit this in the future and see if a newer version of Go has solved it, but for now disable this option in the environment that gitea hook runs in to avoid it.
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Support unicode emojis and remove emojify.js

This PR replaces all use of emojify.js and adds unicode emoji support to various areas of gitea.

This works in a few ways:

First it adds emoji parsing support into gitea itself. This allows us to

 * Render emojis from valid alias (:smile:)
 * Detect unicode emojis and let us put them in their own class with proper aria-labels and styling
 * Easily allow for custom "emoji"
 * Support all emoji rendering and features without javascript
 * Uses plain unicode and lets the system render in appropriate emoji font
 * Doesn't leave us relying on external sources for updates/fixes/features

That same list of emoji is also used to create a json file which replaces the part of emojify.js that populates the emoji search tribute. This file is about 35KB with GZIP turned on and I've set it to load after the page renders to not hinder page load time (and this removes loading emojify.js also)

For custom "emoji" it uses a pretty simple scheme of just looking for /emojis/img/name.png where name is something a user has put in the "allowed reactions" setting we already have. The gitea reaction that was previously hard coded into a forked copy of emojify.js is included and works as a custom reaction under this method.

The emoji data sourced here is from https://github.com/github/gemoji which is the gem library Github uses for their emoji rendering (and a data source for other sites). So we should be able to easily render any emoji and :alias: that Github can, removing any errors from migrated content. They also update it as well, so we can sync when there are new unicode emoji lists released.

I've included a slimmed down and slightly modified forked copy of https://github.com/knq/emoji to make up our own emoji module. The code is pretty straight forward and again allows us to have a lot of flexibility in what happens.

I had seen a few comments about performance in some of the other threads if we render this ourselves, but there doesn't seem to be any issue here. In a test it can parse, convert, and render 1,000 emojis inside of a large markdown table in about 100ms on my laptop (which is many more emojis than will ever be in any normal issue). This also prevents any flickering and other weirdness from using javascript to render some things while using go for others.

Not included here are image fall back URLS. I don't really think they are necessary for anything new being written in 2020. However, managing the emoji ourselves would allow us to add these as a feature later on if it seems necessary.

Fixes: go-gitea#9182
Fixes: go-gitea#8974
Fixes: go-gitea#8953
Fixes: go-gitea#6628
Fixes: go-gitea#5130

* add new shared function emojiHTML

* don't increase emoji size in issue title

* Update templates/repo/issue/view_content/add_reaction.tmpl

Co-Authored-By: 6543 <6543@obermui.de>

* Support for emoji rendering in various templates

* Render code and review comments as they should be

* Better way to handle mail subjects

* insert unicode from tribute selection

* Add template helper for plain text when needed

* Use existing replace function I forgot about

* Don't include emoji greater than Unicode Version 12

Only include emoji and aliases in JSON

* Update build/generate-emoji.go

* Tweak regex slightly to really match everything including random invisible characters. Run tests for every emoji we have

* final updates

* code review

* code review

* hard code gitea custom emoji to match previous behavior

* Update .eslintrc

Co-Authored-By: silverwind <me@silverwind.io>

* disable preempt

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
10 participants