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

Apply NRT to all files where it can be easily applied #5867

Merged
merged 9 commits into from
Jun 24, 2023
Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 23, 2023

Something I've had sitting around. Tested against osu! (see ppy/osu#24019).

Another pass will follow, but feel that getting this one out of the way is a good first step.

  • 75ed421 is an automated pass. reviewing it separately is recommended as it should be a breeze
  • 3064650 are manual fixes after the automated pass to fix non-compile-time warnings (aka r#)
  • d002fda is stuff rider didn't pick up on (CI script unfortunately crashes locally for me)
  • BOM in files is inconsistent in our repository. this arbitrarily changes BOM. please don't focus on this. we should normalise this at a later point.

@bdach bdach self-requested a review June 23, 2023 17:00
osu.Framework/IO/Stores/GlyphStore.cs Show resolved Hide resolved
osu.Framework/Input/Events/FocusLostEvent.cs Show resolved Hide resolved
osu.Framework/Testing/DrawFrameRecordingContainer.cs Outdated Show resolved Hide resolved
@@ -14,7 +12,7 @@ public static class TestingExtensions
/// <summary>
/// Find all children recursively of a specific type. As this is expensive and dangerous, it should only be used for testing purposes.
/// </summary>
public static IEnumerable<T> ChildrenOfType<T>(this Drawable drawable)
public static IEnumerable<T> ChildrenOfType<T>(this Drawable? drawable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's with this one? Is anyone really calling ChildrenOfType() on a null reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be okay. But also may have been a weird osu! test usage (I likely would not have made this change without a good reason). Will need to run tests against this before applying.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't recall from memory then I'm not sure I care that much for you to go check. I was mostly curious is all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out it's to ease cases like this:

2023-06-24 09 43 52@2x

There's around 30 usages osu! side which would need updating if we revert this. I think making the method nullable makes it a touch more flexible though, given its intended purpose.

@bdach
Copy link
Collaborator

bdach commented Jun 23, 2023

Also there are some new build warnings in the iOS project.

@peppy
Copy link
Member Author

peppy commented Jun 23, 2023

Also there are some new build warnings in the iOS project.

Ah right. Mobile platforms were a TODO on these.

peppy and others added 4 commits June 24, 2023 02:51
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Without other bindable classes following suit, this will only lead to
headaches. Noticed in an osu!-side usage.
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Seems good. Going to expedite merge now to minimise chances of merge conflictery here. If it turns out something goes bad we can revert in a separate pull.

@bdach bdach enabled auto-merge June 24, 2023 09:53
@bdach bdach merged commit 9746d7d into ppy:master Jun 24, 2023
@peppy peppy deleted the mass-nrt branch July 3, 2023 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants