Fix/518 - new tab on macos #526
Conversation
There was a problem hiding this comment.
I like your solution to resolving issue around external url alerts, but I don't think the open link in new tab stuff always works reliably
Not that it's your fault or anything -- a lot of swiftui stuff are very new, and to be frank buggy. My position on "Open in new tab" is: if it doesn't result in a good and (mostly) bug free experience, let's disable it
| @@ -37,6 +37,7 @@ class BrowserViewModel: NSObject, ObservableObject, | |||
|
|
|||
| // MARK: - Properties | |||
|
|
|||
| @Published var openExternalLinkURL = URL(string: "") | |||
There was a problem hiding this comment.
Overall I think this is a good solution on resolving the problem where alerts are displayed in all windows. It keeps the state local, and I am not a fan of the notifications either
I have two feedback:
- why not use an optional URL, and have nil when when there isn't an external URL to open?
- (nit) let's use the same variable name here and inside
ExternalLinkHandler, I suggest useexternalLinkURLeverywhere
| @@ -179,7 +180,7 @@ class BrowserViewModel: NSObject, ObservableObject, | |||
| } else if url.isKiwixURL { | |||
| decisionHandler(.allow) | |||
| } else if url.scheme == "http" || url.scheme == "https" { | |||
| NotificationCenter.default.post(name: .externalLink, object: nil, userInfo: ["url": url]) | |||
| self.openExternalLinkURL = url | |||
There was a problem hiding this comment.
| self.openExternalLinkURL = url | |
| openExternalLinkURL = url |
| @@ -38,7 +38,7 @@ struct BrowserTab: View { | |||
| .focusedSceneValue(\.browserViewModel, browser) | |||
| .focusedSceneValue(\.canGoBack, browser.canGoBack) | |||
| .focusedSceneValue(\.canGoForward, browser.canGoForward) | |||
| .modifier(ExternalLinkHandler()) | |||
| .modifier(ExternalLinkHandler(externalURL: $browser.openExternalLinkURL)) | |||
There was a problem hiding this comment.
Fairly sure there is another usage of ExternalLinkHandler on a iOS specific view, could you change that as well?
| @@ -179,7 +180,7 @@ class BrowserViewModel: NSObject, ObservableObject, | |||
| } else if url.isKiwixURL { | |||
| decisionHandler(.allow) | |||
| } else if url.scheme == "http" || url.scheme == "https" { | |||
| NotificationCenter.default.post(name: .externalLink, object: nil, userInfo: ["url": url]) | |||
There was a problem hiding this comment.
Could you remove this as well? (I don't think we need this anymore, right?)
| @@ -11,12 +11,11 @@ import SwiftUI | |||
| import Defaults | |||
|
|
|||
| struct ExternalLinkHandler: ViewModifier { | |||
| @Binding var externalURL: URL? | |||
There was a problem hiding this comment.
| @Binding var externalURL: URL? | |
| @Binding var externalLinkURL: URL? |
(following the 2nd feedback from above)
| @@ -28,8 +27,10 @@ struct ExternalLinkHandler: ViewModifier { | |||
| } | |||
|
|
|||
| func body(content: Content) -> some View { | |||
| content.onReceive(externalLink) { notification in | |||
| guard let url = notification.userInfo?["url"] as? URL else { return } | |||
| content.onChange(of: externalURL, perform: { _ in | |||
There was a problem hiding this comment.
nit: could you write this in the style of
content.onChange(of: externalURL) {
...
}
| @@ -179,7 +180,7 @@ class BrowserViewModel: NSObject, ObservableObject, | |||
| } else if url.isKiwixURL { | |||
| decisionHandler(.allow) | |||
| } else if url.scheme == "http" || url.scheme == "https" { | |||
There was a problem hiding this comment.
If you are introducing isExternalLink on URL, could you change this line to use that variable instead?
| currentWindow.addTabbedWindow(newWindow, ordered: .above) | ||
|
|
||
| // open url in new tab | ||
| NotificationCenter.openURL(newUrl, inNewTab: true) |
There was a problem hiding this comment.
| NotificationCenter.openURL(newUrl, inNewTab: true) | |
| NotificationCenter.openURL(newUrl) |
Context: inNewTab is for iOS / iPadOS only, it doesn't do anything for Mac, so I suggest removing it
| guard let currentWindow = NSApp.keyWindow, | ||
| let windowController = currentWindow.windowController else { return nil } | ||
| windowController.newWindowForTab(nil) | ||
| guard let newWindow = NSApp.keyWindow, currentWindow != newWindow else { return nil } | ||
| currentWindow.addTabbedWindow(newWindow, ordered: .above) | ||
|
|
||
| // open url in new tab | ||
| NotificationCenter.openURL(newUrl, inNewTab: true) |
There was a problem hiding this comment.
This works well most of the times, but not always. The issue I think is @Environment(\.controlActiveState) var controlActiveState is supposed to tell us which window is key, and only the key window should respond to the open URL notification, but there sometimes can be multiple key windows.
I am not an AppKit expert, but I was under the impression only one window can become key at a time, since only one window is supposed to be able to receive keyboard event
You can reproduce the issue with (on macOS 14):
- using right click, open in new window to open stay 4 tabs
- go to the 2nd tab, right click on something, and open in new window
- multiple windows responds to the notification
- with the additional problem that the newly opened window having the incorrect title (haven't investigated why)
|
@anthony-bernardo Thank you for your work here. This PR has been superseeded by #536. |
I refactor the way the alert is receiving notification, because with
NotificationCenter(if there were opened tabs) the alert were displayed in each tabsTODO:
Fixes #518