-
Notifications
You must be signed in to change notification settings - Fork 14
Fix Python crash when hiding the title bar of more windows #67
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes a crash when hiding the title bar on multiple windows by making the stored window procedure callbacks unique per window handle and updating the restore logic accordingly. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- Instead of dynamically creating per-window global names like
old_wndproc_{hwnd}, consider keeping a dedicated dict keyed byhwnd(e.g._wndprocs[hwnd] = {...}) to avoid polluting the global namespace and make lifecycle management clearer. - When storing per-window procedures in globals keyed by
hwnd, think about cleanup for windows that are neverunhided (or that are destroyed) to avoid leaking entries if this code is used with many short-lived windows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of dynamically creating per-window global names like `old_wndproc_{hwnd}`, consider keeping a dedicated dict keyed by `hwnd` (e.g. `_wndprocs[hwnd] = {...}`) to avoid polluting the global namespace and make lifecycle management clearer.
- When storing per-window procedures in globals keyed by `hwnd`, think about cleanup for windows that are never `unhide`d (or that are destroyed) to avoid leaking entries if this code is used with many short-lived windows.
## Individual Comments
### Comment 1
<location> `hPyT/hPyT.py:209-211` </location>
<code_context>
- if globals().get("old_wndproc") is not None:
- set_window_long(hwnd, GWL_WNDPROC, globals()["old_wndproc"])
- globals()["old_wndproc"] = None
+ if globals().get(f"old_wndproc_{hwnd}") is not None:
+ set_window_long(hwnd, GWL_WNDPROC, globals()[f"old_wndproc_{hwnd}"])
+ globals()[f"old_wndproc_{hwnd}"] = None
# Restore the original height if no_span was used
</code_context>
<issue_to_address>
**issue (bug_risk):** `new_wndproc_*` entries are never cleared, which can leak per-window state.
In `unhide`, you reset `old_wndproc_{hwnd}` but leave any `new_wndproc_{hwnd}` entry in `globals()`. This leaves stale wndproc references for windows that are no longer hooked. When unhooking, please also clear or delete the corresponding `new_wndproc_{hwnd}` entry so per-window state is fully released.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hi, @Valer100 nice work! Just a question, why cant we use dictionaries instead of dynamic vars? |
|
We can use dictionaries too, but I didn't think about it first, because I've seen that the procs were stored inside dynamically created global variables before. If you want, I can modify the code to store them in 2 separate dictionaries, one for the old ones and another one for the new ones. |
6c91b1e to
0c782bd
Compare
I found out why the crash in #59 was happening while experimenting with window procedures in a project of mine: it is because the code stores the window procedures (the old and the new ones) in the same variables, regardless of the window. When storing the second window's procedures, the procedures of the first one are lost and that's why the crash is happening. This PR addresses this issue by storing the window procedures in unique variables for every window. Fixes #59.
Summary by Sourcery
Ensure window procedure hooks are stored and restored per-window to prevent crashes when manipulating multiple windows.
Bug Fixes: