fix(wm): improve display enum error handling and added verification step #1634
Conversation
Rejdukien
force-pushed
the
fix/wm-monitor-reconciliation
branch
2 times, most recently
from
717b276 to
21dfae7
Compare
|
Trying a different approach now after noticing it still had issues ocasionally. Now, whenever the queried monitor count differs, we're just re-querying it a bit later to essentially filter out transient events that would change count for a split moment. As it turns out, when that happens, there's also no guarantee that on followup device reconnection event, count increases back up. The windows events are overall very... unique, let's put it that way. Again, would probably be great if someone else tries this out, specifically someone who also sometimes notices a "monitor going missing" after powerplan-induced monitor sleep+wakeup. |
Rejdukien
force-pushed
the
fix/wm-monitor-reconciliation
branch
2 times, most recently
from
ef811ba to
feb1b4f
Compare
|
Now that komorebi for Mac is out I'll look at this during the week! |
|
I think this is good to merge, just rebase and make sure the latest fix in the use after free saga plays well with your changes |
…sy Win32 events This change adds verification steps and robustness improvements to the monitor reconciliator. - Retry display enumeration if it fails, up to 5 times, instead of silently ignoring errors. - When a potential monitor removal is detected, drop locks and wait briefly, then re-query the Win32 display API. If the monitor reappears, treat the event as transient and do not remove the monitor. - Re-acquire locks when needed, so the verification wait doesn't hold internal state locks. Why: - Win32 device/display notifications are noisy and not always immediately available; enumeration can fail (e.g. "Invalid monitor handle" / HRESULT 0x80070585), or multiple DBT_DEVICEARRIVAL/DBT_DEVICEREMOVECOMPLETE events can arrive in quick succession. - Without verification the reconciliator could treat transient events (for example, power-plan–induced monitor sleep where Removes+Adds occur within milliseconds) as real removals, resulting in fewer detected monitors than are actually present.
This prevents a race where OS-initiated minimizes prematurely remove windows that should be transiently restored. Problem: a display-change can trigger a a fast reconciliation path (same count), and shortly after Windows may emit a SystemMinimizeStart for affected windows. The minimize handler treated those as user-initiated and removed the window, making later reconciliation unable to restore it. Fix: timestamp display-change notifications and add a display_change_in_progress(period) check to the minimize handler. While that grace period is active the minimize handler skips remove_window(), preserving windows so the reconciliator can restore them.
Rejdukien
force-pushed
the
fix/wm-monitor-reconciliation
branch
from
feb1b4f to
59ed9ce
Compare
2 participants
This change enhances the stability of monitor detection, particularly when monitors are being connected or disconnected, by adding a verification step against the windows system metrics.
Instead of silently flattening errors during display enumeration, we now partition the results to log any errors that occur when querying display devices. This helps in diagnosing issues with specific hardware/driver constellations.
Race condition mitigation: I discovered that when a monitor connection event fires, the win32-display-data crate might not immediately see the new monitor, even though the OS event has triggered; we now verify the count of enumerated monitors against GetSystemMetrics(SM_CMONITORS). If the count mismatches, we enter a retry loop.
Reason for creating this PR: I've noticed an issue with display enumeration after wakeup from display sleep (the sort that you configure in the power plan that only affects the display, not the entire system).
What I found out is that the displays may not immediately be available for enumeration after the event fires. Usually one or two retries via the added loop are necessary, for the displays to be correctly identified.
For now I'm testing this a bit, to see if I can notice any issues, but it's looking pretty good on my system. I'm no longer having any issues with "unknown" displays after wakeup; containers/windows are where they should be etc.
It would be nice if someone else could try this out as well (especially if you noticed an unmanaged/unknown display after wakeup before), as not everyones setup or system behavior may be the same.
Marking this as a draft for now, as I would like to hear some feedback since I'm very unfamiliar with Rust - is there anything that'd be considered "ugly" in Rust or a big "no-no"? Feedback is welcome.
I had created a support thread on the discord for this, which includes additional logging snippets and my findings here: https://discord.com/channels/898554690126630914/1459645640362426572