Skip to content
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(installer): Re-add URL protocol registration registry keys on install #5521

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

magneticflux-
Copy link
Contributor

@magneticflux- magneticflux- commented Jan 26, 2022

In the installer refactor (88e1786) these registry keys were removed, so new installs did not register a URL protocol and users weren't able to use mumble:// links.
This is a direct translation of Files.wxs to equivalent WixSharp code.

Closes #5473

Checks

@Krzmbrzl Krzmbrzl requested a review from a user January 26, 2022 07:17
@Krzmbrzl
Copy link
Member

Did you verify yet that this does indeed make the mumble-URLS work again?

@Krzmbrzl Krzmbrzl added bug A bug (error) in the software installer labels Jan 26, 2022
@magneticflux-
Copy link
Contributor Author

Well I borked my local build just as I thought I fixed it, so I'm trying to download the PR artifact, but that's not working now either.

@magneticflux-
Copy link
Contributor Author

Looks like the File Table isn't being populated the same, so [#mumble.exe] doesn't work anymore.

@magneticflux-
Copy link
Contributor Author

magneticflux- commented Jan 26, 2022

Okay, it builds, installs, and the registry keys work now!

I just needed to add an explicit ID to the file. (The path is still the same, it just gives it a name in the File Table so other parts of the installer can reference it with [#mumble.exe])

@Krzmbrzl
Copy link
Member

Great! 👍

@magneticflux-
Copy link
Contributor Author

magneticflux- commented Jan 26, 2022

Should I also cherry-pick a backport PR to 1.4.x, or does that get done internally?

@Krzmbrzl
Copy link
Member

I will do that once @ZeroAbility has reviewed this PR. I have an automation tool for that, so it's done very quickly. But thanks for the offer!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a direct translation of Files.wxs to equivalent WixSharp code.

I would clarify this comment. It sounds like you added everything in the old WiX project instead of just adding the registry entries.

If the resulting installer works and the desired behavior occurs when clicking a URL, then I see no reason not to approve it.

…tall

In the installer refactor (88e1786) these registry keys were removed, so new installs did not register a URL protocol and users weren't able to use `mumble://` links.
This is a translation of the registry values from `Files.wxs` to equivalent WixSharp code.

Closes mumble-voip#5473
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this change. A second pair of eyes would be good before merging.

@davidebeatrici
Copy link
Member

I assume the registry keys created by the installer are automatically removed on uninstall, right?

@ghost
Copy link

ghost commented Jan 26, 2022

@magneticflux-
Copy link
Contributor Author

The installer should already track and remove the registry keys it creates (according to the Wix site here: https://wixtoolset.org/documentation/manual/v3/xsd/wix/registrykey.html).

I think the "force" option is only needed if we want to remove something that wasn't put there by this installer.

@ghost
Copy link

ghost commented Jan 27, 2022

@magneticflux- Sometimes WiX is not as advertised, and they are not always very responsive when it comes to bugs. If you are able to just install Mumble fresh without the registry entry and do an uninstall to test removal of the registry entry, I think we would all consider that a sign off.

@ghost ghost assigned Krzmbrzl Jan 27, 2022
@magneticflux-
Copy link
Contributor Author

@ZeroAbility Just confirmed that a fresh install and uninstall removes the registry keys.

@Krzmbrzl Krzmbrzl merged commit 516d941 into mumble-voip:master Jan 27, 2022
@Krzmbrzl
Copy link
Member

Thanks for fixing this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software installer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internet links with protocoltype mumble:// not working
3 participants