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

Terminate Xcodes app after last window closed #621

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Oct 11, 2024

Close #620

Copy link
Contributor

@MattKiazyk MattKiazyk left a comment

Choose a reason for hiding this comment

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

@Kyle-Ye Thanks for the PR!

I'm ok adding this in if a setting gets put inside of Setting > General with the default being not to do this. Thanks

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 13, 2024

Got it. I can accept adding it into Setting > General with the default being not to do this. So it at least got fixed on my side after I enable it on Setting.

But also found the root cause is that after we migrate into SwiftUI lifecycle. We get automatic multi window support for the main Window using WindowGroup.

https://developer.apple.com/forums/thread/710376

Changing into macOS 13's nonisolated public init(_ titleKey: LocalizedStringKey, id: String, @ViewBuilder content: () -> Content) API will disable the multi window support for the main Window and will also fix the issue I mentioned above.

-        WindowGroup("Xcodes") {
+        Window("Xcodes", id: "main") {

Does supporting multi main window a feature we'd like to support? If not, I'd prefer we just switch to the new API and kill the multi window feature.

image

@Kyle-Ye Kyle-Ye requested a review from MattKiazyk October 13, 2024 04:12
@MattKiazyk
Copy link
Contributor

@Kyle-Ye I'm for not having multi window as we don't need it, however I don't want Xcodes to close on X with this change to Window unless the user wants it to.

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 16, 2024

I'm for not having multi window as we don't need it

Got it. But if we do not want multi window support. The WindowGroup -> Window change is needed.

however I don't want Xcodes to close on X

If the app does not have multi windows support, the default behavior is terminate the app after last window is closed. But we can override the behavior with override applicationShouldTerminateAfterLastWindowClosed and return false here.

eg.
1️⃣. Multi Windows support: (The current main branch behavior)

  • 1 window -⌘-N-> 2 window -⌘-N-> 3 window
  • 1 window -⌘-W-> 0 window(Still alive) -⌘-N-> 1 window
  • 1 window -⌘-Q-> terminate app

2️⃣. Multi Windows support with the option to terminate on when last window is closed: (applicationShouldTerminateAfterLastWindowClosed return the value we set on Settings)

  • 1 window -⌘-N-> 2 window -⌘-N-> 3 window
  • 1 window -⌘-W->
    • (setting option value is true) 0 window and terminate
    • (setting option value is false) 0 window and not terminate -⌘-N-> 1 window
  • 1 window -⌘-Q-> terminate app

3️⃣. Without multi Windows support: (WindowsGroup -> Windows)

  • 1 window (No ⌘ N support)
  • 1 window -⌘ W-> 0 window and terminate app by default
  • 1 window -⌘-Q-> terminate app

4️⃣. Without multi Windows support and do not terminate: (WindowsGroup -> Windows && applicationShouldTerminateAfterLastWindowClosed return false)

  • 1 window (No ⌘ N support)
  • 1 window -⌘ W-> 0 visible window and does terminate the app (Still have dot on dock since the window is not actually closed in this situation) -> click the app icon to restore the closed window
  • 1 window -⌘ Q-> terminate app

The round 1 review suggestion seems is using method 2️⃣.

And the latest commits of the PR is using method 3️⃣.

IIUC, you are suggesting we should use method 4️⃣?

@MattKiazyk
Copy link
Contributor

4️⃣. Without multi Windows support and do not terminate: (WindowsGroup -> Windows && applicationShouldTerminateAfterLastWindowClosed return false)

1 window (No ⌘ N support)
1 window -⌘ W-> 0 visible window and does terminate the app (Still have dot on dock since the window is not actually closed in this situation) -> click the app icon to restore the closed window
1 window -⌘ Q-> terminate app

Correct. 4️⃣ would be the preference with applicationShouldTerminateAfterLastWindowClosed be a user setting that if users wanted it to terminate on close they could do so.

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 16, 2024

4️⃣. Without multi Windows support and do not terminate: (WindowsGroup -> Windows && applicationShouldTerminateAfterLastWindowClosed return false)

1 window (No ⌘ N support) 1 window -⌘ W-> 0 visible window and does terminate the app (Still have dot on dock since the window is not actually closed in this situation) -> click the app icon to restore the closed window 1 window -⌘ Q-> terminate app

Correct. 4️⃣ would be the preference with applicationShouldTerminateAfterLastWindowClosed be a user setting that if users wanted it to terminate on close they could do so.

Got it. But the default OS behavior would be close (The same as we return true for applicationShouldTerminateAfterLastWindowClosed) - see method 3️⃣.

So I should use 4️⃣ with applicationShouldTerminateAfterLastWindowClosed be a user setting and the default value set to false, right?

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 16, 2024

Update method 4️⃣ implementation with the applicationShouldTerminateAfterLastWindowClosed settings.

I am not familiar with all language Xcodes support. (Only add localization for language I know.)

Do I need to use some AI service/translator to translate the new LocalizedString keys here? I can see XCStrings Validation / test (pull_request) CI is failing here.

@MattKiazyk
Copy link
Contributor

@Kyle-Ye for the keys you've added, please paste the English translation to each language in the value.

The app isn't hooked up to any automated translation service, so we rely on the community to see the English in their app and make PR's.

Without adding the default translation other languages see the key as the value.

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 16, 2024

@Kyle-Ye for the keys you've added, please paste the English translation to each language in the value.

The app isn't hooked up to any automated translation service, so we rely on the community to see the English in their app and make PR's.

Without adding the default translation other languages see the key as the value.

Done. Also fix another tiny issue for pl Localization.

Move from advanced pane into general
@Kyle-Ye Kyle-Ye requested a review from MattKiazyk October 16, 2024 16:33
Copy link
Contributor

@MattKiazyk MattKiazyk left a comment

Choose a reason for hiding this comment

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

@Kyle-Ye thanks for your time and going back and forth on this!

@MattKiazyk MattKiazyk added the enhancement New feature or request label Oct 18, 2024
@MattKiazyk MattKiazyk merged commit 283c1a4 into XcodesOrg:main Oct 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Xcodes does not quit after last window closed
2 participants