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.

Sign up for GitHub

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

fix issue #1249 #1079 #1392 #1437

Closed
wants to merge 3 commits into from
Closed

fix issue #1249 #1079 #1392 #1437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d058db6
fix issue #1249 #1079 #1392
metacodes Mar 17, 2022
5d33d3a
fix: some app is active but can not get windows
metacodes Mar 20, 2022
c68b298
fix: the variable wasLaunchedBeforeAltTab is not initialized correctly
Mar 29, 2022
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
10 changes: 8 additions & 2 deletions src/logic/Application.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ class Application: NSObject {
return n
}

init(_ runningApplication: NSRunningApplication) {
init(_ runningApplication: NSRunningApplication, _ wasLaunchedBeforeAltTab: Bool = false) {
self.runningApplication = runningApplication
self.wasLaunchedBeforeAltTab = wasLaunchedBeforeAltTab
pid = runningApplication.processIdentifier
super.init()
isHidden = runningApplication.isHidden
Expand Down Expand Up @@ -72,7 +73,7 @@ class Application: NSObject {
}

func addAndObserveWindows() {
if runningApplication.isFinishedLaunching && runningApplication.activationPolicy != .prohibited && axUiElement == nil {
if runningApplication.activationPolicy != .prohibited && axUiElement == nil {
axUiElement = AXUIElementCreateApplication(pid)
AXObserverCreate(pid, axObserverCallback, &axObserver)
debugPrint("Adding app", pid ?? "nil", runningApplication.bundleIdentifier ?? "nil")
Expand Down Expand Up @@ -117,6 +118,11 @@ class Application: NSObject {
if group == nil && !self.wasLaunchedBeforeAltTab && CGSSpaceGetType(cgsMainConnectionId, Spaces.currentSpaceId) == .fullscreen {
throw AxError.runtimeError
}
// workaround: sometimes some apps are launched and are actived but OS returns the windows.count == 0. we wait for a moment
// for example: Bear.app
if group == nil && !self.wasLaunchedBeforeAltTab && self.runningApplication.isActive {
Copy link
Owner

@lwouis lwouis Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if we need self.runningApplication.isActive here. An app could be launching in the background, and still benefit from these retries, no?

The way I think of it is: an app may launch but not have its windows exposed instantly. In that case we want to retry until the 5s timeout.

Note that it may be bad to waste CPU/battery on retrying for 5s, but the case of an app that launches but has no window to show seems very unlikely. So in most cases, I expect the retries to be a good thing, and make the windows visible within a short delay. Whereas the cases where the app actually has no window to show and waste 5s of CPU/battery should be rare.

I'm thinking that we may simplify both if above:

// workaround: opening an app while the active app is fullscreen; we wait out the space transition animation
if group == nil && !self.wasLaunchedBeforeAltTab && CGSSpaceGetType(cgsMainConnectionId, Spaces.currentSpaceId) == .fullscreen {
  throw AxError.runtimeError
}
// workaround: sometimes some apps are launched and are actived but OS returns the windows.count == 0. we wait for a moment
// for example: Bear.app
if group == nil && !self.wasLaunchedBeforeAltTab && self.runningApplication.isActive {
  throw AxError.runtimeError
}

with a single:

// workaround: some apps launch but have no window ready instantly. It's very unlikely an app would launch with no window
// so we retry until timeout, in those rare cases (e.g. Bear.app)
if group == nil && !self.wasLaunchedBeforeAltTab {
  throw AxError.runtimeError
}

I think the reason why an app is not launching with windows is irrelevant in the end. We should just retry, and it's very unlikely there are really no window. And worst case scenario, we wasted 5s of retrying.

What do you think?

Beyond that, the PR looks solid to me, and once we agree on what to do here, I'll merge and release

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 agree with you. I have also done some tests base on your changes. But there are two problem.

Firstly, the variable wasLaunchedBeforeAltTab is not initialized correctly when AltTab was first launched. The value should be true for all of the apps launched before AltTab. But I found that the value always false. That cause AltTab throws lots of AxError.runtimeError when it was launching and the CPU usage was very high(20%~50%). I will add some code to fix this issue.
image

Secondly, there are some apps that have menu bar only and launched without window. In my case, it's a screenshot app called iShot.app. The app launches but has no window. When I use it to capture the screen, the AltTab throws lots of AxError.runtimeError.
image

Base on that two scenario mentioned above, I think we can't simplify both if with a single one although it's very unlikely there are really no window. We need to make sure that the app really has window, so that the retries will be a good thing.

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 have already fixed the issue that the variable wasLaunchedBeforeAltTab is not initialized correctly.

Copy link
Owner

@lwouis lwouis Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We need to make sure that the app really has window, so that the retries will be a good thing.

there is no way to know that tough. In your example, iShot could have no window 99% of the time, but still have a Preferences window that the user can display.

when an app just launched, there is no possible way to know if that app will eventually show a window. It could take 10min to do so, if it's a mathematical simulation for example.

That's why alttab tries to get window on launch, but then also subscribe to notifications for when the app creates windows later.

The tradeoff here is: we don't retry but risk missing late windows that come after launch (but as part of the launch, so without a notification) VS retrying to ensure we get those but at the cost of CPU/resources

I don't think there is any way to win in both sides here

the current situation is that alttab ignores window that are not there at launch and not part of a later notification. What Bear.app does is a bug in my book, and AltTab retrying to get its windows is a compromise to help users but as comes at the cost of potential retries for every other app.

We could keep the status quo here to be honest, and these apps fix themselves, instead of penalizing every alttab users with more resource usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I accept the retry option. In my mind, I just want to reduce the case we retry so that we can reduce CPU usage. But if we add isActive check here, we may miss some windows as some scenarios that we did not expect.
So I accept your changes here.
Do I need to change the code? Or you can change and merge it directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could keep the status quo here to be honest, and these apps fix themselves, instead of penalizing every alttab users with more resource usage.

Frankly, I came to modify the code because the window loss problem has been bothering me for a long time, and I tried to find other alternative applications, but nothing met my needs, so I finally took the plunge and learned Swift and try to solve the problem.

I don't think these problematic apps will be fixed in future versions. Because for them, they probably won't be aware of these problems without AltTab users giving them feedback, and even if they do, they'll probably think it's not a functional problem and ignore it. For AltTab users, they don't think it's a problem with other apps, they probably think it's a functional bug in AltTab.

So personally, I'd like to see a compromise solution to solve these app bugs, maybe even OS bugs, although these compromise solutions will have some maintenance costs and even negative effects in the future. It depends on how we balance the two. But for now we can control the scope of this negative impact, such as the processing of specific applications, although these special codes are annoying in terms of engineering structure and code aesthetics, and these codes will also incur some maintenance costs in the future.

throw AxError.runtimeError
}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/logic/Applications.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Applications {
}

static func addInitialRunningApplications() {
addRunningApplications(NSWorkspace.shared.runningApplications)
addRunningApplications(NSWorkspace.shared.runningApplications, true)
}

static func addInitialRunningApplicationsWindows() {
Expand All @@ -39,10 +39,10 @@ class Applications {
}
}

static func addRunningApplications(_ runningApps: [NSRunningApplication]) {
static func addRunningApplications(_ runningApps: [NSRunningApplication], _ wasLaunchedBeforeAltTab: Bool = false) {
runningApps.forEach {
if isActualApplication($0) {
Applications.list.append(Application($0))
Applications.list.append(Application($0, wasLaunchedBeforeAltTab))
}
}
}
Expand Down