Associate with files and URIs on windows #27001

Susko3 · 2024-02-03T16:51:40Z

Supersedes Associate with files & urls on Windows #26658
Closes Game does not associate with files on windows #14824

Fixed problems from the previous PR

URL associations are implemented
added localisable file descriptions
added file icons
registry keys are properly disposed
implementation is testable in a reasonable way
SHChangeNotify is now called in a non-blocking way
fixed stable registry keys taking priority (this is handled by the windows explorer -- it'll ask the user (once) in which program to open)

Problems with this PR

(Fixed) associations are re-created every time on startup (it's not great, but it probably doesn't matter that much)
(Fixed) associations clash with getStableInstallPathFromRegistry()
- if stable uses HKEY_LOCAL_MACHINE, maybe we could use that there instead. since this PR saves to HKEY_CURRENT_USER
localised file association descriptions are never written

Testing

~~Can be done trough the added test scene. Only manual testing is available.~~

~~Best way to test in the test scene is to install associations, then try the open/present beatmap/url. You can also uninstall and then check that associations don't work.~~

~~For now, uninstalling is only available trough tests and it's not possible to uninstall from the game.~~

Can be tested by running static methods in WindowsAssociationManager.

I have tested a full deploy+install on a test PC and it's working perfectly!

Helps to reduce clutter when developing

Susko3 · 2024-02-05T12:10:33Z

Hmm, I'm not sure how to make this testable now that ppy/osu-resources#308 is closed and icons are supposed to be in osu.Desktop... Maybe move the current code to osu.Game.Platform.Windows, make it more abstract (actual associations are defined somewhere in osu.Desktop or osu.Game.Tests projects, but WindowsAssociationManager knows how to install them).

This could allow us to make eg. .osz-test associations for testing, which is much cleaner.

bdach · 2024-02-05T12:12:27Z

I'm not sure visual tests for this are useful at this stage. If this is gonna hook into squirrel flows the only actual way to test this is via osu-deploy.

I'm kinda uncomfortable with the notion of "visual tests" touching system registry to begin with.

These icons should appear in end-user installation folder.

Can be tested with ``` dotnet publish -f net6.0 -r win-x64 osu.Desktop -o publish -c Debug publish\osu! ```

osu.Desktop/Windows/Win32Icon.cs

osu.Desktop/Windows/Icons.cs

bdach · 2024-02-07T11:17:50Z

osu.Desktop/Windows/WindowsAssociationManager.cs

+        protected override void LoadComplete()
+        {
+            base.LoadComplete();
+            localisationParameters.ValueChanged += _ => updateDescriptions();


It is a bit veiled, but what this ends up implying is that every single change of user language, or even the "show metadata in original language" toggle in settings, is going to cause writes to the windows registry. I am not comfortable with this and do not think that is sane.

I think there are two paths I'd rather take here:

not bother with localising descriptions at all

localise them, but on a one-shot basis, at point of installation.

localise them, but on a one-shot basis, at point of installation.

This relies too heavily on the automatic language selection being correct. How about updating the localised description once the user selects and confirms their language in the first-run setup wizard?

This relies too heavily on the automatic language selection being correct

What do you mean by "too heavily"? I'm not sure I've known that to fail that much. If there's one place where that should work perfectly it's windows.

How about updating the localised description once the user selects and confirms their language in the first-run setup wizard?

Eeeeehhhh I don't know. I'd rather not touch the registry outside of squirrel flows / manual intervention via settings button.

@ppy/team-client I'm happy with the code and this PR is ready for review. The above conversation and stable in 'open with' remain the only open questions.

osu.Desktop/OsuGameDesktop.cs

bdach · 2024-02-07T11:26:07Z

associations clash with getStableInstallPathFromRegistry()

Can you elaborate on what this means?

Susko3 · 2024-02-07T13:45:58Z

associations clash with getStableInstallPathFromRegistry()

Can you elaborate on what this means?

getStableInstallPathFromRegistry() queries the current osu:// URI handler and checks if it is stable. Since lazer will overwrite that handler (for thr current user), the stable install path information will be lost.

File associations for lazer and stable are installed side-by-side, so I'll look into using those for detecting the stable install.

Usages/localisation logic TBD

…den place

Don't ask me why this uses the folder for .NET Framework 4.5

Susko3 · 2024-02-07T23:51:00Z

I've just checked how this behaves with stable installed. I used a clean slate test PC which hasn't ever had stable or lazer installed.

stable registers .osz2, is this a file format lazer can handle? (a quick search yields that it doesn't)
stable uses a lowercase osu! beatmap as the file description
stable calls every file osu! beatmap
there is no 'open with' for protocols, so osu:// is always handled by lazer (HKCU overrides HKLM)
- uninstalling lazer will restore stable as the protocol handler
we can use the osu! entry for getStableInstallPathFromRegistry(), as that is used exclusively by stable

As it currently stands, lazer file associations override stable's, with only lazer appearing in the open with menu:

Luckily, it's a simple fix and can be done either from stable or lazer. The following registry keys need to be added (both HKLM and HKCU works!):

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osk\OpenWithProgIds]
"osu!"=""

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osr\OpenWithProgIds]
"osu!"=""

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osz\OpenWithProgIds]
"osu!"=""

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osz2\OpenWithProgIds]
"osu!"=""

Registry entries installed by stable (reference)

Windows Registry Editor Version 5.00

; files

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osk]
@="osu!"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osr]
@="osu!"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osz]
@="osu!"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.osz2]
@="osu!"

; protocol

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\osu]
@="osu! url handler"
"URL Protocol"=""

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\osu\DefaultIcon]
@="\"C:\\Users\\User\\AppData\\Local\\osu!\\osu!.exe\",1"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\osu\shell\open\command]
@="\"C:\\Users\\User\\AppData\\Local\\osu!\\osu!.exe\" \"%1\""

; file handler (only 1)

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\osu!]
@="osu! beatmap"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\osu!\DefaultIcon]
@="\"C:\\Users\\User\\AppData\\Local\\osu!\\osu!.exe\",1"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\osu!\shell\open\command]
@="\"C:\\Users\\User\\AppData\\Local\\osu!\\osu!.exe\" \"%1\""

`osu` is the `osu://` protocol handler, which gets overriden by lazer. Instead, use `osu!` which is the stable file handler.

bdach · 2024-02-13T08:35:07Z

osu.Desktop/Windows/WindowsAssociationManager.cs

+        /// </summary>
+        public const string SHELL_OPEN_COMMAND = @"Shell\Open\Command";
+
+        public static readonly string EXE_PATH = Path.ChangeExtension(typeof(WindowsAssociationManager).Assembly.Location, ".exe").Replace('/', '\\');


What's with the replace at the end? This already appears to be correct on my machine...

> typeof(Program).Assembly.Location "D:\\Open Source\\osu\\osu.Desktop\\bin\\Debug\\net8.0\\osu!.dll" > Path.ChangeExtension(typeof(Program).Assembly.Location, ".exe") "D:\\Open Source\\osu\\osu.Desktop\\bin\\Debug\\net8.0\\osu!.exe"

When running static methods from Rider (for testing), it would have / as a path separator.

I dunno that's weird. I'd like to think windows should be able to handle either?

Windows doesn't handle / and errors out.

Interesting to note is that the icons work fine because Path.GetDirectoryName will normalise path separators to \.

osu.Desktop/Windows/WindowsAssociationManager.cs

peppy · 2024-02-26T09:30:30Z

Hope we can get this in the next release.

Don't see any reason for them to be public.

bdach · 2024-02-27T13:41:06Z

I've tested this and it seems to be behaving pretty well, and getting along with stable pretty amicably even. Did just one small cleanup commit (87509fb), looks good to go otherwise.

We may need some improvements to client itself (e.g. bring the window to front after something has been imported via shell associations or opened via url protocols) but that can come later.

The localisation stuff basically cannot be tested right now but I'd be okay with it just using hardcoded english really.

peppy

Works well for me!

Susko3 added 3 commits February 3, 2024 16:59

Add way to associate with files and URIs on Windows

498d93b

Associate on startup

0357882

Only associate on a deployed build

2bac09e

Helps to reduce clutter when developing

pull-request-size bot added the size/L label Feb 3, 2024

Susko3 added the platform/windows label Feb 3, 2024

Uninstall associations when uninstalling from squirrel

cdcf5bd

Use cleaner way to specify .exe path

2f42112

Susko3 added 5 commits February 5, 2024 13:44

Move WindowsAssociationManager to osu.Desktop

01efd1b

Copy .ico files when publishing

7789cc0

These icons should appear in end-user installation folder.

Inline constants

4ec9d26

Remove tests

0168ade

Can be tested with ``` dotnet publish -f net6.0 -r win-x64 osu.Desktop -o publish -c Debug publish\osu! ```

Change to class to satisfy CFS hopefully

17033e0

smallketchup82 mentioned this pull request Feb 5, 2024

Associate with files & urls on Windows #26658

Closed

10 tasks

bdach reviewed Feb 7, 2024

View reviewed changes

Susko3 added 12 commits February 7, 2024 21:33

Remove Win32Icon class and use plain strings instead

57d5717

Make WindowsAssociationManager static

eeba937

Usages/localisation logic TBD

Install associations as part of initial squirrel install

f9d257b

Remove duplicate try-catch and move NotifyShellUpdate() to less hid…

0563507

…den place

Move update/install logic into helper

6bdb076

Refactor public methods

738c287

Move public methods closer together

ffdefbc

Use Logger.Error

da8c454

Refactor ProgID logic so it's more visible

7f5dedc

Standardise using declaration

3419b8f

Standardise using declaration

139072f

Make things testable via 'Run static method' in Rider

bf47221

Susko3 added 3 commits February 7, 2024 22:45

Remove unused param

8049270

Merge branch 'master' into windows-file-uri-association

7864810

Copy icons to nuget and install package

dfa0c51

Don't ask me why this uses the folder for .NET Framework 4.5

Susko3 added 2 commits February 8, 2024 00:54

Fix stable install path lookup

1dc54d6

`osu` is the `osu://` protocol handler, which gets overriden by lazer. Instead, use `osu!` which is the stable file handler.

Make NotifyShellUpdate() public to ease testing

6ded79c

Joehuu changed the title ~~Associate with files and URIs on windows V2~~ Associate with files and URIs on windows Feb 8, 2024

bdach reviewed Feb 13, 2024

View reviewed changes

peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Feb 26, 2024

Susko3 and others added 4 commits February 26, 2024 12:27

Refine uninstall logic to account for legacy windows features

6dbba70

Inline EXE_PATH usage

f280747

Inline HKCU\Software\Classes usage

9b3ec64

Privatise registry-related constants

87509fb

Don't see any reason for them to be public.

bdach previously approved these changes Feb 27, 2024

View reviewed changes

bdach requested a review from peppy February 27, 2024 13:41

Fix typos in xmldoc

61cc5d6

peppy dismissed bdach’s stale review via 61cc5d6 March 1, 2024 03:24

peppy approved these changes Mar 1, 2024

View reviewed changes

peppy merged commit 91483bd into ppy:master Mar 1, 2024
8 of 11 checks passed

Susko3 mentioned this pull request Mar 1, 2024

Fix file association descriptions not being written when updating from an older version #27449

Merged

smallketchup82 mentioned this pull request Sep 9, 2024

Add beatmap icon to windows beatmap files #29812

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Associate with files and URIs on windows #27001

Associate with files and URIs on windows #27001

Susko3 commented Feb 3, 2024 •

edited

Loading

Susko3 commented Feb 5, 2024

bdach commented Feb 5, 2024

bdach Feb 7, 2024

Susko3 Feb 7, 2024

bdach Feb 7, 2024 •

edited

Loading

Susko3 Feb 8, 2024 •

edited

Loading

bdach commented Feb 7, 2024

Susko3 commented Feb 7, 2024

Susko3 commented Feb 7, 2024 •

edited

Loading

bdach Feb 13, 2024

Susko3 Feb 26, 2024

bdach Feb 26, 2024

Susko3 Feb 26, 2024

peppy commented Feb 26, 2024

bdach commented Feb 27, 2024 •

edited

Loading

peppy left a comment

Associate with files and URIs on windows #27001

Associate with files and URIs on windows #27001

Conversation

Susko3 commented Feb 3, 2024 • edited Loading

Fixed problems from the previous PR

Problems with this PR

Testing

Susko3 commented Feb 5, 2024

bdach commented Feb 5, 2024

bdach Feb 7, 2024

Choose a reason for hiding this comment

Susko3 Feb 7, 2024

Choose a reason for hiding this comment

bdach Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Susko3 Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

bdach commented Feb 7, 2024

Susko3 commented Feb 7, 2024

Susko3 commented Feb 7, 2024 • edited Loading

bdach Feb 13, 2024

Choose a reason for hiding this comment

Susko3 Feb 26, 2024

Choose a reason for hiding this comment

bdach Feb 26, 2024

Choose a reason for hiding this comment

Susko3 Feb 26, 2024

Choose a reason for hiding this comment

peppy commented Feb 26, 2024

bdach commented Feb 27, 2024 • edited Loading

peppy left a comment

Choose a reason for hiding this comment

Susko3 commented Feb 3, 2024 •

edited

Loading

bdach Feb 7, 2024 •

edited

Loading

Susko3 Feb 8, 2024 •

edited

Loading

Susko3 commented Feb 7, 2024 •

edited

Loading

bdach commented Feb 27, 2024 •

edited

Loading