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

Stage Manager: only one window case #875

Merged
merged 1 commit into from
Aug 13, 2022
Merged

Stage Manager: only one window case #875

merged 1 commit into from
Aug 13, 2022

Conversation

decodism
Copy link
Contributor

@decodism decodism commented Aug 4, 2022

Fixes #873

@@ -14,5 +14,23 @@ class StageUtil {

return stageDefaults.bool(forKey: "GloballyEnabled") && !stageDefaults.bool(forKey: "AutoHide")
}

static func stageActuallyVisible() -> Bool {
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for consistently keeping on top of the feature you introduced! I really appreciate it. A couple notes on this one:

  • stageActuallyVisible is a slightly confusing name for the uninitiated, but I'm not picky and it's fine if you want to leave it. Maybe something like stageWindowPresent
  • CGWindowListCopyWindowInfo can cause delay for some users on certain macs. Calls to this function typically should be used sparingly. Checking the name/w/h is also potentially fragile. If there's another way we can do this, it might be worth exploring that first. At the very least, we would probably want to only execute this if the defaults for stage manager show it enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • stageWindowPresent is fine. Naming is not always easy.
  • I didn't know about CGWindowListCopyWindowInfo. I guess it's the same for the accessibility API. Maybe there is a private API but I am not knowledgeable about RE and it's not ideal. I don't know the code base of Rectangle either but it doesn't seem that windows are cached. Rather than checking if Stage Manager is displaying an app icon, we could just check the number of windows on screen but I don't think that really helps. Perhaps the case this PR addresses could be ignored. I updated the branch anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

Apologies for the delay, thanks for the update. You're correct that Rectangle doesn't cache windows. I'm going to go ahead and roll this in. It's probably not too likely that Macs running Ventura will be impacted by one additional call to CGWindowListCopyWindowInfo here, so I'm willing to take the gamble since I can easily troubleshoot back here if needed.

@rxhanson rxhanson merged commit 67c4a98 into rxhanson:master Aug 13, 2022
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.

Ignore stage manager spage on secondary displays
2 participants