Windows-Apps-Hub / UnitedSets

Bringing back Sets and Browser
https://www.microsoft.com/store/apps/9N7CWZ3L5RWL
MIT License
232 stars 10 forks source link

Thread safety, detaching and disposal improvements #29

Closed mitchcapper closed 1 year ago

mitchcapper commented 1 year ago

Added AsyncEnqueue for the DispatcherQueue from the community toolkit.

A series of changes to improve the reliability, mostly around window detaching/removal.

Thread safety and 'disposal' are the primary areas of concentration. With these changes I find it much more reliable on detach/close to restoring windows to original states.

Note this requires https://github.com/Get0457/WinWrapper/pull/3 to be merged to compile per the notes below (work around also below).

A more detailed set of notes that might help with why on some of the code changes:

Collection Safety

In several spots (specifically tab) collections are enumerated while they could end up changed by another thread or as we desire to remove it directly. I think this is also why several of the loops had breaks in them after single item removal. To get around the enumerating a modified collection just casting the items to an array at the start resolves. As ObservableCollection is not thread safe to make sure notifications happen any modification we should trigger on the UI thread using DispatcherQueue.

UI Thread Queuing

Added EnqueueAsync helper for DispatcherQueue from the Community Toolkit allows us to wait for the tasks to run (similar to CoreDispatcher). EnqueueAsync has perf check so only actually queues into DispatcherQueue if we are not on the UI thread.

Cache the DispatcherQueue on window creation is a fairly safe way to maintain reference to the UI threads queue without requiring an instance to stay around.

Crop Clearing

The crop was not getting unset when toggled off. Once off even though crop is set to 0 it isn't executed to be unset as the update loop itself only calls if ActivateCrop is enabled. While we could work around this rather than rely on the update loop we can clear the crop manually when we turn ActivateCrop off. This adds a ClearCrop command to the HwndHost. Note right now it is setting crop to NULL (completely reset) rather than using InitialCrop. I went back and forth here, but if somehow the crop was set by something else or some race condition with another instance of us, clearing the crop completely seemed like the best expected action (guaranteed to undo any crop that would otherwise ruin the window). This requires https://github.com/Get0457/WinWrapper/pull/3 to be applied before it will work (not only does it not allow NULL passing currently but the older ms PInvoke lib has a bug where even when told to send NULL it actually sent -1). If that may not happen soon we could just call SetWindowRgn with the right size and 0,0 to do similar.

Disposal

When a tab is detached we want to really work to restore everything back to how it was. I updated the order to go in the same reversal order.