-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix file associations not updating or uninstalling #29743
Conversation
private static void configureWindows(VelopackApp app) | ||
{ | ||
app.WithFirstRun(_ => WindowsAssociationManager.InstallAssociations()); | ||
app.WithAfterUpdateFastCallback(_ => WindowsAssociationManager.UpdateAssociations()); |
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.
Shouldn't this be enough to guarantee a fix after a single release? I don't understand the concern in the PR description regarding "two releases".
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.
I can't say for certain, which is why I err on the side of caution of keeping the logic for 2 releases. It shouldn't have much overhead, just becomes useless after a certain number of releases.
The reason I say two releases is because me mistaking installation for updating is what got us in this mess in the first place. My assumption is that these hooks will not run when users update from the current release to the next one, because the hooks aren't installed. The next release would install these hooks, then the release after that would actually end up running them.
If you want an immediate solution that's guaranteed to fix associations, just run WindowsAssociationManager.UpdateAssociations
every time the game starts up for the next release only.
Regardless, you can always just release and evaluate. I'd suggest doing a release and seeing if it fixes the issue for the majority of players. If so, then remove it in the subsequent release. If not, keep it for another, then remove it after that.
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.
Regardless, you can always just release and evaluate. I'd suggest doing a release and seeing if it fixes the issue for the majority of players. If so, then remove it in the subsequent release. If not, keep it for another, then remove it after that.
I'd hope this would be tested before release and guaranteed to work. Can I assume you haven't tested and that is on me to do?
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.
I have not yet tested it. But I'm willing to do that right now if needed.
Though, it'd take me a bit of time to get my environment set up (need to create a windows vm, build, transfer files, create github pat & repo, etc). I'd be looking at maybe an hour to get an environment ready.
Depending on how fast you want this out, you may test it yourself, or leave it for me. Let me know what you'd prefer.
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.
Put it this way: I wouldn't PR anything without testing myself. Do whatever you want with that information.
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.
Noted, will test it ASAP. Sorry for the difficulties, I'll let you know when I've made sure this works as intended.
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.
Pre-Update (modified to have pippi as part of path):
All of my tests pass on Windows 11 x64 using releases from smallketchup82/osu. It should only require one release as per my testing. Everything should be good to go.
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.
Thanks for testing 🙏
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.
Tested locally with a bit of a manual setup, seems to work as expect and fix the issue.
Closes #29730
This PR fixes file associations not being updated and not uninstalling properly post-velopack.
This isn't really the "big file association refactor" that I had planned to do. Primarily because I'm unsure of where the core team stands on #29603. If it's something that we would like, I'm happy to make another PR adding it in. Please let me know what you'd like to do there. The association logic looks good to me otherwise so this is all I'll be doing for now.
Testing Checklist
Notes
current
directory, in which the logic can be safely removed (or kept if you don't care about the unnecessary overhead)