notepad-plus-plus / nppShell

Provide Explorer context menu entry "Edit with Notepad++"
GNU General Public License v3.0
25 stars 13 forks source link

Move sparse package registration into separate thread, to fix threading issue #23

Closed GurliGebis closed 1 year ago

GurliGebis commented 1 year ago

This fixes the problem in #18 by moving the registration and unregistration of the sparse package away from the main thread. Please see #18 for more info - @pjaholkowski should be able to provide more info on the issue.

GurliGebis commented 1 year ago

@donho I have rebased this one, so it should be ready to merge.

donho commented 1 year ago

@GurliGebis This PR is not yet validated. I need only the PR which fix the the crash. Could you create a new PR with the 2nd commit please?

FYI: I'll be on the trip tomorrow morning, and I won't bring my surface 1 on which windows 8.1 installed. So it's not possible to validate the fix during my trip.

GurliGebis commented 1 year ago

@donho I'll create a new branch with only that one. Would a video of me testing it on Windows 8.1 be fine for you to validate it working?

donho commented 1 year ago

@GurliGebis

I'll create a new branch with only that one.

It won't be necessary, I did a cherry-pick and test it last night. It's OK under Win8.1 now - good job.

For this PR (of the first commit), I will build a debug version and reproduce the assert warning under Win11. Once it's validated, the PR will be merged.

Please don't mix fixes in a PR next time, for the sake of facility of code review. Thank you.

GurliGebis commented 1 year ago

@donho sure, no problem 🙂 Let me know if you are able to make it crash as well without the winrt stuff being on another thread.

donho commented 1 year ago

@GurliGebis Validated. I can reproducethe assert warning during the installation, and the PR fixes it.

donho commented 1 year ago

http://download.notepad-plus-plus.org/repository/MISC/nppShell.TEST20/

GurliGebis commented 1 year ago

Perfect, thanks @donho