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

fix issue #1249 #1079 #1392 #1437

Closed
wants to merge 3 commits into from
Closed

Conversation

metacodes
Copy link
Contributor

@metacodes metacodes commented Mar 17, 2022

I am able to reproduce the issue by reinstalling JetBrains AppCode. The first time I open AppCode after reinstalling it, AltTab never shows AppCode window. I debug the code on my Mac, I find that AltTab can get the event of app launched but never get the event of finishedLaunching. So I just remove the code runningApplication.isFinishedLaunching. After that, AltTab work well and the issue is fixed.

The following picture is the debug log when I reproduce this issue:
image
image

Thank you for opening a PR! Please make sure that:

  • The title and description of the PR convey what the change is about, by mentioning the ticket it addresses for instance.
  • The commits messages follow the convention, so your PR can be merged.

I am able to reproduce the issue by reinstall JetBrains AppCode. The first time I open AppCode after reinstall it, AltTab never show AppCode window. I debug the code on my Mac, I find that AltTab can get the event of app launched but never get the event of finishedLaunching. So I just remove the code runningApplication.isFinishedLaunching. After that, AltTab work well and the issue is fixed.
@lwouis
Copy link
Owner

lwouis commented Mar 17, 2022

@metacodes thanks for sharing this

The check is there for a reason: so that we know that the app is finished launching. This is the signal for us to ask for its windows and subscribe to its events.

It's likely that removing this check would break things. Could you please look at the git history for examples of issues the check was mitigating?

I advice you test with a variety of apps like Gimps for example which is slow to launch. See if your change has not broken things there.

Lastly, an app should definitely send this event, so these apps should probably work on the issue instead of us covering up / working around their bugs. That being said AltTab has been pragmatic often so that people have a good UX despite buggy apps.

@metacodes
Copy link
Contributor Author

@lwouis That makes sense.

I have look at the git history and I found that you added these code to solve some background processes. But in the function observeNewWindows there also has runningApplication.isFinishedLaunching, so I think maybe remove the code runningApplication.isFinishedLaunching in the function addAndObserveWindows is safe. We just need to run observeEvents() to work around the issue they never send the finishedLaunching event.
image

I try to test with a variety of apps. You said Gimps. Do you mean this app(https://www.gimp.org)? I have already test it, it seems work fine. I will test this code for a few days to see if the change has not broken things. I will report the result after that.

@lwouis
Copy link
Owner

lwouis commented Mar 17, 2022

It's possible that checking for isFinishedLaunching is not necessary before subscribing to the app events, yes. This comment I left here actually says that we don't even trust this boolean flag at the end of the day, and rely on a subscription to actually complete to consider the app "launched". That's why we have this ridiculous boolean flag isReallyFinishedLaunching.

So yes, maybe your removal of the check in that specific spot is safe. However, better test it carefully, as it could break the app for everyone.

Regarding testing, yes I was referring to Gimp, sorry about the typo. Furthermore, I looked into past tickets, and here are some apps I think are worth testing with your change:

@metacodes
Copy link
Contributor Author

@lwouis Currently, I have tested Bear.app, Protege, Books.app. Only Bear.app has some problems, sometimes AltTab still can not show Bear's window. But AltTab received the event isFinishedLaunching. The problem is the axWindows_.count gets 0. When I add sleep(3) before that, axWindows_.count will get the right count of windows and AltTab shows Bear's window. I guess there are some problems with the function retryAxCallUntilTimeout, but I don't know how to solve this.
Here is the debug code.
image

Could you give me some advice?

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. At this scenario, AXUIElementCopyAttributeValue(self, kAXWindowsAttribute, &value) returns success but  AXUIElementCopyAttributeValue(self, kAXPositionAttribute, &value) and AXUIElementCopyAttributeValue(self, kAXSizeAttribute, &value) return attributeUnsupported. After we wait for a moment, all things run well.
@metacodes
Copy link
Contributor Author

@lwouis I have added some codes to fix Bear.app's issue. Its scenario is different from other apps(Octave.app, Protege.app, Books.app, Firefox.app, JetBrains' app). You can find more details about this issue in my commit comments. For now, these apps(Gimp.app, Bear.app, Octave.app, Protege.app, Books.app, Firefox.app, JetBrains' app) work well after testing. And it seems no impact to other apps which are work well.
image

@metacodes
Copy link
Contributor Author

#1439 I have also tested Sublime Merge.app . Everything works well.

@metacodes
Copy link
Contributor Author

After a week of testing, everything is fine. As Apple says that some applications do not post this notification and so are never reported as finished launching, I think it's reasonable to remove the check of isFinishedLaunching before we create a new observer that can receive notifications from the specified application.
image

@lwouis
Copy link
Owner

lwouis commented Mar 26, 2022

I'll try and test/merge this as soon as possible. I'm struggling to find time these days (see #1179). Please forgive the delays. Hopefully i can finish this soon

@@ -117,6 +117,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

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

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.

@lwouis
Copy link
Owner

lwouis commented Apr 7, 2022

I've embedded these commits in the next release. I committed them slightly altered, but with your author name/email, so you're properly credited. You will also be listed as a contributor on the website and in the app 👍 Closing this PR

@lwouis lwouis closed this Apr 7, 2022
@metacodes
Copy link
Contributor Author

Cool 🚀
Thanks for constructive feedback and taking care of mentioned adjustments.

@lwouis
Copy link
Owner

lwouis commented Apr 9, 2022

@metacodes it seems that my worries about CPU usage were founded :/ I woke up to these tickets, after yesterday new release:

@lwouis lwouis mentioned this pull request Apr 9, 2022
@metacodes
Copy link
Contributor Author

@lwouis sorry about that.😭 I will try to solve this issue as soon as possible.

@lwouis
Copy link
Owner

lwouis commented Apr 9, 2022

@metacodes no worries! This is volunteer work. We don't have a QA team to avoid these scenarios. It's unfortunate, but it is expected given the limited resources we have. So no need to feel guilty or anything.

Now if you find a mitigation for this, it's a win for everyone ;)

@metacodes
Copy link
Contributor Author

metacodes commented Apr 9, 2022

@lwouis I found that the high cpu usage was due to the fact that we kept trying to subscribe to notifications for certain processes, but they were background programs with no windows. (e.g. com.apple.universalcontrol, org.mozilla.plugincontainer)
image

image

For each such process, AltTab would spend 120s to retry. The whole process is actually doing useless work. But for those programs that take a long time to load, this retry is necessary again. I think it is not necessary to keep retrying all the time, we can let the program take a break and retry again, although in rare cases there may be a slight delay in the creation of the window.
image
image

Retries don't happen for most apps, it happens only for those apps which takes for a long time to launch(e.g. JetBrains, Bear.app, Firefox.app etc.). So I add Thread.sleep(forTimeInterval: 1) in the function retryAxCallUntilTimeout_. That can solve the high cpu usage issue. For processes that do have no windows, we avoid doing useless retries. For those apps(e.g. JetBrains, Bear.app, Firefox.app etc.), in the worst case, there is only a 1s delay in displaying the window, which I think is acceptable.

func retryAxCallUntilTimeout_(_ group: DispatchGroup?, _ timeoutInSeconds: Double, _ fn: @escaping (	) throws -> Void, _ startTime: DispatchTime = DispatchTime.now()) {
    do {
        try fn()
        group?.leave()
    } catch {
        Thread.sleep(forTimeInterval: 1) // sleep for 1s to reduce CPU usage
        let timePassedInSeconds = Double(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) / 1_000_000_000
        if timePassedInSeconds < timeoutInSeconds {
            BackgroundWork.axCallsQueue.asyncAfter(deadline: .now() + .milliseconds(10)) {
                retryAxCallUntilTimeout_(group, timeoutInSeconds, fn, startTime)
            }
        }
    }
}

What do you think?

EDIT: I removed Thread.sleep(forTimeInterval: 1) and changed BackgroundWork.axCallsQueue.asyncAfter(deadline: .now() + .milliseconds(10)) to BackgroundWork.axCallsQueue.asyncAfter(deadline: .now() + .milliseconds(1000)):

func retryAxCallUntilTimeout_(_ group: DispatchGroup?, _ timeoutInSeconds: Double, _ fn: @escaping (	) throws -> Void, _ startTime: DispatchTime = DispatchTime.now()) {
    do {
        try fn()
        group?.leave()
    } catch {
        let timePassedInSeconds = Double(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) / 1_000_000_000
        if timePassedInSeconds < timeoutInSeconds {
            BackgroundWork.axCallsQueue.asyncAfter(deadline: .now() + .milliseconds(1000)) {
                retryAxCallUntilTimeout_(group, timeoutInSeconds, fn, startTime)
            }
        }
    }
}

This was referenced Apr 9, 2022
@lwouis
Copy link
Owner

lwouis commented Apr 10, 2022

Increasing the time between retries mitigates the issue, but is not really addressing the issue directly.

As i understand you, it seems that com.apple.universalcontrol never succeeds in subscribing. Is there some metadata/flag on that process that we could use to tell that it will never succeed to subscribe? Maybe the error code from the subscription call is an indication? Anything to be able to discriminate these processes that can never succeed?

@metacodes
Copy link
Contributor Author

metacodes commented Apr 10, 2022

Maybe the error code from the subscription call is an indication?

@lwouis I've considered it, but currently these processes return the value cannotComplete, which is the same as those that have windows but load very slowly. I will continue to try to find some other methods based on your suggestion.

@metacodes
Copy link
Contributor Author

Some LSUIElement app has windows (e.g. menu bar app). For those apps' window will disappear. Further research is needed.

@metacodes
Copy link
Contributor Author

Some LSUIElement app has windows (e.g. menu bar app). For those apps' window will disappear. Further research is needed.

I found it interesting that Apple's default application switcher doesn't show these windows either.

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.

2 participants