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

feature: support url unfurling settings #12441

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

igor-sirotin
Copy link
Contributor

@igor-sirotin igor-sirotin commented Oct 13, 2023

Closes #10866
Partially implements #12397 (I will remove the old settings and rearrange things in a separate PR not to mess things around)

backend implemented here: status-im/status-go#4128

What does the PR do

  • added URL Unfurling mode to settings page
  • added corresponding methods/signals for UrlUnfurlingMode setting
  • added a new gifUnfurlingEnabled local setting
  • removed duplication of the setting enum

Here I didn't bind the UI settings card to the new UrlUnfurlingMode, @alexjba will take that part (I'll open an issue)

@status-im-auto
Copy link
Member

status-im-auto commented Oct 13, 2023

Jenkins Builds

Click to see older builds (7)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e8c32dd #1 2023-10-13 16:25:53 ~5 min tests/statusq 📄log
✔️ e8c32dd #1 2023-10-13 16:26:53 ~6 min tests/nim 📄log
✔️ e8c32dd #1 2023-10-13 16:27:55 ~7 min macos/aarch64 🍎dmg
✔️ e8c32dd #1 2023-10-13 16:30:53 ~10 min macos/x86_64 🍎dmg
✔️ e8c32dd #1 2023-10-13 16:36:16 ~16 min linux/x86_64 📦tgz
✔️ e8c32dd #1 2023-10-13 16:54:09 ~33 min windows/x86_64 💿exe
✔️ e8c32dd #1 2023-10-13 16:54:13 ~33 min tests/e2e 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4bc5bcd #2 2023-10-16 10:20:43 ~6 min tests/nim 📄log
✔️ 4bc5bcd #2 2023-10-16 10:21:04 ~6 min macos/aarch64 🍎dmg
✔️ 4bc5bcd #2 2023-10-16 10:21:24 ~6 min tests/statusq 📄log
✔️ 4bc5bcd #2 2023-10-16 10:26:08 ~11 min macos/x86_64 🍎dmg
✔️ 4bc5bcd #2 2023-10-16 10:31:46 ~17 min linux/x86_64 📦tgz
✔️ 4bc5bcd #2 2023-10-16 10:44:03 ~29 min windows/x86_64 💿exe
✔️ 4bc5bcd #2 2023-10-16 10:50:08 ~35 min tests/e2e 📄log
a3e33d8 #3 2023-10-16 13:37:00 ~2 min macos/x86_64 📄log
✔️ a3e33d8 #3 2023-10-16 13:40:06 ~5 min tests/statusq 📄log
✔️ a3e33d8 #3 2023-10-16 13:40:21 ~6 min tests/nim 📄log
✔️ a3e33d8 #3 2023-10-16 13:42:30 ~8 min macos/aarch64 🍎dmg
✔️ a3e33d8 #3 2023-10-16 13:47:26 ~13 min linux/x86_64 📦tgz
✔️ a3e33d8 #3 2023-10-16 14:00:18 ~25 min windows/x86_64 💿exe
✖️ a3e33d8 #3 2023-10-16 14:02:25 ~28 min tests/e2e 📄log
✔️ a3e33d8 #4 2023-10-16 14:03:18 ~8 min macos/x86_64 🍎dmg
✖️ a3e33d8 #4 2023-10-16 14:36:09 ~26 min tests/e2e 📄log
✔️ a3e33d8 #5 2023-10-16 15:31:43 ~29 min tests/e2e 📄log
✔️ a3e33d8 #7 2023-10-16 16:05:19 ~30 min tests/e2e 📄log

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one minor comment from me

src/app_service/service/accounts/service.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me 👍.
Waiting for the status-go fix to test it.

@igor-sirotin
Copy link
Contributor Author

igor-sirotin commented Oct 16, 2023

Sooo, I made a mistake in my status-im/status-go#4128: after rebasing I didn't change the timestamp of my migration. So I merged a migration with timestamp 1696259336, which is older than the latest one 1697123233 in the directory.

And I also got "lucky" that the newer migrations were merged to status-desktop earlier than this PR:
#12407

This affected a few users (@alexjba @endulab) who migrated their test accounts before I got this PR merged.

Because this doesn't affect a release, and only 2 days (which were weekends) passed since then, I think we shouldn't bother about this.

This PR should be merged ASAP before more users face the bug.


UPD: For the ones who want to fix their databases, please, run the migration manually:

ALTER TABLE settings ADD COLUMN url_unfurling_mode INT NOT NULL DEFAULT 1;
ALTER TABLE settings_sync_clock ADD COLUMN url_unfurling_mode INT NOT NULL DEFAULT 0;

Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks! 👍

@@ -71,7 +71,12 @@ Loader {
return []
const separator = " "
const arr = links.split(separator)
const filtered = arr.filter(v => v.toLowerCase().endsWith('.gif'))
const filtered = arr.filter(value => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving this here for the record. I think it can also be fixed in another PR where we clean old settings. There's a conflict between the new/old settings.
E.g. GIF enabled in the new settings and disabled in the old settings:

Screenshot 2023-10-16 at 15 53 51

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'm working on settings clean-up now, it will be there 👌

@igor-sirotin igor-sirotin merged commit 22ce35c into master Oct 16, 2023
@igor-sirotin igor-sirotin deleted the Feat/url-unfurling-settings branch October 16, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new unfurling setting to change if senders want to fetch Link data when sending messages
4 participants