Add Gif picker

[LBBO]

First time contributor checklist:

• I have read the README and Contributor Guidelines
• I have signed the Contributor Licence Agreement

Contributor checklist:

• My contribution is not related to translations.
• My commits are in nice logical chunks with good commit messages
• My changes are rebased on the latest main branch
• A yarn ready run passes successfully (more about tests here)
• My changes are ready to be shipped to users

Description

Fixes #4841 and https://community.signalusers.org/t/add-gif-search-giphy-to-signal-desktop/3266

This PR adds a GIF picker to Signal Desktop. I tried to make it as similar to the Android app as possible, so the picker uses Giphy as a search engine. More specifically, it uses the @giphy/js-fetch-api package for the actual queries and the @giphy/react-components package for displaying the grid.

I have tested the individual components (GifPicker and GifButton) in Storybook (see the Gifs folder) and have tested them in the running application locally. There are currently no automated tests, but I plan to add as many as I can. Any advice on that is greatly appreciated!

Please note that due to the way that GIFs are currently loaded, the Gif picker won't work in the application without the following changes (Storybook should work out of the box). For a discussion of this, please refer to the open questions below.

1. Remove

   Signal-Desktop/app/protocol_filter.ts

   Line 177 in 6700d2f

   protocol.interceptFileProtocol('https', _disabledHandler);

2. Add https://*.giphy.com to the CSP for images at

   Signal-Desktop/background.html

   Line 23 in 6700d2f

   img-src 'self' blob: data:;

Open questions

1. Could you please generate a unique Giphy API key for the desktop app? I stole the one I'm currently using from the iOS app.
2. Currently, all requests made from the frontend (sorry for imprecise terminology, I'm not that familiar with Electron) are blocked (see linked protocol_filter.ts above). Instead, they're supposed to be made via node.js. I wrote a wrapper around @giphy/js-fetch-api that takes care of just that, but then @giphy/react-components still displays the images in <img> tags with some HTTPS URL and those calls are also blocked. I thought about how to deal with this issue for a while and have a couple of ideas, none of which I'm really happy with:
   a) Modify the response I get from @giphy/js-fetch-api and replace the URL I want to use with a pre-fetched data:-URL. Disadvantage: I'd be meddling with the internals of the Giphy API since @giphy/react-components relies on it. This solution might break with the slightest change of their API OR their components.
   b) Write a custom implementation of @giphy/react-components that just uses pre-fetched data: URLs. This might still break due to changes of their API, but it wouldn't break due to changes of Giphy's components.
   c) Disable the blocking for these requests. I'm not sure if that would be desired and it seemed rather difficult to integrate in the current blocking mechanism since it doesn't seem to allow for filters or such. But I could look into this some more if this is your preference.
   Do you have a better suggestion? Or are you happy with one of the suggestions I provided?
3. @giphy/react-components currently phones home on practically every user interaction with a GIF (hover, click, probably more). There's already an issue about that (Consider making pingback and remote fonts configurable Giphy/giphy-js#291) and it's next on my todo list. Currently, those requests are just blocked along with all other HTTP requests (unless they're enabled again, as I did locally) and cause error messages in the console. However, the picker works just fine. How would you like to deal with this? Is the pinging / are the error messages in the console tolerable? Do you want to wait until they're removed / removable from the component? Should the grid be re-implemented from scratch? Or do you have another suggestion?

Open TODOs:

(More of a checklist for myself, you can view this as a progress bar :D )

• Replace the iOS API key with a new API key
• Correctly load recent GIFs at startup
• Add automatic tests wherever possible
• Add some a11y (Focused Gif is not visually emphasized Giphy/giphy-js#326)
• Focus input after GIF was picked (mainly so that the user can just press Enter to send the GIF)
• Deal with Giphy pingbacks
• Use .webp instead of .gif whenever available. This requires rendering support for WEBP in attachments, which doesn't seem to work yet.
• Improve keyboard navigation (autofocus search box, close picker on escape)

[LBBO]

Update about the Giphy pingbacks: when merged, Giphy/giphy-js#330 would fix that issue. I'm not checking it off my todo list yet because the version will need to be updated here. And, in case my opt-in suggestion is denied and has to be replaced with an opt-out, that opt-out will have to be added.

[LBBO]

Alright, there's been an update from Giphy. As you can see in the issue and my PR linked above, they do not plan to allow developers to disable pingbacks. This makes my questions all the more important. On the one hand, blocking requests is a great way to block these pingbacks, but on the other hand, requests are necessary for loading the GIFs and querying the API.

Could someone maybe comment? Perhaps @indutny-signal or @alvaro-signal?

Also, just as a side-note: I'm already working on fixing the conflicts.

[jamiebuilds-signal]

Hi @LBBO, thank you so much for this pull request, I'm definitely gonna get a lot of use out of Giphy support. I've done an initial review of the code and it looks well written. But since this is a rather large feature, I'm going to take over this pull request to make some necessary changes to it before we can ship it. When we do ship it, I'll make sure that you receive credit for implementing it. I don't have a clear timeline yet but I'll let you know.

[LBBO]

Hey @jamiebuilds-signal, thanks for the response! If there's anything else I can do to help bring this along, just let me know! Also, I'm always eager to learn where I can improve, so if you're interested, I'd love to maybe hop on a Discord call some time to hear what it is that needs to be fixed.

[jamiebuilds-signal]

One of the main things is going to be replacing the Giphy fetching code to go through a proxy. Which isn't something you'll be able to do on your own. We'll also probably go through a design review which I'll just need to go back and forth with one of our designers

[error401de]

@jamiebuilds-signal @scottnonnenberg-signal Is there any news on this topic? It has been discussed since many years now, I hoped the PR would bring some momentum but now there is again no news since 3 months.
Thanks

[Rykee]

@jamiebuilds-signal @scottnonnenberg-signal Is there any news on this topic? It has been discussed since many years now, I hoped the PR would bring some momentum but now there is again no news since 3 6 months.
Thanks

[LiloBzH]

Same question here. To remplace Whatsapp and motive new people, it's a goot idea !

[error401de]

I really dislike the fact that Signal in the meantime is fully and completely disconnected from it's user base. Yes, things can take a while. Breaking promises since many years while ignoring every question from the community is something else. Especially while constantly releasing features nobody publicly requested.

Personally I have stopped donating to Signal since I don't feel the userbase is appreciated.

(Again mentioning @jamiebuilds-signal @scottnonnenberg-signal , even if I wont get an answer anway.)

[scottnonnenberg-signal]

@error401de I'm sorry you're frustrated. At the moment, we just don't have time to review an 1900-line PR, because it doesn't match up with our internal priorities. That's the high-level situation.

[error401de]

@scottnonnenberg-signal "At the moment"? Sorry, but this feature is promised since 2016. 7 years is very, very far away from "at the moment".
You could share an high level roadmap / kanban to give some transparency on the next steps of the apps (as requested by many, many users).
This is the least one can expect from a user-funded project. Not even mentioning a feature voting tool.

[superm1]

@error401de I'm sorry you're frustrated. At the moment, we just don't have time to review an 1900-line PR, because it doesn't match up with our internal priorities. That's the high-level situation.

1900 lines is an exaggeration. It's not all code.

For example you wouldn't review svg files line by line, you would open them in a image viewer.

[23tux]

@error401de I'm sorry you're frustrated. At the moment, we just don't have time to review an 1900-line PR, because it doesn't match up with our internal priorities. That's the high-level situation.

1900 lines is an exaggeration. It's not all code.

For example you wouldn't review svg files line by line, you would open them in a image viewer

I see it in the same way: Part of the PR is just SVGs and license stuff, nothing that you have to code review, more of a content review.

I find it sad that someone spends his time programming a most wanted feature from the community, and Signal isn't willing to provide at least some feedback other than "it's too huge".

[astrostl]

I began a monthly recurring donation years ago and I just canceled it after walking the tree of announcements, to a closed/disabled issue, to other issues, to this PR. I don't know of a more effective way to express my support of this subject. I will plan to resume it once the feature is implemented.

[jeaye]

Thanks so much for making this PR, @LBBO. It's exactly what I think is missing from Signal on desktop right now and I appreciate your attention to the UX.

@scottnonnenberg-signal / @jamiebuilds-signal I know that Signal has not been prioritizing this. I'm interested in how we can measure interest in this to help with prioritization. Is there a better forum for users to express their needs? If so, the folks interested in this feature can gather and we'll get a better idea of the overall demand.

In short, if there's more the community can do here, let's communicate that and I've no doubt that it will be done.

[error401de]

The lack of reaction to people, who want to help, is exactly the "disconnection from community" I was talking about.
4 more months, new features like editing of messages (no ressources, right?), ignorance in here.

[scottnonnenberg-signal]

Thanks for your passion, everyone, and your frustration. It shows that you care.

I want to provide a little more context to make it clear what's happening. First, to implement this to our standards, it needs to use a Signal proxy and a chunking approach to ensure that the proxy can't fingerprint what file is being requested. Second, it needs to conform to our design requirements, and that team doesn't have time to look at this right now. Third, it needs to be comprehensively implemented and tested: a11y, keyboard support, and dark/light theme support are just some of the required elements of a new large feature like this.

It's a surprisingly large amount of work to coordinate with the community to make all of this happen. It's only a little bit easier than simply implementing it ourselves. And it's not on our near-term priority list right now.

Given all that, perhaps you can see why third-party PRs should generally be bugfixes! Truthfully, for the best chance of PR merge, please consider reaching out to us beforehand and asking which changes are most realistic as third-party PRs.

[LBBO]

TL;DR: As it was my goal to implement this "to [your] standards" from the beginning, I analyzed the gif picker implementations of the iOS and Android apps before even starting to implement anything and found that they (to this day) seem to not use any kind of proxy for queries to Giphy (code references below). Therefore, at least in this regard, I don't understand why this PR would be beneath Signal's standards. I also disagree with your representation of how much time reviewing this PR would take vs. implementing it yourself and I strongly disagree with the notion that open source contributions should be limited to bug fixes.

---

Hey @scottnonnenberg-signal,

thanks for taking the time to respond to the messages in this thread. As the author of this PR, I'm a bit confused about especially one of your points.

Before I implemented any changes, I did my research and found this blog post from November 2016 where the proxy approach you mentioned is introduced. That post made me very happy, as I was hoping for some sort of privacy-friendly GIF picker implementation in the other Signal clients that I could then copy. So, in a search to figure out how it worked, I looked at the iOS and Android code.

On iOS, I was surprised to find (what looks to me, as a non-Swift developer, like) no use of any such proxy. What I assume to be the controller for the GIF picker calls a GiphyAPI (code), which seems to be just a custom binding of the actual Giphy API (code, please note the kGiphyBaseURL which points straight to Giphy and is used directly for setting up a session. This is also where I copied the API token in this PR from, see one of the open questions way above). There may be some sort of intercepting going on somewhere but I wasn't able to find it (again - I've never seen real-life Swift code before).

On Android, I found basically the same thing. The GiphyMp4ViewModel (which presumably powers the GIF picker) gets data from a GiphyMp4PagedDataSource (code) which seems to be a similar custom binding to the Giphy API as seen on iOS (code, note how it also links directly to the Giphy API but this time pulls the API key from some build config).

Again, there may be some magic intercepting going on somewhere, but I couldn't find it after some searching. Searches for "intercept" or "proxy" yielded nothing interesting in either codebase. If there is some intercepting going on somewhere, I'd love to be pointed to the corresponding code!

All this to say: if I understand the two mobile apps correctly, they either don't use a proxy, in which case I don't see why the desktop app should, OR they perform some kind of intercepting, in which case I would argue that taking the same approach here would be nice just for consistency reasons. In that case, I'd also really appreciate some additional documentation in the other app's code bases, especially considering the high standards you're holding yourself to.

Then, regarding something else you said:

[...] please consider reaching out to us beforehand and asking which changes are most realistic as third-party PRs.

There were at least two separate requests for this feature with many likes. However, both had no response indicating there would be any progress and so opening a PR was just the next best thing to do in order to get closer to this feature actually existing. And I was very willing to work with you to make any changes necessary for this to get merged, but there was never even a single moment of discussion of actual code.

[Reviewing a PR for a feature is] only a little bit easier than simply implementing it ourselves.

Of course a PR doesn't take the entire work off you, but testing needs to happen either way, no matter who writes the code. The same goes for a good code review. In the end, such a PR should take away the complete development time, but the reviewing time would be there no matter who opens the PR. Sure, it might be a bit more effort if the author isn't used to your conventions etc., but that should be no comparison to actual development time it saves you.

Given all that, perhaps you can see why third-party PRs should generally be bugfixes!

Generally, I strongly disagree with this. Sure, large features usually come from project maintainers, but I've seen plenty features being implemented by others (including at the open-source-core company I work at). Especially since I would personally consider this a minor feature (the general UI concepts already exist for stickers and emojis, the same feature has been implemented for other platforms which can serve as a roadmap for this implementation), I wouldn't consider this feature off-limits for third-party contributions. But if this is how you feel about feature PRs in general, I recommend you clarify this in your CONTRIBUTING.md.

[scottnonnenberg-signal]

@LBBO We are indeed looking at our CONTRIBUTING.md to ensure it properly matches our third-party PR expectations. Thanks for the suggestion.

[astrostl]

@LBBO We are indeed looking at our CONTRIBUTING.md to ensure it properly matches our third-party PR expectations. Thanks for the suggestion.

Any response to the other 95% of the post?

[g0djan]

Hi @LBBO,

Sorry to hear that giphy search is not coming to desktop app and that your work couldn't be merged.
I just came across and got concerned whether the proclaimed feature is actually using proxy and it seems proxy feature is actually there.

On iOS, I was surprised to find (what looks to me, as a non-Swift developer, like) no use of any such proxy. What I assume to be the controller for the GIF picker calls a GiphyAPI (code), which seems to be just a custom binding of the actual Giphy API (code, please note the kGiphyBaseURL which points straight to Giphy and is used directly for setting up a session

The proxy is set here and according to library docs

This property controls which proxy tasks within sessions 
based on this configuration use when connecting to remote hosts.

I'm also not Swift user but I guess this Foundation library utilises it.

The initial commit where proxy was introduced to iOS app coming back to 2017

On Android, I found basically the same thing. The GiphyMp4ViewModel (which presumably powers the GIF picker) gets data from a GiphyMp4PagedDataSource (code) which seems to be a similar custom binding to the Giphy API as seen on iOS (code, note how it also links directly to the Giphy API but this time pulls the API key from some build config).

Search found the same url for Android.
And it's initial giphy proxy commit for Android from 2016

[githubuser6000]

All this to say: if I understand the two mobile apps correctly, they either don't use a proxy, in which case I don't see why the desktop app should, OR they perform some kind of intercepting, in which case I would argue that taking the same approach here would be nice just for consistency reasons.

Could you respond to this? @scottnonnenberg-signal

[ExtRIELICi]

It's almost 2025....