Stage Manager: snap windows from recent apps #895
Conversation
| if StageUtil.stageCapable() && StageUtil.stageEnabled() && StageUtil.stagePresent(windowInfo) { | ||
| // In case the window is in Stage Manager recent apps | ||
| var prevWindowInfo: CGWindowInfo? | ||
| while prevWindowInfo?.id != resultWindowInfo.id { |
There was a problem hiding this comment.
I feel like I'm missing something here with this while loop. It appears to me that it is unnecessary, but perhaps you can elaborate on what it's doing?
There was a problem hiding this comment.
You can only drag the foreground window of a scene from the recent apps. This loop is used to find this window.
There was a problem hiding this comment.
Hmm. Sorry to keep pushing on this, but when I test this out, I never get a different value for the resultWindowInfo (there's no iteration in the while loop because it gets the same value twice and exits, and it's not straightforward to me how it would obtain different values in any different scenario). Even if I remove this block entirely, I can still drag & snap the windows from recent apps because it appears to get the correct windowId. Perhaps there is an edge case that I'm not hitting that would justify having this?
The reason why this caught my attention in the first place is because 1) while loops that don't have an intuitive exit are too easy to end up in an infinite loop (if not now, maybe with other additional refactoring later), and 2) I pretty much never force unwrap optionals. Both of these scenarios are potential for application crashes, and even if your code here was straightforward to me, I would have still suggested removing both of those. In other apps I've written, places where I was certain there was no chance of a force unwrap causing a crash, still caused a crash due to some very tricky memory management from macOS that had released memory I thought wouldn't get released. One could argue that a crash is desirable in certain scenarios, but I rarely have those scenarios - I'd almost always want the app to still chug along even if there's something like this that isn't quite right.
There was a problem hiding this comment.
I'll try to clarify: in the case where a background window of a scene from the recent apps is dragged (by the part that sticks out behind the other windows of the scene), it won't be that window but the foreground window of the scene that will be dragged. Returning the window under the cursor is therefore not enough, it is necessary to find the window in the foreground of the scene. To do so, we can search for the first window (because they are already sorted by layer) that intersects the window under the cursor. However, the window in the foreground does not necessarily intersect the window under the cursor depending on their positioning. It's therefore not enough either, it is necessary to go up the first windows that intersect. We know that it is no longer possible to go up when we find the same window twice.
As for the loop itself, it seems safe to me. What would you suggest?
There was a problem hiding this comment.
Thanks for the clarification, I understand now. As for the while loop, I'm good with it as-is. I appreciate that you've continued the work on Stage Manager!
|
And here's the analyzed crash report: |
Fixes #853