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

Add DisplayVersion to uninstaller registry hive #2

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

frehov
Copy link
Contributor

@frehov frehov commented Jan 27, 2023

Minor convenience fix for users of winget.

Sets the String DisplayVersion to the current full version in the uninstaller registry hive located at HKCU\Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat

Reason for change.
Winget can currently install windirstat, but loses track immediately of the version and relies upon the version being available on the uninstaller entry under the key DisplayVersion

Change has been tested locally by adding the registry key manually and verifying that winget now lists the version when listing installed apps. Change has been verified by setting a lower version (1.1.1) and verifying that winget upgrade includes windirstat 1.1.2 as an available upgrade.

image

Minor convenience fix for users of winget.

Sets the String `DisplayVersion` to the current full version in the uninstaller registry hive located at `HKCU\Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat`

Reason for change.
Winget can currently install windirstat, but loses track immediately of the version and relies upon the version being available on the uninstaller entry under the key `DisplayVersion`

Change has been tested locally by adding the registry key manually and verifying that winget now lists the version when listing installed apps.
Change has been verified by setting a lower version (1.1.1) and verifying that winget upgrade includes windirstat 1.1.2 as an available upgrade.
Comment on lines 271 to 273
WriteRegDWORD HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "dwVersionMajor" "${dVersionMajor}"
WriteRegDWORD HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "dwVersionMinor" "${dVersionMinor}"
WriteRegDWORD HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "dwVersionRev" "${dVersionRev}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also include VersionMajor and VersionMinor:

+ WriteRegDWORD HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "VersionMajor" "${dVersionMajor}"
+ WriteRegDWORD HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "VersionMinor" "${dVersionMinor}"

See microsoft, nsis, comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find other instances of dwVersionMajor, dwVersionMinor, dwVersionRev and dwVersionBuild anywhere, they don't appear to be valid for the uninstall reg.
May be easier to remove dw from dwVersionMajor and dwVersionMinor (only those two, because VersionRev and VersionBuild aren't valid)
Unless those dw… keys are used somewhere else in the code. I couldn't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe those dw keys are from a this kind of struct?
https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-osversioninfoa

But I can fix the VersionMajor and VersionMinor parts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, OSVERSIONINFO is an entirely different beast, although I'll concede that the naming was probably inspired by it a bit 😉

@@ -267,6 +267,7 @@ Function CreateUninstallEntry
WriteRegExpandStr HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "InstallLocation" "$INSTDIR"
WriteRegStr HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "DisplayName" "${sVersionFull}"
WriteRegStr HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "DisplayIcon" "$INSTDIR\windirstat.exe,0"
WriteRegStr HKCU "Software\Microsoft\Windows\CurrentVersion\Uninstall\WinDirStat" "DisplayVersion" "${dVersionMajor}.${dVersionMinor}.${dVersionRev}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to use version already formatted as a string, probably ${sVersionS} or either of ${sVersion_Ansi} and ${sVersion_Unicode} if they always point to the same version

Copy link
Contributor Author

@frehov frehov Feb 13, 2023

Choose a reason for hiding this comment

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

Nice, didn't know there was a preformatted version string.
As long as it's consistent, it shouldn't really matter I think.

But can change to ${sVersionS}, I didn't find any references where it's being set, so I opted for the safe route of constructing it manually

Just to be sure I downloaded from fosshub just now, and the filename comes out as windirstat1_1_2_setup.exe but maybe the most important part is that the what is injected to the properties of the installer is the full version i.e <major>.<minor>.<revision/patch>.<build>

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure I downloaded from fosshub just now, and the filename comes out as windirstat1_1_2_setup.exe but maybe the most important part is that the what is injected to the properties of the installer is the full version i.e <major>.<minor>.<revision/patch>.<build>

The 1_1_2 string comes from ${sVersionFile}, see:

OutFile "windirstat${sVersionFile}_setup.exe"

The FileVersion in the properties of the installer (1.1.2.79) comes from ${sVersionS}, see:
VIAddVersionKey /LANG=${LANG_ENGLISH} "FileVersion" "${sVersionS}"

This for some reason is different from the version in the program's exe (1.1.2.80).

But can change to ${sVersionS}, I didn't find any references where it's being set, so I opted for the safe route of constructing it manually

Yeah, those are probably set internally, I agree that without knowing constructing it manually was the best choice.
Maybe @assarbad can say which one should be preferred ${dVersionMajor}.${dVersionMinor}.${dVersionRev}, ${sVersionS} or ${sVersion_Ansi}/${sVersion_Unicode}?

@assarbad
Copy link
Contributor

Hmm, using winget myself (and encountering the same issue with a number of other software, too).

That said, I see no issue in getting this change in. However:

  1. it doesn't help any current users and will only take effect upon next upgrade
  2. the current in-progress work uses WiX and therefore MSI (also in the hope that a Microsoft tool will be more inclined to work properly with installers using their technology ...)

Microsoft has changed the rules for what values should be written how and where so many times, it's no fun.

But this makes sense, I'll see how I can get this in, despite using Hg.

@assarbad
Copy link
Contributor

assarbad commented Feb 14, 2023

@frehov do you want to update your PR (with those two REG_DWORD values)? Otherwise I'll pull it and then add the other two values myself.

But as I pointed out, it will only really take effect for the next upgrade 😑

@frehov
Copy link
Contributor Author

frehov commented Feb 14, 2023

Sorry, life's been a bit busy.
If I haven't updated the PR within the next 24 hours, feel free to just pull it in.
I should be able to fire off a quick commit tomorrow at work.

Appreviate the feedback though :)
Stoked for any updates that may come down the line.

@Bertaz
Copy link
Contributor

Bertaz commented Feb 18, 2023

I'm going to open a new PR including @frehov's commit and my suggestions. You can either merge this one, wait for my merge from upstream, and then merge mine or merge directly mine and close this one, whichever you prefer. Former would be better for proper attribution.

@vibcodes
Copy link

Hi all, any idea when this will be fixed in the stable install? I am still facing this issue in the latest version in windows

@assarbad assarbad merged commit 65d8937 into windirstat:master Dec 18, 2023
@assarbad assarbad mentioned this pull request Jan 28, 2024
@JaneX8
Copy link

JaneX8 commented Apr 3, 2024

Can this please be fixed? I still have this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants