Copy ASIO4ALL settings from old version by ann0see · Pull Request #1260 · jamulussoftware/jamulus

ann0see · 2021-03-14T21:48:12Z

Fixes #1244

ann0see

Not yet ready; added some explanations

windows/installer.nsi

ann0see · 2021-03-14T21:52:07Z

windows/installer.nsi

@@ -212,6 +218,30 @@ Section "Install_64Bit" INST_64
                MessageBox MB_OKCANCEL|MB_ICONEXCLAMATION "$(OLD_VER_REMOVE_FAILED)" /sd IDCANCEL IDOK continueinstall
                goto quit
            ${EndIf}


This code will only be executed if we find jamulus in the 32 bit prog. files folder and the current install is 64 bit.

ann0see · 2021-03-14T21:57:52Z

Feel free to edit the code.

ann0see · 2021-03-15T11:27:00Z

The latest commit makes it very simple. And seems work. We're not checking for anything, so probably it would also overwrite some other things.

ann0see · 2021-03-15T13:21:54Z

Tests so far with new code:

Install new version on top of old version from non admin account with admin privileges from admin account: If I click "yes" in the my message, the settings get transfered. Note: the new registry key doesn't exist yet.
Remove new version and install old version --> old settings are saved for the old version
Install new version on top of old version after changes have been made in the new version. Press no to the copy message on the message in the installer --> Previously set settings from the new version get saved
Remove new version, install old version (without removing registry key), afterwards install new version. New key doesn't get touched. As expected. I'll need to move the message.

Still todo:

Check what happens for the admin user (at least the new registry key seems to be created)

windows/deploy_windows.ps1

ann0see · 2021-03-15T13:29:46Z

windows/installer.nsi

+
+            ; ask to move old ASIO4ALL registry configuration
+
+            MessageBox MB_YESNO|MB_ICONEXCLAMATION "If your're using ASIO4ALL: Do you want to copy ASIO4ALL settings from the previous version if possible? We will do this for every user." /sd IDYES IDNO continueinstall


Do we need this message?

I can't think of any reason why users would not want it.
As a user, I'd expect Jamulus+ASIO4ALL to work the same after the update than before. This logic tries to do that.
The logic is supposed to be non-destructive, i.e. it should not overwrite any configs (we should be really sure about that).

So my tendency would be to not add a (possibly confusing) message. Would also have the benefit of not needing another translators ping...

Ok. Just did a force push without this change. I think now we'll just need to do some tests.

Fixes jamulussoftware#1244

ann0see · 2021-03-15T14:26:30Z

Still one edge case: What should happen if an user now sets a different installation path? Do we need to handle this?

hoffie · 2021-03-15T14:29:58Z

Still one edge case: What should happen if an user now sets a different installation path? Do we need to handle this?

That would have always been a problem, wouldn't it?

I think we could add a message on the page where the target path is chosen and warn about it ("If you change the installation path from previous installations, you may have to re-configure ASIO4ALL."). However, as this is not a regression as far as I understand) and because it would benefit from being translated, I'd suggest to postpone that part for 3.7.1?

ann0see · 2021-03-15T14:39:11Z

That would have always been a problem, wouldn't it?

I think so. Not sure, but the installation path couldn't even be saved for an upgrade in the old installer, could it?

softins

Appears to work ok on my W7 64-bit:

Installed 3.6.2
Made sure key 7A49ECC9 did not exist and 8A9E7A56 did exist.
Installed artifact from this PR. Was warned about removing the 32-bit version, which I accepted.
Checked registry and confirmed everything in 8A9E7A56 had been copied to 7A49ECC9.
Only one user on this PC had ever used Jamulus, so couldn't check for others.

ann0see · 2021-03-15T16:31:57Z

Logged in as admin:

Removed new key in registry; installed the version from the artifact --> Previous settings are restored (since the new key gets created) for admin and user.
Didn't remove new key in registry; installed the version from the artifact --> Only settings from the new version are there (the new key is not over written) for admin and user.

I think this is the most important check we need to make.

hoffie

I can't really comment on the NSH and NSI code as I have zero experience with it. I can't test on Windows either, sadly.

However, I'm confident that we should have this change and logic-wise it looks sounds.

I'd be happy if yet another person could test the installer, either explicitly or through another rc.

windows/installer.nsi

windows/NSISCopyRegistryKey/registry.nsh

Co-authored-by: Christian Hoffmann <[email protected]>

ann0see · 2021-03-15T19:50:19Z

Before merging this, I'd like to have feedback from @gilgongo everything should work as expected.

gilgongo · 2021-03-15T19:59:23Z

I can install it on my machine? It has old settings from a previous install. Where do I download it?

ann0see · 2021-03-15T20:05:01Z

I think the windows compilation process isn't finished yet, so you'll need to wait a bit. Afterwards you can find the binary on the checks page of this PR.

ann0see · 2021-03-15T20:41:30Z

I've uploaded the binary here: https://github.com/jamulussoftware/jamulus/releases/tag/untagged-964d405309c4e6a354c3

ann0see · 2021-03-15T21:26:15Z

I've merged this now since I think it works as expected.

ann0see commented Mar 14, 2021

View reviewed changes

ann0see mentioned this pull request Mar 15, 2021

Fix changing GUI style. #1267

Merged

ann0see commented Mar 15, 2021

View reviewed changes

windows/deploy_windows.ps1 Outdated Show resolved Hide resolved

ann0see commented Mar 15, 2021

View reviewed changes

ann0see requested review from gilgongo, hoffie and softins March 15, 2021 13:30

Copy ASIO4ALL settings from old version

c27a540

Fixes jamulussoftware#1244

ann0see force-pushed the MoveA4AReg branch from 4326e5e to c27a540 Compare March 15, 2021 14:22

ann0see changed the title ~~DRAFT:Copy ASIO4ALL settings from old version~~ Copy ASIO4ALL settings from old version Mar 15, 2021

ann0see marked this pull request as ready for review March 15, 2021 14:23

softins approved these changes Mar 15, 2021

View reviewed changes

ann0see self-assigned this Mar 15, 2021

softins added this to the Release 3.7.0 milestone Mar 15, 2021

hoffie approved these changes Mar 15, 2021

View reviewed changes

windows/installer.nsi Outdated Show resolved Hide resolved

windows/installer.nsi Outdated Show resolved Hide resolved

windows/installer.nsi Outdated Show resolved Hide resolved

pljones reviewed Mar 15, 2021

View reviewed changes

windows/NSISCopyRegistryKey/registry.nsh Show resolved Hide resolved

ann0see and others added 2 commits March 15, 2021 20:36

Apply suggestions from code review

7fdb24b

Co-authored-by: Christian Hoffmann <[email protected]>

Add licensing information

30df17e

pljones approved these changes Mar 15, 2021

View reviewed changes

ann0see merged commit b20c40a into jamulussoftware:master Mar 15, 2021

ann0see deleted the MoveA4AReg branch March 15, 2021 21:25


		; ask to move old ASIO4ALL registry configuration

		MessageBox MB_YESNO\|MB_ICONEXCLAMATION "If your're using ASIO4ALL: Do you want to copy ASIO4ALL settings from the previous version if possible? We will do this for every user." /sd IDYES IDNO continueinstall

Comments

Conversation

ann0see commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ann0see Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

ann0see commented Mar 14, 2021

Uh oh!

ann0see commented Mar 15, 2021

Uh oh!

ann0see commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ann0see Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

hoffie Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

ann0see Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

ann0see commented Mar 15, 2021

Uh oh!

hoffie commented Mar 15, 2021

Uh oh!

ann0see commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins left a comment

Choose a reason for hiding this comment

Uh oh!

ann0see commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ann0see commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilgongo commented Mar 15, 2021

Uh oh!

ann0see commented Mar 15, 2021

Uh oh!

ann0see commented Mar 15, 2021

Uh oh!

ann0see commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ann0see commented Mar 14, 2021 •

edited

Loading

ann0see commented Mar 15, 2021 •

edited

Loading

ann0see commented Mar 15, 2021 •

edited

Loading

ann0see commented Mar 15, 2021 •

edited

Loading

ann0see commented Mar 15, 2021 •

edited

Loading