Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #627 Add brave://search URL scheme #3582

Merged
merged 5 commits into from
Jun 21, 2021
Merged

Fix #627 Add brave://search URL scheme #3582

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2cf8ab0
Adding brave://search?q=<search-query> URL scheme
Apr 27, 2021
a284191
Removed comment I left in code
KacperWybranski Apr 27, 2021
219c74b
Add search URL scheme test
May 2, 2021
58026b2
Added requested changes
KacperWybranski May 16, 2021
d569892
Added requested changes and deleted unused functions
KacperWybranski May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions BraveShareTo/ShareToBraveViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ class ShareToBraveViewController: SLComposeServiceViewController {
return URL(string: "brave://open-url?url=\(url)")
}

private func searchScheme(for text: String) -> URL? {
return URL(string: "brave://search?q=\(text)")
}

override func configurationItems() -> [Any]! {
guard let inputItems = extensionContext?.inputItems as? [NSExtensionItem] else {
return []
Expand All @@ -35,22 +39,31 @@ class ShareToBraveViewController: SLComposeServiceViewController {

provider.loadItem(of: provider.isUrl ? kUTTypeURL : kUTTypeText) { item, error in
var urlItem: URL?
var nonUrlText: String?
Copy link
Contributor

@iccub iccub May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This work, but I think we can make the code cleaner now, if you look at logic below that, there is a lot of nested ifs and the code is not super readable,

For us differences between text and url are small, we can remove code duplication a little and improve architecture

What do you think if we create a new structure to ShareToBraveViewController class holding either URL or text and handling the scheme properly,
Sample code

private struct Scheme {
        private enum SchemeType {
            case url, query
        }
        
        private let type: SchemeType
        private let urlOrQuery: String
        
        init?(item: NSSecureCoding) {
            if let text = item as? String {
                urlOrQuery = text
                type = .query
            } else if let url = (item as? URL)?.absoluteString.firstURL?.absoluteString {
                urlOrQuery = url
                type = .url
            } else {
                return nil
            }
        }
        
        var schemeUrl: URL? {
            guard let percentEncodedUrlOrQuery = urlOrQuery
                    .addingPercentEncoding(withAllowedCharacters: .alphanumerics) else {
                return nil
            }
            
            // Can improve even further with adding param using `URLComponents`
            switch type {
            case .url:
                return URL(string: "brave://open-url?url=\(percentEncodedUrlOrQuery)")
            case .query:
                return URL(string: "brave://search?q=\(percentEncodedUrlOrQuery)")
            }
        }
    }

Then calling it from provider.loadItem is much cleaner, you can simply do:

guard let item = item, let schemeUrl = Scheme(item: item)?.schemeUrl else {
    self.cancel()
    return
 }
            
self.handleUrl(schemeUrl)

what do you think about it?


// We can get urls from other apps as a kUTTypeText type, for example from Apple's mail.app.
if let text = item as? String {
urlItem = text.firstURL
if let url = text.firstURL {
urlItem = url
} else {
nonUrlText = text
}
} else if let url = item as? URL {
urlItem = url.absoluteString.firstURL
} else {
self.cancel()
return
}

// Just open the app if we don't find a url. In the future we could
// use this entry point to search instead of open a given URL
let urlString = urlItem?.absoluteString ?? ""
if let braveUrl = urlString.addingPercentEncoding(withAllowedCharacters: .alphanumerics).flatMap(self.urlScheme) {
self.handleUrl(braveUrl)
// Open url if it was found, in other case search for text with default search engine in browser
if let urlString = urlItem?.absoluteString {
if let braveUrl = urlString.addingPercentEncoding(withAllowedCharacters: .alphanumerics).flatMap(self.urlScheme) {
self.handleUrl(braveUrl)
}
} else if let text = nonUrlText {
if let braveUrl = text.addingPercentEncoding(withAllowedCharacters: .alphanumerics).flatMap(self.searchScheme) {
self.handleUrl(braveUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is too duplicated, what do you think about adding new var urlOrText: String? then assign to it either url absolute string or the nonUrlText, and call rest of the code.

This way you won't need to call addingPercentEncoding two times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll solve it this way

}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't remove one of the .addingPercentEncoding parts since later there is a .flatMap part which is different in both cases, but I managed to make it little simpler

}

Expand Down
3 changes: 3 additions & 0 deletions Client/Application/NavigationRouter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ enum NavigationPath: Equatable {
} else if urlString.starts(with: "\(scheme)://open-text") {
let text = components.valueForQuery("text")
self = .text(text ?? "")
} else if urlString.starts(with: "\(scheme)://search") {
let text = components.valueForQuery("q")
self = .text(text ?? "")
} else {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion Client/Frontend/Browser/Toolbars/UrlBar/TopToolbarView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ class TopToolbarView: UIView, ToolbarProtocol {
if search {
locationTextField?.text = text
// Not notifying when empty agrees with AutocompleteTextField.textDidChange.
delegate?.topToolbar(self, didEnterText: text)
delegate?.topToolbar(self, didSubmitText: text)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change needed for?
Currently it seems to break other part of the app when you are on google.com for example, and tap on url bar, it reloads the app instead of focusing on url bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that without this change, view is stuck with empty url bar and doesn't submit searching. Thanks for your comment, I haven't realized it breaks other part of the app, will try to solve that in different way.

} else {
locationTextField?.setTextWithoutSearching(text)
}
Expand Down
8 changes: 8 additions & 0 deletions ClientTests/NavigationRouterTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ class NavigationRouterTests: XCTestCase {
XCTAssertEqual(badNav, NavigationPath.url(webURL: URL(string: "blah"), isPrivate: false))
}

func testSearchScheme() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding unit test for it

let query = "Foo Bar".addingPercentEncoding(withAllowedCharacters: .alphanumerics)!
let appURL = "\(appScheme)://search?q="+query
let navItem = NavigationPath(url: URL(string: appURL)!)!

XCTAssertEqual(navItem, NavigationPath.text("Foo Bar"))
}

func testDefaultNavigationPath() {
let url = URL(string: "https://duckduckgo.com")!
let appURL = URL(string: "\(self.appScheme)://open-url?url=\(url.absoluteString.escape()!)")!
Expand Down