Clean up windows association manager code #31450
Conversation
Refactor `WindowsAssociationManager` to be usable
| @@ -189,14 +190,16 @@ public void Install() | |||
| // register a program id for the given extension | |||
| using (var programKey = classes.CreateSubKey(programId)) | |||
| { | |||
| programKey.SetValue(null, description); | |||
There was a problem hiding this comment.
Default that was previously written in UpdateDescription(null) is now written here.
There was a problem hiding this comment.
The fact that this is passing LocalisableString to SetValue() directly is unsettling me. Looking at dotnet/runtime source it should do what we'd want here but the API is very magic-typed so I'd prefer an explicit .ToString() here.
There was a problem hiding this comment.
Triggers an inspection unfortunately :(
Would this work for you?
diff --git a/osu.Desktop/Windows/WindowsAssociationManager.cs b/osu.Desktop/Windows/WindowsAssociationManager.cs
index 98e77b1ff6..d30fd7878b 100644
--- a/osu.Desktop/Windows/WindowsAssociationManager.cs
+++ b/osu.Desktop/Windows/WindowsAssociationManager.cs
@@ -217,7 +217,10 @@ public void LocaliseDescription(LocalisationManager localisationManager)
if (classes == null) return;
using (var programKey = classes.OpenSubKey(programId, true))
- programKey?.SetValue(null, localisationManager.GetLocalisedString(description));
+ {
+ string localisedString = localisationManager.GetLocalisedString(description);
+ programKey?.SetValue(null, localisedString);
+ }
}
/// <summary>
There was a problem hiding this comment.
Uhhh that's the wrong call that you're trying to do this on no? Your patch seems to be touching line 220, this is line 193.
| { | ||
| protocolKey.SetValue(URL_PROTOCOL, string.Empty); | ||
| protocolKey.SetValue(null, $@"URL:{description}"); |
There was a problem hiding this comment.
Default that was previously written in UpdateDescription(null) is now written here.
There was a problem hiding this comment.
What's with the URL: prefix in the string? That looks new?
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh that is hidden away, I completely failed to spot it...... Sure yeah.
I'm supposed to be working on this logic to add support to stable, but I couldn't work with this class so I spent 10 minutes fixing it up.
recordsfor anything that isn't a barebones modelnullcalls to setup defaults. The localisation flow is now completely isolated and not required to be called until we want to support it.