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

On macOS closing a window should not switch to another workspace #1379

Closed
aequitas opened this issue Feb 13, 2019 · 16 comments
Closed

On macOS closing a window should not switch to another workspace #1379

aequitas opened this issue Feb 13, 2019 · 16 comments

Comments

@aequitas
Copy link

The behaviour of Kitty when closing windows in workspaces is different from other applications on macOS.

When using workspaces on macOS. If a Kitty terminal window is open in one workspace. Closing the last Kitty window in another workspace should not shift focus to the window in the other workspace (and thereby switching to that workspace). But rather stay on the same workspace.

Also Kitty seems to focus on the previous window when closing a window, disregarding which workspace the window is in. So closing a window in a workspace which still has other Kitty windows open also causes a switch of workspace.

If you like I can make a video to clarify what I mean.

@Luflosi
Copy link
Contributor

Luflosi commented Feb 14, 2019

I can reproduce this issue and I agree it should be fixed.
If I remove the two lines from _glfwPlatformFocusWindow() from glfw/cocoa_window.m, this issue disappears, so we should figure out where this function is called when it shouldn't be called.

@kovidgoyal
Copy link
Owner

This is by design, since otherwise closing kitty windows does not focus other windows at all on macOS. See destroy_os_window() in glfw.c. If you want to fix this you would need to make that code workspace aware and restrict focusing switching to other windows on the same worksace.

@Luflosi
Copy link
Contributor

Luflosi commented Feb 14, 2019

Hmm, you're right. I'll try to look into it when I have some time.

@kovidgoyal
Copy link
Owner

@aequitas
Copy link
Author

@kovidgoyal I figured the behavior is because of that. I think making the code workspace aware would be the best solution. @Luflosi is this something you want to pick up or should I have a go at it?

@Luflosi
Copy link
Contributor

Luflosi commented Feb 14, 2019

@aequitas feel free to have a go at it, I'm quite busy atm.

@aequitas
Copy link
Author

aequitas commented Feb 14, 2019

After some experimenting I ended up with a working PoC to read workspaces: #1383

There is still some work to be done. The linkedlist stack for focus history might not be ideal for this use case so I'd have to see how to solve that. As now you might have to look deeper into the stack to find the next window (in the current workspace) to focus to.

I'll try to get it sorted out over the weekend.

@aequitas
Copy link
Author

@kovidgoyal awesome, looks a lot better than what I was going for.

I've been testing it out and in concept your changes work as expected, only this function seems to return 0 for all workspaces and 1 for the first workspace:

if (window) CGSGetWindowWorkspace(_CGSDefaultConnection(), [window windowNumber], &ans);

Which makes it effectively not work (except for workspace 1).

I'll try to investigate why as this is a direct call to the private API so it should work.

@kovidgoyal
Copy link
Owner

Sorry, I have no idea why that should happen, not really a mac expert.

@aequitas
Copy link
Author

No problem. I will investigate it further and open a PR if I find a solution or maybe it's just a fluke on my system. Thanks for implementing this.

@koekeishiya
Copy link
Contributor

@kovidgoyal @aequitas

The CGS***Workspace APIs have all been deprecated at least since the release of macOS El Capitan.
The new versions are an abstraction under the name of CGSSpace. I have written code in chunkwm to identify the (potential multiple workspaces) a window is visible on, see: https://github.com/koekeishiya/chunkwm/blob/master/src/common/accessibility/display.mm#L642

You can safely get rid of the macos_space abstraction that I have for my purposes, and have it simply return an array of workspace ids. What you would be interested in there is the SpaceId value, however note that this is an ID internal to macOS and does not map 1-1 to the incrementing desktop ID visible in mission-control. This should not matter with regards to what you are trying to achieve here.

Do let me know if you require more information and I will try to help out.

@kovidgoyal
Copy link
Owner

@koekeishiya Thanks I have changed the code to use CGSSpace.

@aequitas I have not tested my changes as I dont currently have access to my mac (beyond seeingthat theycompile) please do that.

@aequitas
Copy link
Author

@koekeishiya thanks for investigating.

@kovidgoyal I can test this out later today when I'm on my mac.

@Luflosi
Copy link
Contributor

Luflosi commented Feb 19, 2019

@kovidgoyal I don't think it works. Here is what I tried:

  1. Open kitty
  2. Open a second OS window (same workspace)
  3. Close that window again
  4. The first window doesn't get focus

I haven't looked at the code at all so I don't know what's going on.

@aequitas
Copy link
Author

@kovidgoyal I've tested current master. I think there is a error in the logic of: https://github.com/kovidgoyal/kitty/blob/master/kitty/glfw.c#L610

i starts as -1 but this causes the entire loop to be skipped.

Also window_in_same_cocoa_workspace never seems to be called.

The logic to read space ID's for windows seems to work fine as I get back a different ID for a window in each workspace.

@koekeishiya
Copy link
Contributor

koekeishiya commented Feb 19, 2019

@kovidgoyal @aequitas

I expect the i = -1 to be a typo considering it is of type size_t, thus rendering the loop meaningless. If you are testing this locally, make i start at 0 and I suspect it will all work fine.

Edit:

id_type is also an unsigned type and highest_focus_number has to be initialized to 0 instead of -1: https://github.com/kovidgoyal/kitty/blob/master/kitty/glfw.c#L611

I've made the fixes that I mentioned in this comment and confirmed that the focus change on window destruction is now workspace aware and working properly. Tested on Mojave 10.14.2

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

No branches or pull requests

4 participants