Use NSPanel for Sparkle windows to allow untitled windows form Dock

[GetToSet]

Background

It takes time to download updates and users tend to continue using an app while it's downloading updates.

Document based apps often override -[NSApplicationDelegate applicationShouldOpenUntitledFile:]`` & -[NSApplicationDelegate applicationOpenUntitledFile:]` to allow opening new document when the Dock icon being clicked when no window exists.

An interesting behavior that I discover is that while Sparkle has its window on screen (e.g. downloading updates), AppKit no longer calls -[NSApplicationDelegate applicationOpenUntitledFile:]. This behavior especially bothers users for apps such as iTerm2, since closing an unneeded pseudo terminal and reopen a new one from Dock is an extremely frequent interaction. Users have to right-click or control-click the Dock icon to achieve the desired behavior.

This PR uses NSPanel (rather than NSWindow) for Sparkle windows to allow untitled windows form Dock.

Deeper Dive

Here is a typical call stack that AppKit handles opening untitled file:

Disassembling AppKit binary (12.0.1, 21A559) and looking for symbol -[NSApplication(NSAppleEventHandling) _handleAEReopen:] signifies AppKit prevents -[NSApplication _doOpenUntitled]call when-[NSApplication _appHasOpenNSWindow]isYES`:

For its implementation (notice the third argument differs):

-[NSApplication _appHasOpenNSWindow] has a dedicated check that excludes NSPanel class:

and here comes this PR.

Checklist:

• My change is being tested and reviewed against the Sparkle 2.x branch. New changes must be developed on the 2.x development branch first.
• My change is being backported to master branch (Sparkle 1.x). Please create a separate pull request for 1.x, should it be backported. Note 1.x is feature frozen and is only taking bug fixes, localization updates, and critical OS adoption enhancements.
• I have reviewed and commented my code, particularly in hard-to-understand areas.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• My change is or requires a documentation or localization update

Testing

I tested and verified my change by using one or multiple of these methods:

• Sparkle Test App
• Unit Tests
• My own app
• Other (please specify)
  • CotEditor

1. Make sure target app implements -[NSApplicationDelegate applicationShouldOpenUntitledFile:]
2. Close all windows except any Sparkle windows, click the Dock icon.
3. -[NSApplicationDelegate applicationShouldOpenUntitledFile:] should be called.

macOS version tested: All latest major versions from 10.11-12.0 have been tested with the Test App. A version of CotEditor using the branch version of Sparkle has been tested on all latest major versions from 10.15-12.0

[zorgiepoo]

Thank you for the PR and in depth analysis and testing. It's probably good. I assume this change to NSPanel does not make any changes in behavior or activation state when it's in the background.

Please update the UI tests in SUTestApplicationTest.swift as they're currently failing. You'll want to use app.dialogs instead of app.windows in these two lines:

app.windows["SUUpdateAlert"].buttons["Install Update"].click()
/*...*/
app.windows["SUStatus"].buttons["Install and Relaunch"].click()

[zorgiepoo]

This change will also make this work correctly:

- (BOOL)applicationShouldTerminateAfterLastWindowClosed:(NSApplication *)sender
{
    return YES;
}

As panels don't appear to count here, so Sparkle's status or update windows won't prevent the application from terminating. I vaguely remember maybe iTerm running into these sort of issues (cc @gnachman) but can't find the exact issue. (edit: my bad, I was actually thinking of Keka @aonez)

Will likely cherry-pick back to 1.x if I don't see issues.

[aonez]

I was actually thinking of Keka @aonez

@zorgiepoo my issue was just the inverse, the app terminated before being updated.

[GetToSet]

This change will also make this work correctly:

- (BOOL)applicationShouldTerminateAfterLastWindowClosed:(NSApplication *)sender
{
    return YES;
}

As panels don't appear to count here, so Sparkle's status or update windows won't prevent the application from terminating. I vaguely remember maybe iTerm running into these sort of issues (cc @gnachman) but can't find the exact issue. (edit: my bad, I was actually thinking of Keka @aonez)

Will likely cherry-pick back to 1.x if I don't see issues.

Sorry that I didn't even notice there's a UI Test.

Using NSPanel can result a different behavior when apps implementing -[NSApplicationDelegate applicationShouldTerminateAfterLastWindowClosed:] to return YES and may introduce other side-effects that require developers' attention. Apple clearly documents this behavior here.

It might be better to let developers to have more real-world testing on beta branch, rather than cherry-picking it to 1.x.

[zorgiepoo]

I'll put it in a beta tag for 1.x for a short while and mention it in the release notes. I generally like back porting bug-fixes for the time being. The default behavior previously was that an app would not quit if a Sparkle panel appeared and other windows were closed by the user (if they opted into this delegate without considering Sparkle) but when you tried to install an update, the app would quit.

Thank you for this change!

[zorgiepoo]

I took more time to play with these changes but I think I prematurely merged this and came to some wrong conclusions here. Sorry.

The default behavior previously was that an app would not quit if a Sparkle panel appeared and other windows were closed by the user (if they opted into this delegate without considering Sparkle) but when you tried to install an update, the app would quit

This is wrong according to my recent tests (on macOS 12.0.1 (21A559)). When I try to implement -applicationShouldTerminateAfterLastWindowClosed: and always return YES and I only have a Sparkle update alert window open prior to these changes, if I try to install an update, it ends up working and the app doesn't get terminated unexpectedly in between. Which I think is actually the desired behavior. Now with these changes if the status window or update alert window is a panel and shown onscreen, and closing other windows terminates the application (without consideration of Sparkle), that is unexpected.

To the original issue: I think clicking the app in the dock should bring up the window that was last in key focus. If Sparkle has an update alert window or status window shown (often there is some user initiation here), clicking the dock icon should bring up Sparkle's window if it was the one in key focus. This is the case when all other document windows are closed. And this is what I expect when I want to eg switch back to iTerm - I don't expect an untitled window should open if Sparkle's status or alert window is on screen. And a developer may want to prioritize the user to give attention to updating the app.. The dock is about bringing the app back more-so than opening new documents (if you're already in the app you can open a new document from menu bar or command n or similar). I don't think Sparkle is "auxiliary" enough to have its windows be panels.

So it's a bit more complicated than what I thought. Likely I am going to revert these changes but we can continue discussion here as needed.

[GetToSet]

I took more time to play with these changes but I think I prematurely merged this and came to some wrong conclusions here. Sorry.

The default behavior previously was that an app would not quit if a Sparkle panel appeared and other windows were closed by the user (if they opted into this delegate without considering Sparkle) but when you tried to install an update, the app would quit

This is wrong according to my recent tests (on macOS 12.0.1 (21A559)). When I try to implement -applicationShouldTerminateAfterLastWindowClosed: and always return YES and I only have a Sparkle update alert window open prior to these changes, if I try to install an update, it ends up working and the app doesn't get terminated unexpectedly in between. Which I think is actually the desired behavior. Now with these changes if the status window or update alert window is a panel and shown onscreen, and closing other windows terminates the application (without consideration of Sparkle), that is unexpected.

To the original issue: I think clicking the app in the dock should bring up the window that was last in key focus. If Sparkle has an update alert window or status window shown (often there is some user initiation here), clicking the dock icon should bring up Sparkle's window if it was the one in key focus. This is the case when all other document windows are closed. And this is what I expect when I want to eg switch back to iTerm - I don't expect an untitled window should open if Sparkle's status or alert window is on screen. And a developer may want to prioritize the user to give attention to updating the app.. The dock is about bringing the app back more-so than opening new documents (if you're already in the app you can open a new document from menu bar or command n or similar). I don't think Sparkle is "auxiliary" enough to have its windows be panels.

So it's a bit more complicated than what I thought. Likely I am going to revert these changes but we can continue discussion here as needed.

Yeah it seems more complicated than I thought. Maybe there can be a less intrusive alternative to the original issue, or
making it possible for developers opt-in this behavior. I'll continue to investigate for a better approach.

[aonez]

making it possible for developers opt-in this behavior

This can be a good option. For Keka is use a combination of those for opening a new window, so I really don't care if there're other windows opened:

- (BOOL)applicationShouldHandleReopen:(NSApplication *)theApplication hasVisibleWindows:(BOOL)flag;
- (void)applicationDidBecomeActive:(NSNotification *)aNotification;
- (void)applicationDidResignActive:(NSNotification *)aNotification;

[zorgiepoo]

The first one of those delegate methods sounds like it may give the app control of what to do, maybe if it detects Sparkle sessionInProgress && canCheckForUpdates (a checkable session is in progress) or so...

I don't think there's much to do here from Sparkle's end as I think its default behavior for its standard user interface behaves expectably. NSApplication delegate customization is outside the realm of Sparkle. Another approach is to make a custom UI that more inlines the update process to a single window (eg launch the Sparkle Test App by holding down Shift, or check what Nova does #857 (comment))