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

chore: Remove the Browser from the app completely for now #15984

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

caybro
Copy link
Member

@caybro caybro commented Aug 5, 2024

What does the PR do

  • completely removes the ui/app/AppLayouts/Browser QML app section
  • removes the app_service/service/bookmarks and app/modules/main/browser_section NIM modules
  • remove the Browser settings page and associated popups/components
  • HTML links now always open in an external browser
  • adjust the section indexes in Constants
  • fixup the e2e tests

Fixes #14614

Affected areas

App

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
Zaznam.obrazovky.z.2024-08-07.09-59-16.webm

@caybro caybro linked an issue Aug 5, 2024 that may be closed by this pull request
@status-im-auto
Copy link
Member

status-im-auto commented Aug 5, 2024

Jenkins Builds

Click to see older builds (47)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 002ce07 #1 2024-08-05 11:05:21 ~6 min tests/nim 📄log
✔️ 002ce07 #1 2024-08-05 11:07:07 ~8 min macos/aarch64 🍎dmg
✔️ 002ce07 #1 2024-08-05 11:09:10 ~10 min macos/x86_64 🍎dmg
✔️ 002ce07 #1 2024-08-05 11:13:38 ~14 min tests/ui 📄log
✔️ 002ce07 #1 2024-08-05 11:14:00 ~15 min linux-nix/x86_64 📦tgz
✔️ 002ce07 #1 2024-08-05 11:16:42 ~17 min linux/x86_64 📦tgz
✔️ 654558f #2 2024-08-05 11:38:49 ~4 min macos/aarch64 🍎dmg
✔️ 654558f #2 2024-08-05 11:40:55 ~6 min tests/nim 📄log
✔️ 654558f #2 2024-08-05 11:42:34 ~8 min macos/x86_64 🍎dmg
✔️ 274121e #3 2024-08-05 11:48:09 ~3 min macos/aarch64 🍎dmg
✔️ 274121e #3 2024-08-05 11:50:14 ~6 min tests/nim 📄log
✔️ 274121e #3 2024-08-05 11:51:41 ~7 min macos/x86_64 🍎dmg
274121e #3 2024-08-05 11:56:31 ~12 min tests/ui 📄log
✔️ 274121e #3 2024-08-05 12:00:32 ~16 min linux-nix/x86_64 📦tgz
✔️ 274121e #3 2024-08-05 12:00:41 ~16 min linux/x86_64 📦tgz
✔️ 274121e #3 2024-08-05 12:14:19 ~30 min windows/x86_64 💿exe
✔️ 274121e #4 2024-08-05 13:16:20 ~12 min tests/ui 📄log
✔️ b951cb6 #4 2024-08-05 13:39:47 ~4 min macos/aarch64 🍎dmg
✔️ b951cb6 #4 2024-08-05 13:42:12 ~6 min tests/nim 📄log
✔️ b951cb6 #4 2024-08-05 13:43:47 ~8 min macos/x86_64 🍎dmg
✔️ b951cb6 #4 2024-08-05 13:48:22 ~12 min linux-nix/x86_64 📦tgz
✔️ b951cb6 #5 2024-08-05 13:49:07 ~13 min tests/ui 📄log
✔️ b951cb6 #4 2024-08-05 13:50:59 ~15 min linux/x86_64 📦tgz
✔️ b951cb6 #4 2024-08-05 14:01:19 ~25 min windows/x86_64 💿exe
✔️ d213ff3 #5 2024-08-05 14:48:10 ~6 min tests/nim 📄log
✔️ b60c0aa #6 2024-08-05 14:53:03 ~4 min macos/aarch64 🍎dmg
✔️ b60c0aa #6 2024-08-05 14:55:14 ~6 min tests/nim 📄log
✔️ b60c0aa #6 2024-08-05 14:59:24 ~10 min macos/x86_64 🍎dmg
✔️ b60c0aa #7 2024-08-05 15:01:02 ~12 min tests/ui 📄log
✔️ b60c0aa #6 2024-08-05 15:05:55 ~17 min linux-nix/x86_64 📦tgz
✔️ b60c0aa #6 2024-08-05 15:06:07 ~17 min linux/x86_64 📦tgz
✔️ b60c0aa #6 2024-08-05 15:13:53 ~25 min windows/x86_64 💿exe
✔️ 27d4e87 #7 2024-08-05 16:31:21 ~4 min macos/aarch64 🍎dmg
✔️ 27d4e87 #7 2024-08-05 16:32:48 ~5 min tests/nim 📄log
✔️ 27d4e87 #7 2024-08-05 16:35:00 ~8 min macos/x86_64 🍎dmg
27d4e87 #8 2024-08-05 16:39:10 ~12 min tests/ui 📄log
✔️ 27d4e87 #7 2024-08-05 16:39:19 ~12 min linux-nix/x86_64 📦tgz
✔️ 27d4e87 #7 2024-08-05 16:42:06 ~15 min linux/x86_64 📦tgz
✔️ 27d4e87 #7 2024-08-05 16:51:11 ~24 min windows/x86_64 💿exe
✔️ 27d4e87 #9 2024-08-05 18:09:06 ~12 min tests/ui 📄log
✔️ 4d22350 #8 2024-08-07 08:11:36 ~5 min macos/aarch64 🍎dmg
✔️ 4d22350 #8 2024-08-07 08:12:41 ~6 min tests/nim 📄log
✔️ 4d22350 #8 2024-08-07 08:14:55 ~8 min macos/x86_64 🍎dmg
✔️ 4d22350 #8 2024-08-07 08:19:27 ~13 min linux-nix/x86_64 📦tgz
✔️ 4d22350 #10 2024-08-07 08:19:40 ~13 min tests/ui 📄log
✔️ 4d22350 #8 2024-08-07 08:19:56 ~13 min linux/x86_64 📦tgz
✔️ 4d22350 #8 2024-08-07 08:32:34 ~26 min windows/x86_64 💿exe
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2f39da7 #9 2024-08-07 09:09:46 ~4 min macos/aarch64 🍎dmg
✔️ 2f39da7 #9 2024-08-07 09:11:33 ~6 min tests/nim 📄log
✔️ 2f39da7 #9 2024-08-07 09:13:17 ~8 min macos/x86_64 🍎dmg
✔️ 2f39da7 #9 2024-08-07 09:17:01 ~11 min linux-nix/x86_64 📦tgz
✔️ 2f39da7 #11 2024-08-07 09:18:08 ~13 min tests/ui 📄log
✔️ 2f39da7 #9 2024-08-07 09:19:05 ~14 min linux/x86_64 📦tgz
✔️ 2f39da7 #9 2024-08-07 09:38:16 ~33 min windows/x86_64 💿exe
✔️ f19a1ae #10 2024-08-07 09:59:22 ~4 min macos/aarch64 🍎dmg
✔️ f19a1ae #10 2024-08-07 10:01:43 ~6 min tests/nim 📄log
✔️ f19a1ae #10 2024-08-07 10:03:02 ~7 min macos/x86_64 🍎dmg
✔️ f19a1ae #10 2024-08-07 10:07:06 ~12 min linux-nix/x86_64 📦tgz
✔️ f19a1ae #12 2024-08-07 10:07:36 ~12 min tests/ui 📄log
✔️ f19a1ae #10 2024-08-07 10:08:28 ~13 min linux/x86_64 📦tgz
✔️ f19a1ae #10 2024-08-07 10:18:58 ~23 min windows/x86_64 💿exe

@caybro caybro force-pushed the 14614-remove-the-browser-from-the-app branch 6 times, most recently from b60c0aa to 27d4e87 Compare August 5, 2024 16:26
@status-im-auto
Copy link
Member

✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15984#7 🔹 ~5 min 55 sec 🔹 27d4e87 🔹 📦 tests/nim package

@caybro caybro force-pushed the 14614-remove-the-browser-from-the-app branch from 27d4e87 to 4d22350 Compare August 7, 2024 08:06
@caybro caybro changed the title WIP: Remove the Browser from the app completely for now chore: Remove the Browser from the app completely for now Aug 7, 2024
@caybro caybro marked this pull request as ready for review August 7, 2024 08:08
Copy link
Collaborator

@alaibe alaibe left a comment

Choose a reason for hiding this comment

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

What about the services part? nothing to clean up here?

@caybro
Copy link
Member Author

caybro commented Aug 7, 2024

What about the services part? nothing to clean up here?

Which part specifically? Some NIM stuff?

@caybro caybro requested a review from stefandunca August 7, 2024 08:16
@caybro
Copy link
Member Author

caybro commented Aug 7, 2024

@stefandunca just chiming in to check whether I didn't remove too much :) (esp. wrt to wallet connect/dapp stuff)

@alaibe
Copy link
Collaborator

alaibe commented Aug 7, 2024

I mean:
https://github.com/status-im/status-desktop/blob/master/src/backend/backend.nim#L221
and
https://github.com/status-im/status-desktop/tree/master/src/app_service/service/dapp_permissions

For example. are they used somewhere else?
They might be other stuff purely related to the browser in service

@caybro
Copy link
Member Author

caybro commented Aug 7, 2024

I mean: https://github.com/status-im/status-desktop/blob/master/src/backend/backend.nim#L221 and https://github.com/status-im/status-desktop/tree/master/src/app_service/service/dapp_permissions

For example. are they used somewhere else? They might be other stuff purely related to the browser in service

Yeah, not sure about these; same for the Bookmark code in backend.nim

@alaibe
Copy link
Collaborator

alaibe commented Aug 7, 2024

Yeah, not sure about these; same for the Bookmark code in backend.nim

Bookmark is probably even more certain, if you cannot find any usage i would personally remove all that!

@caybro caybro force-pushed the 14614-remove-the-browser-from-the-app branch from 4d22350 to 2f39da7 Compare August 7, 2024 09:04
@caybro
Copy link
Member Author

caybro commented Aug 7, 2024

Yeah, not sure about these; same for the Bookmark code in backend.nim

Bookmark is probably even more certain, if you cannot find any usage i would personally remove all that!

Bookmark stuff removed from backend.nim

@status-im-auto
Copy link
Member

✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15984#9 🔹 ~6 min 30 sec 🔹 2f39da7 🔹 📦 tests/nim package

Copy link
Contributor

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

🧹

- completely removes the `ui/app/AppLayouts/Browser` QML app section
- removes the `app_service/service/bookmarks`,
`app/modules/main/browser_section` and
`src/app_service/service/dapp_permissions` NIM modules
- remove the Browser settings page and associated popups/components
- HTML links now always open in an external browser
- adjust the section indexes in `Constants`
- fixup the e2e tests

Fixes #14614
@caybro caybro force-pushed the 14614-remove-the-browser-from-the-app branch from 2f39da7 to f19a1ae Compare August 7, 2024 09:54
Copy link
Contributor

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Nice!

I assume we can't remove QtWebEngine, it's still used somewhere else?


Also, maybe we also need to update some external resources?

Docs

I didn't find mentions of browser there, but didn't look deep.

cc @jorge-campo

Website

https://status.app/apps:

image image

cc @prichodko

@caybro
Copy link
Member Author

caybro commented Aug 7, 2024

Nice!

I assume we can't remove QtWebEngine, it's still used somewhere else?

No we can't since the walletconnect/dApps stuff has a runtime dependency on this, correct @alexjba ?

@stefandunca
Copy link
Contributor

@stefandunca just chiming in to check whether I didn't remove too much :) (esp. wrt to wallet connect/dapp stuff)

I don't see anything related to the new dapps implementation. Good job!

@caybro caybro merged commit ed650d3 into master Aug 7, 2024
9 checks passed
@caybro caybro deleted the 14614-remove-the-browser-from-the-app branch August 7, 2024 14:45
@jorge-campo
Copy link

Docs

I didn't find mentions of browser there, but didn't look deep.

cc @jorge-campo

Thanks, @igor-sirotin ! We'll update the docs accordingly -> https://github.com/status-im/status-website/issues/1165

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.

Remove the Browser from the app completely for now
8 participants