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

Use hashtable for storage #7

Merged
merged 12 commits into from
Nov 3, 2023
Merged

Use hashtable for storage #7

merged 12 commits into from
Nov 3, 2023

Conversation

koekeishiya
Copy link
Contributor

Uses hashtable for storage instead of having separate tracked arrays for windows and borders.
Also fixes bug mentioned in #6

@koekeishiya
Copy link
Contributor Author

The hashtable is just copied out of yabai code. The rest is kinda janky again, sorry for low quality code -- most changes are kinda rushed and I have not spent too much time looking at how all the code is organized.

Again, feel free to readjust w/e before merging. You probably want to do another code formatting pass to get style that fits with your code.

@FelixKratz
Copy link
Owner

FelixKratz commented Nov 3, 2023

I think the PR has one major problem. When moving a window to a different space, there is a brief moment when two borders with the same wid exist, because the window is created at the target space first and then destroyed at the origin space. Thats why I used wid and sid checks to find the window border. I am not sure if sticky windows will fall into the same problem.
This leads to the window "loosing" its border on the target space, because it is destroyed just after being created.

So my suggestion would be to use a hash that is combined in some clever way from wid and sid to identify the window. Or the event logic needs to be adapted to capture this eventuality.

@koekeishiya
Copy link
Contributor Author

koekeishiya commented Nov 3, 2023

I believe the latest commit should adress that problem just fine.

Edit: I did not do any testing with sticky windows, that might be broken.

Edit2: I might have overlooked some technical differences in border_remove and window_remove when doing these changes, by the looks of it. Might be better to scrap this PR and do changes from scratch with the caveats you mentioned in mind from the start.

@koekeishiya
Copy link
Contributor Author

koekeishiya commented Nov 3, 2023

Don't merge this. I definitely missed something in your logic and screwed something important. Upon moving a window to a different space (and following focus), moving back to the previous space, now all other windows lack their border.

@FelixKratz
Copy link
Owner

It could also be I destroyed something during merging…

@koekeishiya
Copy link
Contributor Author

koekeishiya commented Nov 3, 2023

Ok so I am definitely breaking your logic with the changes in this PR, but it seems to work the way we want it to.
I'll let you make the decisions.

I changed the response to EVENT_UNHIDE and EVENT_HIDE to look for existing border and call SLSOrderWindow to manipulate visibility instead of it calling create/destroy. This appears to mitigate the issue I mentioned in my comment above.

Probably smart to test-drive for a while. There is likely more event logic that need adjustments.

@FelixKratz
Copy link
Owner

Ok so I have cleaned up and refactored this a bit, what do you think? I am happy with it and will merge once you approve my changes.

@koekeishiya
Copy link
Contributor Author

Looks good to me.

@FelixKratz FelixKratz merged commit 5b84be1 into FelixKratz:main Nov 3, 2023
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.

None yet

2 participants