realm_emoji: Show still image of animated emoji in right sidebar when not hovered.

[Riken-Shah]

Fixes #19521

Testing plan:
Tested localy.

GIFs or screenshots:

Screen.Recording.2021-08-12.at.3.28.47.PM.mov

[zulipbot]

Hello @zulip/server-emoji, @zulip/server-sidebars members, this pull request was labeled with the "area: emoji", "area: right-sidebar" labels, so you may want to check it out!

[Riken-Shah]

This PR still doesn't support animated png yet.

[timabbott]

Let's open animated PNG as a separate issue, since we don't handle those in general with custom emoji.

[timabbott]

I pushed back to this PR with the changes below, which I believe address the items noted above. (Also did some copy-editing on the API documentation and renumbered the migration).

diff --git a/zerver/lib/upload.py b/zerver/lib/upload.py
index 9502a6c8d8..512cf09f90 100644
--- a/zerver/lib/upload.py
+++ b/zerver/lib/upload.py
@@ -11,7 +11,7 @@ import unicodedata
 import urllib
 from datetime import timedelta
 from mimetypes import guess_extension, guess_type
-from typing import IO, Any, Callable, Optional, Tuple, Union, cast
+from typing import IO, Any, Callable, Optional, Tuple
 
 import boto3
 import botocore
@@ -168,7 +168,7 @@ def resize_gif(im: GifImageFile, size: int = DEFAULT_EMOJI_SIZE) -> bytes:
 
 def resize_emoji(
     image_data: bytes, size: int = DEFAULT_EMOJI_SIZE
-) -> Tuple[bytes, bool, Union[bytes, None]]:
+) -> Tuple[bytes, bool, Optional[bytes]]:
     # This function returns three values:
     # 1) Emoji image data.
     # 2) If emoji is gif i.e animated.
@@ -189,21 +189,23 @@ def resize_emoji(
                 or len(image_data) > MAX_EMOJI_GIF_FILE_SIZE_BYTES  # filesize too large
             )
 
-            # Generating still image.
-            im.seek(1)
+            # Generate a still image from the first frame.  Since
+            # we're converting the format to PNG anyway, we resize unconditionally.
             still_image = im.copy()
-
-            if should_resize:
-                image_data = resize_gif(im, size)
-                still_image = ImageOps.exif_transpose(still_image)
-                still_image = ImageOps.fit(still_image, (size, size), Image.ANTIALIAS)
-
+            still_image.seek(0)
+            still_image = ImageOps.exif_transpose(still_image)
+            still_image = ImageOps.fit(still_image, (size, size), Image.ANTIALIAS)
             out = io.BytesIO()
             still_image.save(out, format="PNG")
             still_image_data = out.getvalue()
 
+            if should_resize:
+                image_data = resize_gif(im, size)
+
             return image_data, True, still_image_data
         else:
+            # Note that this is essentially duplicated in the
+            # still_image code path, above.
             im = ImageOps.exif_transpose(im)
             im = ImageOps.fit(im, (size, size), Image.ANTIALIAS)
             out = io.BytesIO()
@@ -699,18 +701,21 @@ class S3UploadBackend(ZulipUploadBackend):
             resized_image_data,
         )
         if is_animated:
+            assert still_image_data is not None
             upload_image_to_s3(
                 self.avatar_bucket,
                 emoji_path.replace(".gif", "-still.png"),
                 "image/png",
                 user_profile,
-                cast(bytes, still_image_data),
+                still_image_data,
             )
 
         return is_animated
 
     def get_emoji_url(self, emoji_file_name: str, realm_id: int, still: bool = False) -> str:
         if still:
+            # We currently only support animated GIFs.
+            assert emoji_file_name.endswith(".gif")
             emoji_path = RealmEmoji.STILL_PATH_ID_TEMPLATE.format(
                 realm_id=realm_id,
                 emoji_filename_without_extension=os.path.splitext(emoji_file_name)[0],
@@ -955,16 +960,14 @@ class LocalUploadBackend(ZulipUploadBackend):
         write_local_file("avatars", ".".join((emoji_path, "original")), image_data)
         write_local_file("avatars", emoji_path, resized_image_data)
         if is_animated:
-            write_local_file(
-                "avatars", emoji_path.replace(".gif", "-still.png"), cast(bytes, still_image_data)
-            )
+            assert still_image_data is not None
+            write_local_file("avatars", emoji_path.replace(".gif", "-still.png"), still_image_data)
         return is_animated
 
     def get_emoji_url(self, emoji_file_name: str, realm_id: int, still: bool = False) -> str:
         if still:
-            if not emoji_file_name.endswith(".gif"):
-                return ""
-
+            # We currently only support animated GIFs.
+            assert emoji_file_name.endswith(".gif")
             return os.path.join(
                 "/user_avatars",
                 RealmEmoji.STILL_PATH_ID_TEMPLATE.format(
diff --git a/zerver/models.py b/zerver/models.py
index 642a0f0377..4297ca8c0e 100644
--- a/zerver/models.py
+++ b/zerver/models.py
@@ -970,6 +970,7 @@ class RealmEmoji(models.Model):
     # The basename of the custom emoji's filename; see PATH_ID_TEMPLATE for the full path.
     file_name: Optional[str] = models.TextField(db_index=True, null=True, blank=True)
 
+    # Whether this custom emoji is an animated image.
     is_animated: bool = models.BooleanField(default=False)
 
     deactivated: bool = models.BooleanField(default=False)
@@ -995,24 +996,26 @@ def get_realm_emoji_dicts(
         if realm_emoji.author:
             author_id = realm_emoji.author_id
         emoji_url = get_emoji_url(realm_emoji.file_name, realm_emoji.realm_id)
+
+        emoji_dict = dict(
+            id=str(realm_emoji.id),
+            name=realm_emoji.name,
+            source_url=emoji_url,
+            deactivated=realm_emoji.deactivated,
+            author_id=author_id,
+        )
+
         if realm_emoji.is_animated:
-            d[str(realm_emoji.id)] = dict(
-                id=str(realm_emoji.id),
-                name=realm_emoji.name,
-                source_url=emoji_url,
-                still_url=get_emoji_url(realm_emoji.file_name, realm_emoji.realm_id, still=True),
-                deactivated=realm_emoji.deactivated,
-                author_id=author_id,
-            )
-        else:
-            d[str(realm_emoji.id)] = dict(
-                id=str(realm_emoji.id),
-                name=realm_emoji.name,
-                source_url=emoji_url,
-                deactivated=realm_emoji.deactivated,
-                author_id=author_id,
+            # For animated emoji, we include still_url with a static
+            # version of the image, so that clients can display the
+            # emoji in a less distracting (not animated) fashion when
+            # desired.
+            emoji_dict["still_url"] = (
+                get_emoji_url(realm_emoji.file_name, realm_emoji.realm_id, still=True),
             )
 
+        d[str(realm_emoji.id)] = emoji_dict
+
     return d
 
 
diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py
index 051b6f1aa4..64042aaae8 100644
--- a/zerver/tests/test_upload.py
+++ b/zerver/tests/test_upload.py
@@ -6,7 +6,6 @@ import shutil
 import time
 import urllib
 from io import StringIO
-from typing import cast
 from unittest import mock
 from unittest.mock import patch
 
@@ -1278,7 +1277,8 @@ class EmojiTest(UploadSerializeMixin, ZulipTestCase):
         im = Image.open(io.BytesIO(resized_img_data))
         self.assertEqual((50, 50), im.size)
         self.assertTrue(is_animated)
-        still_image = Image.open(io.BytesIO(cast(bytes, still_img_data)))
+        assert still_img_data is not None
+        still_image = Image.open(io.BytesIO(still_img_data))
         self.assertEqual((50, 50), still_image.size)
 
         # Test corrupt image exception
@@ -1297,7 +1297,7 @@ class EmojiTest(UploadSerializeMixin, ZulipTestCase):
             im = Image.open(io.BytesIO(resized_img_data))
             self.assertEqual((50, 50), im.size)
             self.assertTrue(is_animated)
-            still_image = Image.open(io.BytesIO(cast(bytes, still_img_data)))
+            still_image = Image.open(io.BytesIO(still_img_data))
             self.assertEqual((50, 50), still_image.size)
 
         # Test an image file larger than max is resized
@@ -1310,7 +1310,8 @@ class EmojiTest(UploadSerializeMixin, ZulipTestCase):
             im = Image.open(io.BytesIO(resized_img_data))
             self.assertEqual((50, 50), im.size)
             self.assertTrue(is_animated)
-            still_image = Image.open(io.BytesIO(cast(bytes, still_img_data)))
+            assert still_img_data is not None
+            still_image = Image.open(io.BytesIO(still_img_data))
             self.assertEqual((50, 50), still_image.size)
 
         # Test an image smaller than max and smaller than file size max is not resized
@@ -1323,8 +1324,11 @@ class EmojiTest(UploadSerializeMixin, ZulipTestCase):
             im = Image.open(io.BytesIO(resized_img_data))
             self.assertEqual((256, 256), im.size)
             self.assertTrue(is_animated)
-            still_image = Image.open(io.BytesIO(cast(bytes, still_img_data)))
-            self.assertEqual((256, 256), still_image.size)
+
+            # We unconditionally resize the still_image
+            assert still_img_data is not None
+            still_image = Image.open(io.BytesIO(still_img_data))
+            self.assertEqual((50, 50), still_image.size)
 
     def tearDown(self) -> None:
         destroy_uploads()
@@ -1734,10 +1738,13 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase):
         with get_test_image_file("img.png") as image_file:
             upload_emoji_image(image_file, file_name, user_profile)
         url = zerver.lib.upload.upload_backend.get_emoji_url(file_name, user_profile.realm_id)
-        still_url = zerver.lib.upload.upload_backend.get_emoji_url(
-            file_name, user_profile.realm_id, still=True
-        )
-        self.assertEqual(still_url, "")
+
+        # Verify the assert statement for trying to fetch the still
+        # version of a non-GIF image, since we only support animated GIFs.
+        with self.assertRaises(AssertionError):
+            still_url = zerver.lib.upload.upload_backend.get_emoji_url(
+                file_name, user_profile.realm_id, still=True
+            )
 
         emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format(
             realm_id=user_profile.realm_id,

There are a few outstanding issues, but this is otherwise very close to completion:

• I think the naming scheme for the still images might be problematic -- what happens if you try uploading foo and then foo-still as emoji names? I currently expect that to create a collision in the file upload space. I think we might be better off doing a still/ directory prefix instead.
• Confirmation on whether the changes I made (noted in comments above) are correct.
• I believe that as presently written, we will need to re-upload any existing animated custom emoji after deploying this PR before they'll benefit from this behavior (though I don't think anything will be broken, since is_animated will just be incorrectly false for those). It seems likely that we'll want to end up writing that as a migration as a follow-up to this PR. Does that sound correct to you @Riken-Shah?
• A few tests are failing, and I don't know why.