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

remove Tab.TabContent.url #2647

Merged
merged 16 commits into from
Apr 26, 2024
Merged

remove Tab.TabContent.url #2647

merged 16 commits into from
Apr 26, 2024

Conversation

mallexxx
Copy link
Collaborator

Task/Issue URL: https://app.asana.com/0/0/1207078384259685/f

Description:

  • this is the first PR in the series of Trusted URL indicator for local pages - not to be merged until the following stacked PRs are merged
  • the PR disambiguates Tab.TabContent.url usage to urlForWebView and userEditableUrl

Steps to test this PR:

  1. Validate duck urls (except the duck player) cannot be bookmarked/fireproofed, regular URLs should be bookmarkable/fireproofable
  2. Validate no duck urls can be reloaded, regular URLs should be reloadable

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@mallexxx mallexxx requested a review from tomasstrba April 17, 2024 08:47
@github-actions github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 17, 2024
@github-actions github-actions bot removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 17, 2024
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

Unfotunately, the compilation fails for me. Am I missing something?
Screenshot 2024-04-19 at 10 06 05 AM


/// user-editable URL displayed in the address bar
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename it to urlForAddressBar? 🤷 Not a strong opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don‘t know, I think user editable url is pretty expressive to keep it

let urlScheme = tabViewModel.tab.content.url?.scheme
let isHypertextUrl = urlScheme == "http" || urlScheme == "https"
let isHypertextUrl = if case .url(let url, _, _) = tabViewModel.tab.content,
!(url.isDuckPlayer || url.isDuckURLScheme),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a computed property for this condition would make sense for reuse in future since these URLs will always have a special treatment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refined it a little bit for more clarity

Copy link
Contributor

Choose a reason for hiding this comment

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

better 👍 thank you!

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

Works well except one issue: I managed to add Settings to bookmarks. Let me know if you can't reproduce.

Screenshot 2024-04-23 at 8 42 17 AM

@@ -30,6 +30,11 @@ extension URL.NavigationalScheme {
return [.http, .https, .file]
}

/// HTTP or HTTPS
var isHypertextScheme: Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

let urlScheme = tabViewModel.tab.content.url?.scheme
let isHypertextUrl = urlScheme == "http" || urlScheme == "https"
let isHypertextUrl = if case .url(let url, _, _) = tabViewModel.tab.content,
!(url.isDuckPlayer || url.isDuckURLScheme),
Copy link
Contributor

Choose a reason for hiding this comment

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

better 👍 thank you!

@mallexxx
Copy link
Collaborator Author

mallexxx commented Apr 23, 2024

Actually it seems we're allowing to bookmark any url, see the design review
https://app.asana.com/0/0/1207123305870683/1207144706111173/f

@mallexxx
Copy link
Collaborator Author

I‘ve fixed opening duck:// url bookmarks (like settings) and allowed bookmarking the trusted pages, see the comment above

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

@mallexxx mallexxx merged commit 78f919c into main Apr 26, 2024
17 of 18 checks passed
@mallexxx mallexxx deleted the alex/tab-content-url branch April 26, 2024 15:21
samsymons added a commit that referenced this pull request Apr 26, 2024
…utdown

# By Dax the Duck (9) and others
# Via GitHub (6) and others
* main: (47 commits)
  Update BSK to 141.1.1 (#2713)
  macOS: VPN Metadata Improvements (#2704)
  duck page suggestions (#2666)
  macOS: Add pixels to track VPN wake and stop attempts (#2694)
  Trusted url indicator (#2665)
  remove Tab.TabContent.url (#2647)
  Bump version to 1.85.0 (176)
  Update release automation to support Privacy Pro section in release notes (#2710)
  Fix for VPN stop issues. (#2689)
  Hard-codes the VPN waitlist flags to ON (#2709)
  Add subscription status to the macOS metadata (#2680)
  Bump version to 1.85.0 (175)
  Call finish in the correct place (#2702)
  Fix layout issue on DBP (#2700)
  Update autoconsent to v10.6.1 (#2618)
  Make Clear All History shortcut available without entering Main Menu (#2682)
  Fix DataBrokerProtectionProcessor.swift lint
  Fix SwiftLint
  Add parameter allowed encoding to error descriptions (#2691)
  Remove debug flags
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
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.

2 participants