Link preview generation #11311
Merged
Link preview generation #11311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (33)
|
|
@vkjr there are conflicts in the branch. Could you take a look, please? |
flexsurfer
reviewed
Oct 22, 2020
| Runnable r = new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| String res = Statusgo.getLinkPreviewWhitelist(); |
There was a problem hiding this comment.
just wondering why not a service ?
| (re-frame/reg-fx | ||
| ::get-link-preview-whitelist | ||
| (fn [] | ||
| (native-module/link-preview-whitelist #(do |
| (disj (get multiaccount :link-previews-enabled-sites #{}) site)) | ||
| {}))) | ||
|
|
||
| (fx/defn cache-link-preview-data |
There was a problem hiding this comment.
could you pls elaborate on this ?
There was a problem hiding this comment.
During session we save locally data about link preview to not fetch it every time when chat re-creates
src/status_im/constants.cljs
Outdated
| :webview-allow-permission-requests? false}) | ||
| :webview-allow-permission-requests? false | ||
| :link-previews-enabled-sites #{} | ||
| :can-ask-to-preview-links true}) |
There was a problem hiding this comment.
will be renamed but it is set to false when user says "Don't ask me again about link preview"
| [status-im.native-module.core :as native-module])) | ||
|
|
||
| (fx/defn set-link-preview | ||
| [{{:keys [multiaccount]} :db :as cofx} site enabled?] |
There was a problem hiding this comment.
{:events [:link-preview/enable]}
src/status_im/events.cljs
Outdated
| @@ -1264,3 +1265,18 @@ | |||
| :multiaccounts (keycard/multiaccounts-screen-did-load %) | |||
| (:wallet-stack :wallet) (wallet.events/wallet-will-focus %) | |||
| nil)))) | |||
|
|
|||
| (handlers/register-handler-fx | |||
There was a problem hiding this comment.
we don't use this ns anymore, just use {:events []} in the link-preview ns
| @@ -205,7 +206,9 @@ | |||
| [message-author-name from modal]]) | |||
| ;;MESSAGE CONTENT | |||
| [react/view | |||
| content]]] | |||
| content] | |||
| (when-let [link (link-preview/get-link (:parsed-text (:content message)))] | |||
There was a problem hiding this comment.
can we move this to status-go, so we'll have links from the whitelist in (:valid-links (:content message))
There was a problem hiding this comment.
that's a good idea, maybe is worth to have only: links (say something like {:domain ... :url ...} rather than valid-links, and in status-react we match against a domain whitelist.
The rationale is the whitelist might be changing because the user enables/disables and we won't be reloading messages probably, what do you think?
There was a problem hiding this comment.
yeah all links is better, i wanted to suggest it first, but for some reason changed my mind in the last moment :)
There was a problem hiding this comment.
Thanks, will dig it on status-go side
There was a problem hiding this comment.
https://github.com/status-im/status-go/blob/7467ca7b103f685dd64d73485555f90c181a4484/protocol/common/message.go#L306 is a class you can modify to fetch links and store them in the database, similarly to how we do with mentions
flexsurfer
reviewed
Oct 23, 2020
| @@ -81,6 +81,8 @@ | |||
| "wakuext_sendEmojiReaction" {} | |||
| "wakuext_sendEmojiReactionRetraction" {} | |||
| "wakuext_emojiReactionsByChatID" {} | |||
| "wakuext_getLinkPreviewWhitelist" {} | |||
| "wakuext_getLinkPreviewData" {} | |||
There was a problem hiding this comment.
probably better to move this to accounts or settings service
vkjr
force-pushed
the
url_unfurling
branch
5 times, most recently
from
80befcc
to
6ddb774
Compare
|
@cammellos, @flexsurfer, review notes addressed, could you please check again? @qoqobolo, rebased) |
64% of end-end tests have passedFailed tests (36)Click to expand
Passed tests (63)Click to expand
|
|
@qoqobolo - done :) |
90% of end-end tests have passedFailed tests (10)Click to expand
Passed tests (89)Click to expand
|
|
All failed e2e are not related to PR (ropsten network issues) |
|
@vkjr |
|
@churik, hmm, it was never discussed if reaction should work on the link-preview, so I can say it is expected since it wasn't implemented :)) Anyway I'd be happy to merge this and add improvements as a separate prs :) |
|
also not sure about transparency of preview, it can cause such behavior: cc @errorists is it OK to merge like this from your POV? |
|
whoa, I didn't know we had a PR for this already. cool 😅 the fix for transparency looks simple enough, just place a white background for the container. I think yes, long press and reactions should be possible for the unfurled message alone with actions: Open and Copy, without replies. oh and yes it's ok to merge if we follow up with the long press menu fixes |
|
Would be happy implement long press menu fixes immediately after this one) |
|
@vkjr dark mode is affected |
|
@churik, fixed :) |
|
Thanks @vkjr ! |
|
@cammellos, @flexsurfer, needs your approval :) |
flexsurfer
approved these changes
Nov 2, 2020
| @@ -393,6 +393,17 @@ | |||
| (log/debug "[native-module] deactivateKeepAwake") | |||
| (.deactivateKeepAwake ^js (status))) | |||
|
|
|||
| (defn link-preview-whitelist | |||
There was a problem hiding this comment.
yes, thanks for catching!
Signed-off-by: Volodymyr Kozieiev <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #11158
Summary
Link preview generation implemented. Currently works for
youtube.Review notes
Platforms
Functional
Steps to test
status: ready