Apply NRT to all skin editor classes #22430
Conversation
| @@ -342,7 +344,7 @@ public void Save() | |||
| currentSkin.Value.UpdateDrawableTarget(t); | |||
|
|
|||
| skins.Save(skins.CurrentSkin.Value); | |||
| onScreenDisplay?.Display(new SkinEditorToast(ToastStrings.SkinSaved, currentSkin.Value.SkinInfo.ToString())); | |||
| onScreenDisplay?.Display(new SkinEditorToast(ToastStrings.SkinSaved, currentSkin.Value.SkinInfo.ToString() ?? "Unknown")); | |||
There was a problem hiding this comment.
Not sure I would have bothered to fall back with this one. It's only an issue because this is object.ToString() which is allowed to return string?. In practice a Live<SkinInfo> should never return null from ToString().
There was a problem hiding this comment.
What would you prefer in this scenario? .AsNonNull()? Seems like one which is going to come up quite regularly..
There was a problem hiding this comment.
Probably .AsNonNull() yeah. Not that it matters much.
| [Resolved] | ||
| private OsuGame game { get; set; } = null!; |
There was a problem hiding this comment.
Was this dependency on OsuGame intentionally changed from optional to required here?
There was a problem hiding this comment.
Yeah, there were already usages where it wasn't null-checked (see Update()) and it does seem required for this overlay to work as expected.
Of note, this is SkinEditorOvleray, the component which creates a SkinEditor for the current screen.
osu.Game/Skinning/SkinnableSound.cs
Outdated
| @@ -79,7 +75,7 @@ public SkinnableSound([NotNull] ISampleInfo sample) | |||
| /// <summary> | |||
| /// The samples that should be played. | |||
| /// </summary> | |||
| public ISampleInfo[] Samples | |||
| public ISampleInfo[]? Samples | |||
There was a problem hiding this comment.
This is a bit awful... The property can accept null to unset the samples (and this is used in places), but will never return null. Which is tripping a code quality inspection as well (albeit in a test scene).
Don't necessarily have any good ideas what to do here. Simplest thing would be to just make the test shut up. But it'd be nice to actually have this not nullable - maybe there could be a ClearSamples() convenience method or something?
There was a problem hiding this comment.
Yeah I was worried about this, but didn't want to expand the scope of this PR further. I'll see if the non-nullable route requires minimal changes.
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
|
Going to get this one in, as it's part of a chain of PRs I have that are co-dependent. |
Doing some clean-up passes before adding more to the skin editor because the... skinning namespace is a bit shocking.