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 detection of screen layout changes #44

Merged
merged 1 commit into from
Nov 18, 2018
Merged

Conversation

tmandry
Copy link
Owner

@tmandry tmandry commented Nov 18, 2018

Fixes #42.

@tmandry tmandry merged commit bdead27 into master Nov 18, 2018
@tmandry tmandry deleted the fix-screen-layout-event branch November 18, 2018 19:40
@choco
Copy link

choco commented Nov 18, 2018

Is it good to use OperationQueue.main? Wouldn't it be better to keep the queue nil so we don't force processing on the main thread?

@tmandry
Copy link
Owner Author

tmandry commented Nov 18, 2018

Is it good to use OperationQueue.main? Wouldn't it be better to keep the queue nil so we don't force processing on the main thread?

As of now, all Swindler events are dispatched on the main thread, and since this ultimately leads to an event I put it on the main queue. Otherwise, this assertion would have ended up failing.

Perhaps it would be better to allow the user to customize where event handlers are run. My thinking has generally been that the biggest slowdowns are likely going to be from the AX API calls, which halt the current thread until the requested application responds. Those are all handled on the background queue. Other processing I expect to be fast and not a real blocker, but I'd love to know what you think about this.

As for the processing going on within Swindler itself, I don't think it would be a bottleneck. It should probably happen on the same thread as the event is going out on to reduce unnecessary overhead. The only possible slowdown I can see is accessing NSScreen.screens, which gets its information from the window system. I imagine NSScreen works similarly to Swindler, caching that information and updating it when a notification fires (meaning the call returns instantly), but I don't know this for sure.

@tmandry
Copy link
Owner Author

tmandry commented Nov 18, 2018

Actually, I just remembered that there is a lot more going on than NSScreen.screens, to get display IDs for each screen and match them to the old screens. At the very least, I should add a trace to measure how long this is taking, and consider moving off main.

@choco
Copy link

choco commented Nov 18, 2018

I should probably read the whole code before commenting, but in my opinion we shouldn't tie the event system to the main thread if possible; actually giving the ability to set the preferred queue would be cool.

There were some missing notifications when we didn't use the main thread in chunkwm, but I figured those were issues with the interoperability between the C api using event loops and c++ threads not playing well together....
Right now you may not have a lot of processing, but once you'll introduce Spaces in the mix you'll unfortunately start to experience how painfully inconsistent are macOS APIs for dealing with those :(

@tmandry
Copy link
Owner Author

tmandry commented Nov 18, 2018

That's true, there will likely be a lot of internal processing to do for Spaces.

Dispatching external event handlers on one thread seemed much easier to deal with for user code, but if folks want to customize this then that's fine with me. I opened #46. Do you need concurrency or you just want to get it off the main thread specifically?

Out of curiosity, with a "headless" app like chunkwm, do you know of any specific problems encountered when processing on the main thread?

@choco
Copy link

choco commented Nov 19, 2018

Do you need concurrency or you just want to get it off the main thread specifically?

Mainly I just want to keeps as much stuff as possible of the main thread.

Out of curiosity, with a "headless" app like chunkwm, do you know of any specific problems encountered when processing on the main thread?

It's been quite a while since I read that codebase, but if I remember correctly, only the notification listening code is on the main thread. Event handlers just push the events on a queue where there are some worker threads processing these for the different registered plugins.

The main pain points arise when dealing with any user interaction. For example the border plugin, which must execute on the main thread, uses higher level Cocoa API, which don't play well when mixing threads, and the mutexes in the main thread event handling code. So you just end up dispatching everything async to the main thread to avoid any deadlocks. There are also some concurrency issues, which are hard to pinpoint leading to some glitchy behaviour probably because of some race conditions.

Ideally, I'd like to recreate the functionality of chunkwm (which imo doesn't have decent alternatives), tiling, rule system, follow focus mouse and border, but with a more user friendly UI.

@tmandry
Copy link
Owner Author

tmandry commented Jan 3, 2019

@choco Are you working on such a project now? I'm working on x3 which currently is a clone of i3 but I have a lot of the same goals as you.

@choco
Copy link

choco commented Jan 9, 2019

@tmandry Nope, haven't started working on it yet.

I'm not really familiar with i3, and in general with other tiling window manager. kwm and chunkwm were actually my first so I'm not sure what's the state of the art on linux, how much those differ and how many features are actually achievable on macOS with the constrained accessibility APIs.

All I have are some things I would like to be able to do, how I'd want some interactions to behave, but I don't know if they are the best solution for the problem. It would be nice to brainstorm some ideas and have a better understanding of what's good

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