osu#2 - #81
Closed
winnerspiros wants to merge 77 commits into
Closed
Conversation
Grouped notifications for more than 1 person were added in #36180 but it looks like they forgot to add the Transient and IsImportant flags, which means the grouped notifications would still stay in the notification list/flash the taskbar. Before: https://github.com/user-attachments/assets/8a34bbc0-2b5c-4086-b2ee-1daa6d1e6e10 After: https://github.com/user-attachments/assets/03c25ba6-7c8e-464c-bbb1-688ab9da6bb6
…6616) - Closes #35389 Same as: https://github.com/ppy/osu/blob/2efe0c95e63817f312f5fb12cc60dd56bee0023b/osu.Game/Screens/Edit/Editor.cs#L1173-L1180 There's also seeking hit objects and sample points, but the seeks are relatively close to each other and probably useless when playing(?). If we want to make those cases not stuck at the same point in time, I believe the leniency should be lower than 1000 ms. With the above, that is why I just copy-pasted the code, as we may want to have different leniencies. Edit: forgot the automated label thing, will not label next time
## [Specify `Accept` header in registration request](28edb78) The lack of it meant that in specific scenarios web would respond with a chunk of HTML instead of JSON. ## [Allow showing registration error message even if no redirect is given](6ad4994) There are scenarios where this can happen, and if it did, previously the strict requirement to have both would cause the specific message to be discarded and replaced with the generic "something happened" one.
…d beatmap is online (#36632) - Closes #36584 The last two commits could be either fixes to the issue above, but in a code quality perspective, the scheduler in `setLink()` seems unnecessary as the other set methods don't have it (other than making it run last) and the other commit is self explanatory.
- closes #36016 Co-authored-by: Dean Herbert <pe@ppy.sh>
Adds a `DamageInfo` property to `RankedPlayDamageInfo` to be used by the result screen. The issue this is trying to solve is that once the result screen initializes, the HP value of each player has already been updated in the room state so the previous values are no longer accessible. Doing this without the state exposing it would require some kinda setup to keep the previous MatchState's HP values around on the client which would introduce a lot of unnecessary weirdness.
Update production endpoints in an attempt to fix Russian player connections
… all results used
Refactor hit result methods on `Ruleset`
Part of #32584. Very much inspired by the respective component for displaying profile pictures on the user overlay * allow disabling interactivity/tooltips * add option to show placeholder on null team instead of hiding component entirely * move setting corner radius out to respective parent components to allow for easier overriding
- Related to ppy/osu-server-spectator#406 Adding this field to this model has several vague reasons that I can't fully formulate yet, but I can't really see myself going forward *without* this. - People were very excited about having referees displayed on the room participants' list, and so adding the referees as real `MultiplayerRoomUser`s helps this. Having the role could even be used client-side to show a special icon or other status on the participants list. (Which isn't done yet, could be as an aesthetic follow-up after the basics are in place.) - Server-side, having this field is convenient for things like permission checks or just plain logic, as with two hubs you just need to do different *stuff* on a `MultiplayerRoomUser`.
Fixes incorrect glow corner radius around user tag buttons Before: <img width="267" height="123" alt="image" src="https://github.com/user-attachments/assets/3e602e84-bb13-46f7-942c-85ddf3954946" /> After: <img width="227" height="102" alt="image" src="https://github.com/user-attachments/assets/4dca2e3d-80e2-4b6c-988e-d14f371cfbe8" />
…ps (#36663) Because people get confused by how this works. Shows on results screen where the post-play statistics updates go. https://github.com/user-attachments/assets/ef3a91d1-86dd-4029-8f0f-bdf0b727ca6c
Closes #33395 Copies the bookmarks from `referenceWorkingBeatmap` while creating a new difficulty from scratch. I adapted the tests in `TestSceneEditorBeatmapCreation` to include the bookmark checks. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
Resolves #36288. If the current selection is still available after leaving scoped mode, it's left as is. If it's not, the selection from before entering scoped mode is restored. https://github.com/user-attachments/assets/b1ac3de1-7c7f-4949-82a9-1dd0459f3f61 --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
…lected object is changed (#36681) Before: https://github.com/user-attachments/assets/d0a0373d-2d46-48a9-9ea5-bac82a612f32 After: https://github.com/user-attachments/assets/f5785b54-d7fc-4ce0-86b0-60c96ff22bc3 --- Closes #36677. Kinda shocking this went by unnoticed for this long.
…o stable (#36702) - Closes #30293 - Fixes https://osu.ppy.sh/community/forums/topics/2179339?n=1 Aside from fixing the off-by-one error that I mentioned in #30293 (comment), this also: - Brings back the behaviour wherein if timing points are arranged very weird and nightcore would play e.g. two first beats in a timing point back-to-back, the second timing point is silent. - Brings back the behaviour wherein the finish sample only plays if `OmitFirstBarLine` on the timing point is disabled. However: - This does not bring back the behaviour wherein hat samples only play if the slider tick rate is even because that only kind of makes sense in common time, and if common time is mixed with waltz time or other time signatures, it just gets weird. - Also stable has zero attempt for compensating for waltz time anyway, lazer's behaviour is bespoke, so that is not going to match any way you cut it. My testing procedure essentially consisted of getting stable to log when it was playing nightcore samples and cross-checking the first 30sec or so of https://osu.ppy.sh/beatmapsets/534385#osu/1131956 (check out the timing of that beatmap, for something ranked it is DEEPLY messed up). I guess I can add test cases if deemed required but I already wasted much more time than I would have liked here...
…36729) - Fixes #36727 Copies/sets the non-maximised edge effect initially: https://github.com/ppy/osu/blob/c144cf188a37ebc965b25818938527c02013efc4/osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/PlayerGrid.Cell.cs#L83-L88
Fix transient user stats animation changing speed after first display
Various song select class cleanup
It's still broken: https://github.com/ppy/osu/actions/runs/22310482792/job/64540909415 and I'm not sure what else it could be other than this, so I'm giving it one more honest try before I throw up hands. For better or worse, https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions explicitly lists that the `pull-requests` permission allows tagging PRs, so I'm holding out hope that this is it. Still terrible marks for documentation here, both to the github action, as well as everything to do with `GITHUB_TOKEN`.
Previously we were always making header content in the base class then overwriting it, which felt ick.
`AlwaysPresent` is a code smell here. Rather than doing this, let's just using masking as we usually do. Closes #36748.
Uses `HotkeyDisplay` for toolbar button tooltips rather than `SpriteText` <img width="433" height="160" alt="image" src="https://github.com/user-attachments/assets/d74c4dd2-27fd-4e7c-881e-3c6152982dd6" /> --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
Fix input settings being interactive even when collapsed
…ition bank if set to auto (#36741) Closes #36703. It was only broken via keyboard hotkeys and not via the sample popover because the sample popover has a separate copy of the logic that didn't have the bug. Compare: https://github.com/ppy/osu/blob/13aeed15f95fa6585ff01eedaccc43b28ff656cb/osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs#L473-L475 I considered splitting a helper to have one copy of the logic but it's not very simple to do compared to a two-liner fix so I gave up.
- [x] Depends on #36741 for merge conflict avoidance RFC, cc @OliBomby ## [Adjust behaviour of automatic bank assignment during placement](547f55e) Diatribe time! This is fallout of the discussion about auto bank in #36705. Auto bank in lazer as written before this commit is confused. On stable, auto bank is closer to "no bank", as in "go look up the current sample timing point, get the bank of that, and use that". lazer has no timing points anymore, but people still want auto bank. So what do? Auto bank for normal samples is somewhat sane still. It only works during placement, and will just copy the normal bank of the previous object - if one exists. That said, one *might not* exist, but the resulting object will still have its normal sample created with `editorAutoBank: true`. That is largely cosmetic and without consequences, but this commit fixes that. Auto bank for *addition* samples, however... Hoo boy. - For placed objects, auto bank means "take the normal sample, read its bank, and use that". Simple enough, right? - Hoooooowever. During placement, auto bank before this commit used to mean "look at the *previous object*, check if it has an addition sound and then use its bank, if not use *the previous object's* normal sample and then use its bank" which is a completely different thing with its own implications. Like, say, what happens if the previous object uses the auto addition bank too? What should be copied over? Should it be the notion of "auto bank" in that the addition bank should match the normal bank, or should it be the literal bank that the previous object is using? This change attempts to define this unambiguously. "Auto additions bank" means "the same bank as the normal bank of this object", full stop. ## [Do not touch sample toggle state if there are no selected objects](052cde5) Fixes issue described in #36705 (comment) wherein opening a sample popover will disable addition bank toggles and toggle off all addition samples. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
`ScreenTestScene` already provides one.
Remove redundant footer from `TestScenePlaylistsRoomSubscreen`
Rebase of smoogipoo#193 Going forward, the client will have to know the type of pool being invited to so that it can enter the appropriate screen when clicking the notification. Unfortunately, SignalR does not support overloading methods, or even adding parameters to them, so this PR deprecates the `MatchmakingRoomInvited` event and adds its replacement `MatchmakingRoomInvitedWithParams` with a complex `invitation` parameter that we _can_ extend in the future if required (such as potentially adding the name of the pool). This also prepares the notification by extracting some code to a `Complete` method receiving said `invitation` parameter. This part of code will be further modified to enter the correct screen: https://github.com/smoogipoo/osu/blob/0a4018045b9d908f66c63dee65d0059d05b26e43/osu.Game/Screens/OnlinePlay/Matchmaking/Queue/QueueController.cs#L200 In particular, I have tested that new clients continue to work with the old server (dev.ppy.sh) in quick play | | Old Server | New Server | | ------------- |:-------------:| :-----:| | Old Client | :green_circle: | :green_circle: | | New Client | :green_circle: | :green_circle: |
Part of the screen footer refactor. Once footer content is being managed by `OsuScreen`, the current tests which simply create the tested overlay and `ScreenFooter` in a container will no longer work. This PR refactors them to use `ScreenTestScene` with the setup being creating a dedicated testing `OsuScreen` which does the bare minimum to create the tested overlay and necessary components (eg. `FooterButtonFreeModsV2` for `TestSceneFreeModsOverlay`). Most of the changes here can be described as `%s/<...>Overlay/screen.Overlay/g`, with some minor touchups as necessary, given that we're now testing a more complete flow which checks more things that were previously not handled by the tests. ## [Move footer to front in ScreenTestScene](f8740e0) Self-explanatory. Without it the footer would show below the actual overlay, breaking tests depending on manual input. For the sake of tests not breaking in CI, both #36718 and this have this included - would prefer the former to be merged first since it was already reviewed there. ## `TestSceneModSelectOverlay` There were a few tests (`TestColumnHidingOnIsValidChange`, `TestColumnHidingOnTextFilterChange`, and `TestHidingOverlayClearsTextSearch`) that would create a custom overlay instance instead of the globally provided one. I've tested both and the tests run fine with the default overlay, so they're now using that instead. ## `TestSceneFreeModSelectOverlay` Updated to use footer v2. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
reads a bit better when filename isn't in main text also added "click to view" text by analogy with `LogsExportFinished` | master | pr | |-|-| | <img width="336" height="114" alt="image" src="https://github.com/user-attachments/assets/2555390c-1299-43ae-9be5-cb8d091b3387" /> | <img width="336" height="108" alt="image" src="https://github.com/user-attachments/assets/a8f18d9f-fa11-4d8f-82af-c88b0f82576c" /> |
📝 WalkthroughWalkthroughThis pull request introduces significant architectural changes including promotion of protected methods to public in rulesets, comprehensive test infrastructure refactoring from component-based to screen-based approaches, removal of legacy V2 suffixed classes, sample bank automation updates, new team flag UI components, and multiple dependency version updates across the framework. Changes
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs (1)
55-61:⚠️ Potential issue | 🟠 MajorReplace debug-only invariant check with a runtime guard.
Line 55 uses
Debug.Assert, which is stripped in release builds. IfPerformFail()is called beforeApplyToPlayer(), this silent bypass leavesretriesuninitialized, causing incorrect behavior in production. Replace with a runtime check that fails fast.Suggested fix
public bool PerformFail() { - Debug.Assert(retries != null); + if (retries is null) + throw new InvalidOperationException($"{nameof(ModEasyWithExtraLives)} must be applied before fail handling."); if (retries == 0) return true; health.Value = health.MaxValue; retries--; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs` around lines 55 - 61, The Debug.Assert(retries != null) must be replaced with a runtime guard that throws when retries is uninitialized; in ModEasyWithExtraLives, update the beginning of PerformFail() to check if retries is null and throw an InvalidOperationException (or similar) with a clear message that ApplyToPlayer() must be called before performing fails, then proceed with the existing logic (if retries == 0 return true; reset health.Value, decrement retries). This ensures failures surface in release builds instead of silently misbehaving.osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs (1)
368-381:⚠️ Potential issue | 🟡 Minor
SetSampleBank()early-return predicate is narrower than the applied update.The method now mutates
HIT_NORMALandEditorAutoBanksamples, buthasRelevantBank()only validatesHIT_NORMAL. This can skip needed auto-bank sync work.💡 Suggested fix
bool hasRelevantBank(HitObject hitObject) { - bool result = hitObject.Samples.Where(o => o.Name == HitSampleInfo.HIT_NORMAL).All(s => s.Bank == bankName); + bool result = hitObject.Samples.Where(o => o.Name == HitSampleInfo.HIT_NORMAL || o.EditorAutoBank).All(s => s.Bank == bankName); if (hitObject is IHasRepeats hasRepeats) { foreach (var node in hasRepeats.NodeSamples) - result &= node.Where(o => o.Name == HitSampleInfo.HIT_NORMAL).All(s => s.Bank == bankName); + result &= node.Where(o => o.Name == HitSampleInfo.HIT_NORMAL || o.EditorAutoBank).All(s => s.Bank == bankName); } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs` around lines 368 - 381, The early-return uses hasRelevantBank but that predicate only checks HitSampleInfo.HIT_NORMAL while the update mutates samples with either Name == HitSampleInfo.HIT_NORMAL or EditorAutoBank == true; update the predicate used by SelectedItems.All(hasRelevantBank) (or the hasRelevantBank implementation) to treat a hit object as relevant if any of its Samples or NodeSamples satisfy (s.Name == HitSampleInfo.HIT_NORMAL || s.EditorAutoBank), so the early-return only fires when no selected object needs the new-bank assignment for either normal hits or editor-auto-bank samples.
🧹 Nitpick comments (22)
osu.Game/Online/Multiplayer/MultiplayerRoomUserRole.cs (1)
6-10: Consider assigning explicit integer values for serialization stability.This enum is used in
MultiplayerRoomUser, which is serialized via MessagePack. Without explicit values, future insertions or reordering of members could break backward compatibility with existing serialized data.♻️ Suggested improvement
public enum MultiplayerRoomUserRole { - Player, - Referee, + Player = 0, + Referee = 1, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Online/Multiplayer/MultiplayerRoomUserRole.cs` around lines 6 - 10, Assign explicit integer values to the MultiplayerRoomUserRole enum to ensure MessagePack serialization stability: update the MultiplayerRoomUserRole definition (used by MultiplayerRoomUser) so each member has a fixed integer (e.g., Player = 0, Referee = 1) to prevent future reordering/insertions from breaking compatibility.osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs (1)
189-189: Consider verifying actual bookmark values, not just count.The assertion only checks that there are 2 bookmarks but doesn't verify they contain the expected values (500 and 1000). While the count check provides basic coverage, verifying actual values would make the test more robust.
💡 Optional: More thorough assertion
- AddAssert("created difficulty has bookmarks", () => EditorBeatmap.Bookmarks.Count == 2); + AddAssert("created difficulty has bookmarks", () => EditorBeatmap.Bookmarks.SequenceEqual(new[] { 500, 1000 }));Note: Similar improvements could be applied to assertions at lines 259, 320, and 406.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs` at line 189, The test currently only checks the bookmark count; update the assertion that uses AddAssert("created difficulty has bookmarks", () => EditorBeatmap.Bookmarks.Count == 2) to verify the actual bookmark values (e.g., assert EditorBeatmap.Bookmarks.SequenceEqual(new[] {500, 1000}) or check both Contains(500) and Contains(1000) and order if relevant) so the test validates contents not just count; apply same pattern to the other similar assertions referenced (lines with AddAssert checks for bookmarks at the other test points).osu.Game.Rulesets.Osu.Tests/Mods/TestSceneOsuModEasy.cs (2)
51-54: Consider catching the specific expected exception type.The test expects
ApplyToPlayerto throwInvalidOperationExceptionon reapplication (perModEasyWithExtraLivesimplementation). Catching the specific exception type would make the test's intent clearer and avoid masking unexpected exceptions.♻️ Suggested narrower catch
- catch + catch (System.InvalidOperationException) { - // don't care if this fails. in fact a failure here is probably better than the alternative. + // Expected: ApplyToPlayer throws when applied to a player twice. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Rulesets.Osu.Tests/Mods/TestSceneOsuModEasy.cs` around lines 51 - 54, The empty catch should be narrowed to the expected exception type so unexpected errors aren't swallowed: change the broad catch in TestSceneOsuModEasy around the ApplyToPlayer call (testing ModEasyWithExtraLives reapplication) to catch InvalidOperationException specifically and handle it the same way (i.e., ignore or assert as intended), leaving other exceptions to surface.
41-41: Consider adding a null check forPlayerinPassCondition.
PassConditionmay be evaluated beforePlayeris initialized. While the test framework likely handles this, a defensive null check would make the test more robust and clarify intent.🛡️ Suggested defensive null check
PassCondition = () => { + if (Player == null) + return false; + if (((ModEasyTestPlayer)Player).FailuresSuppressed > 0 && !reapplied)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Rulesets.Osu.Tests/Mods/TestSceneOsuModEasy.cs` at line 41, PassCondition currently dereferences Player and casts it to ModEasyTestPlayer; add a defensive null check to ensure Player is not null before accessing ((ModEasyTestPlayer)Player).FailuresSuppressed and before using reapplied logic. Update the PassCondition implementation to first check Player != null (or use pattern matching like "Player is ModEasyTestPlayer pe && pe.FailuresSuppressed > 0") so the condition short-circuits when Player is null and avoids a NullReferenceException while preserving the existing behavior when a ModEasyTestPlayer is present.osu.Game/Overlays/Settings/Sections/InputSubsection.cs (2)
121-128: Consider usingVERTICAL_PADDINGfor consistent spacing.The
ToggleableHeaderuses a hardcodedVertical = 12margin while the baseCreateHeader()usesVERTICAL_PADDING(which equals 11.5f). This creates a minor visual inconsistency between toggleable and regular subsection headers.♻️ Suggested fix
headerText = new OsuSpriteText { Text = InputSettingsStrings.Device(text), Font = OsuFont.Style.Heading2, - Margin = new MarginPadding { Vertical = 12 }, + Margin = new MarginPadding { Vertical = VERTICAL_PADDING }, X = 18, Y = -1, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Settings/Sections/InputSubsection.cs` around lines 121 - 128, The ToggleableHeader creation uses a hardcoded MarginPadding Vertical = 12 which causes a visual mismatch with the base CreateHeader() that uses the VERTICAL_PADDING constant (11.5f); update the ToggleableHeader's headerText Margin to use VERTICAL_PADDING instead of 12 so both ToggleableHeader and CreateHeader() share the same vertical spacing (locate the headerText instantiation in InputSubsection.cs and replace the hardcoded Vertical value with VERTICAL_PADDING).
133-147: The bidirectional binding logic is complex but correct.The pattern works: the
Disabledtoggle on lines 141-143 prevents feedback loops whenCurrentupdatesswitchButton.Current. However, a simpler approach using a directBindTargeton load might reduce complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Settings/Sections/InputSubsection.cs` around lines 133 - 147, Replace the manual bidirectional wiring in LoadComplete with a direct bind target to simplify the logic: instead of subscribing to switchButton.Current.ValueChanged and using Current.BindValueChanged with temporary switchButton.Current.Disabled toggles, call Current.BindTarget(switchButton.Current) (or switchButton.Current.BindTarget(Current)) on load to connect the two bindables, and keep only a simple one-way update for the Disabled state controlled by the existing toggleable flag (e.g. set switchButton.Current.Disabled = !toggleable when toggleable changes). This eliminates the temporary disabling dance in LoadComplete and uses the built‑in BindTarget mechanism to avoid feedback loops while preserving the toggleable behavior.osu.Game.Tests/Visual/UserInterface/TestSceneLoadingLayer.cs (1)
138-138: MakePressableButtonprivate. This internal test class is only used withinTestSceneLoadingLayerand does not need public visibility. Making itprivate sealedreduces surface area with no downsides.♻️ Proposed refactor
- public partial class PressableButton : RoundedButton, IKeyBindingHandler<GlobalAction> + private sealed partial class PressableButton : RoundedButton, IKeyBindingHandler<GlobalAction>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/UserInterface/TestSceneLoadingLayer.cs` at line 138, The nested test class PressableButton is unnecessarily public; change its declaration inside TestSceneLoadingLayer from "public partial class PressableButton" to "private sealed partial class PressableButton" (or "private sealed class PressableButton" if partial isn't needed) to restrict visibility and seal it, keeping the class usage the same but reducing the surface area.osu.Game/Overlays/Toolbar/ToolbarButton.cs (1)
160-169: APIs verified. TheHotkeyDisplayandHotkeytypes exist with the expected signatures:
HotkeyDisplayis a public class with aHotkeyproperty of typeHotkeyHotkeyis a record struct with a constructor acceptingGlobalActionThe code
Hotkey = new Hotkey(Hotkey.Value)correctly passes the non-nullGlobalActionvalue to the constructor. While the naming does involveHotkeyappearing three times in different contexts (ToolbarButton property, HotkeyDisplay property, and struct type), this is standard C# scoping and is contextually clear in the object initializer. If readability is a concern, an intermediate local variable could improve clarity, but it is not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Toolbar/ToolbarButton.cs` around lines 160 - 169, The code is correct but the repeated identifier "Hotkey" (the ToolbarButton property, the HotkeyDisplay.Hotkey property, and the Hotkey record struct) is confusing; to clarify, capture the ToolbarButton's Nullable<GlobalAction> into a local (e.g. var globalAction = Hotkey.Value) inside the if (Hotkey != null) block and then set HotkeyDisplay.Hotkey using that local (Hotkey = new Hotkey(globalAction)), leaving all other behavior unchanged and keeping references to HotkeyDisplay and the Hotkey record type.osu.Game/Screens/OnlinePlay/Matchmaking/Queue/QueueController.cs (1)
193-194:invitationparameter is currently unused inComplete(...).Either use
invitation.Typefor pool-specific UX or explicitly discard it to signal intent.♻️ Minimal clarity tweak
- public void Complete(MatchmakingRoomInvitationParams invitation) + public void Complete(MatchmakingRoomInvitationParams _)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Matchmaking/Queue/QueueController.cs` around lines 193 - 194, The Complete(MatchmakingRoomInvitationParams invitation) method currently ignores its invitation parameter; either consume invitation.Type where room-pool-specific behavior or explicitly discard the parameter to document intent. Update the Complete method (in QueueController.cs) to reference invitation.Type in the UX/path-selection logic if pool-specific handling is required, otherwise add an explicit discard (or comment) for the invitation parameter so static analysis and readers know the unused parameter is intentional.osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderReversal.cs (1)
293-298: Assertion names don't match the indices used.The indices and assertion messages appear swapped:
ControlPoints[^2]is the second-to-last point (in the last arc), but the message says "First arc's control"ControlPoints[1]is the second point (in the first arc), but the message says "Last arc's control"This is confusing for future maintainers. Consider swapping the assertion names to match the actual indices being checked.
Proposed fix: Swap assertion names
-AddAssert("First arc's control is not at the slider's middle", () => +AddAssert("Last arc's control is not at the slider's middle", () => Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, selectedSlider.Path.PositionAt(0.5)) > 1 ); -AddAssert("Last arc's control is not at the slider's middle", () => +AddAssert("First arc's control is not at the slider's middle", () => Vector2.Distance(selectedSlider.Path.ControlPoints[1].Position, selectedSlider.Path.PositionAt(0.5)) > 1 );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderReversal.cs` around lines 293 - 298, The two AddAssert calls have swapped assertion messages vs the control-point indices; update the descriptive strings so they match the indices being checked — when asserting Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, selectedSlider.Path.PositionAt(0.5)) use a message stating "Last arc's control is not at the slider's middle", and when asserting Vector2.Distance(selectedSlider.Path.ControlPoints[1].Position, selectedSlider.Path.PositionAt(0.5)) use "First arc's control is not at the slider's middle" so the messages correspond to the actual ControlPoints indices in TestSceneSliderReversal.osu.Game.Tests/Visual/Multiplayer/TestSceneFreeModSelectOverlay.cs (2)
81-85: ScopeSelectAllModsButtonlookup to the overlay container.Using
this.ChildrenOfType<SelectAllModsButton>().Single()is brittle in the new screen model because both footer and overlay can host similarly-typed controls. Prefer querying fromfreeModSelectOverlay(or another precise parent) to avoid ambiguousSingle()matches.Suggested change
-InputManager.MoveMouseTo(this.ChildrenOfType<SelectAllModsButton>().Single()); +InputManager.MoveMouseTo(freeModSelectOverlay.ChildrenOfType<SelectAllModsButton>().Single());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/Multiplayer/TestSceneFreeModSelectOverlay.cs` around lines 81 - 85, The test currently locates the SelectAllModsButton globally via this.ChildrenOfType<SelectAllModsButton>().Single(), which can return ambiguous matches; change the lookup to scope to the overlay container (e.g. use freeModSelectOverlay.ChildrenOfType<SelectAllModsButton>().Single() or otherwise query from freeModSelectOverlay) in the AddStep that moves and clicks the button so the test targets the overlay's button explicitly (keep the existing InputManager.MoveMouseTo and InputManager.Click usage but obtain the target from freeModSelectOverlay instead of this).
242-246: Dispose overlay registration before callingbase.Dispose().Disposing registration first gives deterministic teardown ordering for overlay lifecycle and avoids expiring overlay state after parent disposal.
Suggested change
protected override void Dispose(bool isDisposing) { - base.Dispose(isDisposing); overlayRegistration?.Dispose(); + base.Dispose(isDisposing); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/Multiplayer/TestSceneFreeModSelectOverlay.cs` around lines 242 - 246, Move the overlay registration disposal to occur before the base class teardown: in the override Dispose(bool isDisposing) of TestSceneFreeModSelectOverlay, call overlayRegistration?.Dispose() before calling base.Dispose(isDisposing) so the overlay registration is torn down deterministically prior to parent disposal and avoids expired overlay state after the base class disposes.osu.Game/Overlays/Settings/Sections/Input/KeyBindingRow.KeyButton.cs (1)
145-156: Logic is sound; the edge case of modifier-only triggers is unlikely but theoretically possible.The conditional virtual-key mapping for global vs gameplay bindings is correctly implemented. However, the method's documentation states "only allowing a single non-modifier key to be specified", yet the code doesn't enforce this. If
triggerKeyis a modifier key (e.g., a user binds only Shift), it could appear twice in thekeysarray: once from filteringfullState.Keysand once from appending. Whether this causes issues depends onKeyCombination's internal handling, which cannot be verified without the framework code.In normal usage, this is unlikely to occur, as users typically bind non-modifier keys. If framework conventions include deduplication, no action is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Settings/Sections/Input/KeyBindingRow.KeyButton.cs` around lines 145 - 156, The current assembly of the keys array can produce duplicate modifier entries or multiple non-modifier keys (contradicting the method doc); modify the logic around fullState.Keys, triggerKey and combination creation so you (1) deduplicate identical keys (e.g. via a distinct step) to prevent a modifier appearing twice, and (2) enforce "only a single non-modifier key" by ensuring at most one non-modifier remains (if multiple non-modifiers are present, drop all but the intended triggerKey or the last non-modifier) before calling UpdateKeyCombination(new KeyCombination(...)); operate on the variables shown (fullState.Keys, triggerKey, keys, KeyBinding.Value.RulesetName, GetVirtualKey, KeyCombination) so the global-vs-gameplay virtual-key mapping logic is preserved.osu.Game/Tests/Visual/ScreenTestScene.cs (1)
56-56: Resolve the TODO before merge (or track it explicitly).Line [56] leaves uncertainty in a core test-scene composition path. Please either remove the TODO with a decision, or convert it into a tracked issue.
If you want, I can draft the follow-up issue text with acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Tests/Visual/ScreenTestScene.cs` at line 56, Remove or resolve the stray TODO comment inside the ScreenTestScene composition path: either delete the line or replace it with a reference to a tracked issue ID and brief rationale; update the ScreenTestScene class (where the TODO sits) to reflect the decision (deleted comment if resolved or a comment like "See ISSUE-1234: reason" if tracked) so the ambiguity is removed before merging.osu.Game/Online/API/Requests/Responses/APITeam.cs (1)
21-21: Minor observation: Nullable type with non-null default.Making
FlagUrlnullable is appropriate since the API may not always provide a flag URL. The non-null default (string.Empty) provides a safe fallback, though it's slightly unconventional for nullable types. This approach works but consider ifnullwould be a more semantically correct default for a nullable property.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Online/API/Requests/Responses/APITeam.cs` at line 21, The FlagUrl field is declared nullable but initialized to an empty string; change this to be semantically consistent by either making FlagUrl non-nullable (remove the '?') and keep the empty-string default, or keep it nullable and set the default to null; update the APITeam class's FlagUrl declaration accordingly (refer to the FlagUrl field in APITeam) so the type and default value match the intended semantics.osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs (3)
575-589: Consider usingSingleOrDefaultwith explicit null check for robustness.The
Single(d => d.Enabled.Value)call will throwInvalidOperationExceptionif zero or multiple spread displays are enabled, which could lead to confusing test failures. Consider usingSingleOrDefaultwith an explicit assertion for clearer diagnostics.🛡️ Proposed defensive approach
private void scopeBeatmap(bool grouped) { if (grouped) { AddUntilStep("wait for spread indicator", () => this.ChildrenOfType<PanelBeatmapSet.SpreadDisplay>().Any(d => d.Enabled.Value)); - AddStep("click spread indicator", () => this.ChildrenOfType<PanelBeatmapSet.SpreadDisplay>().Single(d => d.Enabled.Value).TriggerClick()); + AddStep("click spread indicator", () => + { + var display = this.ChildrenOfType<PanelBeatmapSet.SpreadDisplay>().SingleOrDefault(d => d.Enabled.Value); + Assert.That(display, Is.Not.Null, "Expected exactly one enabled spread display"); + display!.TriggerClick(); + }); } else { AddUntilStep("wait for spread indicator", () => this.ChildrenOfType<PanelBeatmapStandalone.SpreadDisplay>().Any(d => d.Enabled.Value)); - AddStep("click spread indicator", () => this.ChildrenOfType<PanelBeatmapStandalone.SpreadDisplay>().Single(d => d.Enabled.Value).TriggerClick()); + AddStep("click spread indicator", () => + { + var display = this.ChildrenOfType<PanelBeatmapStandalone.SpreadDisplay>().SingleOrDefault(d => d.Enabled.Value); + Assert.That(display, Is.Not.Null, "Expected exactly one enabled spread display"); + display!.TriggerClick(); + }); } WaitForFiltering(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs` around lines 575 - 589, In scopeBeatmap, avoid calling Single(...) on the enabled SpreadDisplay collections; replace the Single( d => d.Enabled.Value ) usages for both PanelBeatmapSet.SpreadDisplay and PanelBeatmapStandalone.SpreadDisplay with SingleOrDefault(...) and then add an explicit assertion/guard that the result is not null (and optionally that no duplicate enabled items exist) before calling TriggerClick(), so test failures report a clear assertion instead of an InvalidOperationException.
591-591: HelperfindBeatmapcould benefit from defensive handling.
First()will throw if no beatmap matches the substring, which could result in unclear test failures. Consider adding a fallback or clearer error message.🛡️ Proposed defensive approach
- private BeatmapInfo findBeatmap(string difficultySubstring) => Beatmap.Value.BeatmapSetInfo.Beatmaps.First(b => b.DifficultyName.Contains(difficultySubstring)); + private BeatmapInfo findBeatmap(string difficultySubstring) + { + var beatmap = Beatmap.Value.BeatmapSetInfo.Beatmaps.FirstOrDefault(b => b.DifficultyName.Contains(difficultySubstring)); + Assert.That(beatmap, Is.Not.Null, $"Could not find beatmap with difficulty name containing '{difficultySubstring}'"); + return beatmap!; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs` at line 591, The helper findBeatmap currently uses First(...) which throws a generic exception when no match is found; update findBeatmap to use FirstOrDefault(...) on Beatmap.Value.BeatmapSetInfo.Beatmaps, then check for null and throw a clear exception (or call Assert.Fail) that includes the searched difficultySubstring and the available beatmap difficulty names to make test failures actionable; reference the findBeatmap method and the Beatmap.Value.BeatmapSetInfo.Beatmaps sequence when making this change.
420-451: Unnecessary cleanup step on line 450.The
TestUnscopeRevertsToOriginalSelectiontest doesn't set any star difficulty filter, yet line 450 resetsDisplayStarsMaximumto 10.1. This cleanup step appears to be copy-pasted from another test and is unnecessary here.🧹 Proposed fix to remove unnecessary cleanup
checkMatchedBeatmaps(6); AddAssert("normal difficulty is selected", () => Beatmap.Value.BeatmapInfo, () => Is.EqualTo(findBeatmap("Normal"))); - - AddStep("reset star difficulty filter", () => Config.SetValue(OsuSetting.DisplayStarsMaximum, 10.1)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs` around lines 420 - 451, The test method TestUnscopeRevertsToOriginalSelection contains an unnecessary cleanup AddStep that calls Config.SetValue(OsuSetting.DisplayStarsMaximum, 10.1); remove that AddStep (the step resetting the star difficulty filter) from the method so the test no longer performs the irrelevant Config.SetValue call; locate the call by searching for Config.SetValue and OsuSetting.DisplayStarsMaximum within TestUnscopeRevertsToOriginalSelection and delete that single AddStep block.osu.Game.Tests/Visual/UserInterface/TestSceneFirstRunSetupOverlay.cs (2)
56-68: ResetlastNotificationin setup to avoid cross-test state bleed.
lastNotificationcan carry stale data between tests if no new notification is posted.♻️ Proposed change
AddStep("setup dependencies", () => { performer.Reset(); notificationOverlay.Reset(); + lastNotification = null; performer.Setup(g => g.PerformFromScreen(It.IsAny<Action<IScreen>>(), It.IsAny<IEnumerable<Type>>())) .Callback((Action<IScreen> action, IEnumerable<Type> _) => action(null));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/UserInterface/TestSceneFirstRunSetupOverlay.cs` around lines 56 - 68, Reset lastNotification at the start of the "setup dependencies" step to avoid cross-test state bleed: inside the AddStep lambda that currently calls performer.Reset() and notificationOverlay.Reset(), set lastNotification = null (or appropriate default) before configuring performer.Setup(...) and notificationOverlay.Setup(...); reference symbols: lastNotification, AddStep("setup dependencies", ...), performer.Reset, notificationOverlay.Reset, performer.Setup, notificationOverlay.Setup.
218-218: NarrowOverlaymutability in the nested screen.A property with a private setter prevents accidental reassignment from tests/helpers.
♻️ Proposed change
- public FirstRunSetupOverlay Overlay = null!; + public FirstRunSetupOverlay Overlay { get; private set; } = null!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/UserInterface/TestSceneFirstRunSetupOverlay.cs` at line 218, Replace the publicly mutable field "Overlay" (type FirstRunSetupOverlay) with a property that has a private setter to prevent external reassignment; specifically change the declaration of Overlay from a public field to a public property with a private set (keeping the null-forgiving initializer) and update any assignments inside the nested screen to set the property internally (leave external reads unchanged). Ensure references to "Overlay" elsewhere still compile and that only the nested screen's code performs the assignment.osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchFreestyleSelect.cs (1)
59-60: Update stale log source name in abort message.On Line 59, the message uses
nameof(OnStart)insideStartAction(), which makes diagnostics misleading.♻️ Proposed fix
- Logger.Log($"{nameof(OnStart)} aborted due to {nameof(operationInProgress)}"); + Logger.Log($"{nameof(StartAction)} aborted due to {nameof(operationInProgress)}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchFreestyleSelect.cs` around lines 59 - 60, The log message inside StartAction() uses the stale nameof(OnStart) which is misleading; update the Logger.Log call in StartAction (the line currently logging $"{nameof(OnStart)} aborted due to {nameof(operationInProgress)}") to reference nameof(StartAction) instead (e.g. $"{nameof(StartAction)} aborted due to {nameof(operationInProgress)}") so diagnostics reflect the correct source.osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomSubScreen.cs (1)
252-256: Strengthen the abort test to avoid false positives.At Line 252 and Line 255, the test aborts after
Key.Downwithout proving the selection actually changed first. If focus/input fails, this can still pass. Add an intermediate assertion that the freestyle selection moved before aborting.Proposed test hardening
AddStep("select next beatmap", () => InputManager.Key(Key.Down)); + AddUntilStep("selection changed in freestyle select", + () => !Beatmap.Value.BeatmapInfo.Equals(importedSet.Beatmaps[0])); AddStep("abort", () => playlistsScreen.Stack.CurrentScreen.Exit()); AddUntilStep("beatmap not changed", () => Beatmap.Value.BeatmapInfo.Equals(importedSet.Beatmaps[0]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomSubScreen.cs` around lines 252 - 256, The test currently presses Key.Down then aborts without verifying the selection actually moved; modify the sequence around AddStep("select next beatmap", () => InputManager.Key(Key.Down)) and before calling playlistsScreen.Stack.CurrentScreen.Exit() to first capture the pre-selection (Beatmap.Value.BeatmapInfo), then assert the selection changed (e.g. via AddUntilStep or AddAssert that Beatmap.Value.BeatmapInfo != previous or equals the expected next beatmap from importedSet.Beatmaps[1]) to ensure the Key.Down had effect before aborting; reference symbols: AddStep, InputManager.Key(Key.Down), Beatmap.Value.BeatmapInfo, importedSet.Beatmaps, AddUntilStep/AddAssert, playlistsScreen.Stack.CurrentScreen.Exit().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderReversal.cs`:
- Around line 195-198: The assertion is comparing a path-relative control point
(selectedSlider.Path.ControlPoints[^2].Position) against absolute coordinates
(oldStartPos/oldEndPos); convert one side so both use the same coordinate space.
Fix by transforming the control point position to the same absolute space as
oldStartPos/oldEndPos (e.g. add the slider's drawable/anchor position or call
the appropriate space-conversion helper on selectedSlider) or alternatively
convert oldStartPos/oldEndPos into the slider/path-relative space, then use
Vector2.Distance as before to assert the middle control point isn't equal to
start or end.
- Around line 132-135: The assertion compares
selectedSlider.Path.ControlPoints[^2].Position (a path-relative coordinate) to
oldStartPos/oldEndPos (absolute playfield coordinates), causing a meaningless
large-distance pass; fix it by converting the absolute coordinates to
path-relative before comparing — e.g. obtain path-relativeStart =
selectedSlider.Path.ToPathSpace(oldStartPos) and path-relativeEnd =
selectedSlider.Path.ToPathSpace(oldEndPos) (or the inverse: convert the control
point to playfield space) and then assert
Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position,
path-relativeStart) > 1 &&
Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position,
path-relativeEnd) > 1 so the comparison is done in the same coordinate system.
In `@osu.Game.Tests/Database/RulesetStoreTests.cs`:
- Around line 152-153: Tests mutate the static LoadTestRuleset.OnlineID (e.g.
setting to 2) which can leak state between tests and cause order-dependent
flakes; update the tests that touch LoadTestRuleset.OnlineID to restore the
original value after the change (capture the original at start and set it back
in a finally/teardown) or refactor the test to avoid mutating the static at all
(use a local instance or a test-scoped subclass). Locate occurrences of
LoadTestRuleset.OnlineID in this file and either wrap the assignment in
try/finally that reassigns the saved original, or change the test to not write
the static. Ensure every test that touches OnlineID reverts it to prevent
cross-test leakage.
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneFreeModSelectOverlay.cs`:
- Around line 213-215: The IOverlayManager test dependency should be required so
missing DI fails fast: change the [Resolved] private IOverlayManager?
overlayManager { get; set; } to a non-nullable [Resolved] private
IOverlayManager overlayManager { get; set; } (and do the same for the second
occurrence at the other declaration), then remove any null-conditional usage
(?.) that skips registration; this forces the test framework to throw if the
overlay manager is not provided rather than silently continuing.
In `@osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.cs`:
- Around line 78-79: The test uses InputManager.Key(Key.ShiftLeft) which does a
press+release and releases the modifier before the binding capture; update the
AddStep call in TestSceneKeyBindingPanel to use
InputManager.PressKey(Key.ShiftLeft) to hold the modifier during capture and
ensure a subsequent AddStep uses InputManager.ReleaseKey(Key.ShiftLeft) to
release it (matching the pattern used in TestBindingSingleModifier).
In `@osu.Game.Tests/Visual/UserInterface/TestSceneFirstRunSetupOverlay.cs`:
- Around line 95-97: After calling Stack.Exit() in TestSceneFirstRunSetupOverlay
replace the immediate call to createScreen() with a wait step that ensures the
exit has completed; specifically, after invoking Stack.Exit() add an
AddUntilStep("wait for screen exit", () => Stack.CurrentScreen == null)
(matching the base class pattern like addExitAllScreensStep()) and only call
createScreen() once that check passes so the asynchronous transition/disposal
cannot race with creation.
In `@osu.Game.Tests/Visual/UserInterface/TestSceneLoadingLayer.cs`:
- Line 117: The test step description is inconsistent: AddStep currently reads
AddStep("press enter", () => InputManager.Key(Key.F8));; update the step text to
match the key being pressed, e.g. change the first argument of AddStep to "press
F8" so the log and failure messages accurately reflect the action performed by
the InputManager.Key(Key.F8) call in TestSceneLoadingLayer.
In `@osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs`:
- Line 306: Replace the call to AddStep that checks the screen with AddUntilStep
so the test waits until the condition becomes true instead of running once;
specifically change the AddStep("wait until screen is loaded", () =>
screenWithTwoOverlays.IsCurrentScreen()) to use AddUntilStep and consider
mirroring createScreen() at line 106 by also asserting
screenWithTwoOverlays.IsLoaded (e.g. wait until IsCurrentScreen() && IsLoaded())
to avoid flakiness.
In `@osu.Game/Rulesets/Ruleset.cs`:
- Around line 375-379: GetValidHitResults currently returns all enum values
which violates the strict-validation contract; change GetValidHitResults to
return the ordered HitResult values from
EnumExtensions.GetValuesInOrder<HitResult>() but filter out HitResult.None and
any enum members marked obsolete. Locate the GetValidHitResults method and
replace the direct return with a filtered sequence (use a Where filter to
exclude value == HitResult.None and use reflection to detect [Obsolete] on the
enum member) so base rulesets no longer expose non-applicable results.
In `@osu.Game/Screens/Edit/BookmarkController.cs`:
- Around line 110-116: The lenience calculation uses the raw clock rate which
can be negative during reverse playback; update the calculation of seekLenience
in BookmarkController (the variable seekLenience computed when clock.IsRunning
and using ((IAdjustableClock)clock).Rate) to use Math.Abs(rate) so the value is
always non-negative (mirror the pattern used in FrameStabilityContainer.cs),
ensuring the LastOrDefault b => b < clock.CurrentTimeAccurate - seekLenience
logic behaves correctly for reverse playback.
In `@osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs`:
- Around line 300-318: The current guard uses samplesInSelection.Length > 0
which only checks number of sample-list entries, not whether any selected
objects actually contain samples; change the condition to test the flattened
sample collection (e.g. samplesInSelection.SelectMany(s => s).Any()) so the
subsequent updates to SelectionSampleStates, SelectionBankStates,
SelectionAdditionBanksEnabled and SelectionAdditionBankStates (which call
GetStateFromSelection) only run when there are actual selected samples to
evaluate.
In `@osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs`:
- Around line 112-114: The query against beatmapManager.QueryBeatmap using
initialItem.Beatmap.OnlineID can return unrelated local beatmaps when onlineId
<= 0; update the logic in MultiplayerMatchSongSelect so that before calling
beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId) you check that onlineId
> 0 (or otherwise only query when OnlineID is valid) and fall back to the
existing local/hash-based lookup or skip the online-ID lookup when it's not
positive; change references around initialItem.Beatmap.OnlineID and beatmapInfo
to only perform the QueryBeatmap call when onlineId > 0.
In `@osu.Game/Screens/OnlinePlay/OnlinePlayFreestyleSelect.cs`:
- Around line 56-66: The lookup fallback currently sets itemLength and
beatmapSetId to 0 when realm.Run fails to find a BeatmapInfo, which then causes
downstream restrictive checks to wrongly block valid selections; change
itemLength and beatmapSetId to nullable types (double? itemLength, int?
beatmapSetId), assign them from the realm lookup (beatmap?.Length and
beatmap?.BeatmapSet?.OnlineID) without defaulting to 0, and update the
subsequent selection criteria (the checks that use itemLength and beatmapSetId)
to only apply when those nullable values have a value (e.g. use HasValue or
pattern matching) so missing local beatmaps don't trigger restrictive behavior.
In `@osu.Game/Tests/Visual/ScreenTestScene.cs`:
- Around line 58-63: The overlayContent is declared before screenStackFooter so
the footer renders on top and blocks overlays; change the initialization order
so screenStackFooter = new ScreenStackFooter(Stack, backReceptor) is created
first and overlayContent = new Container { RelativeSizeAxes = Axes.Both, Child =
DialogOverlay = new DialogOverlay() } is added afterward, ensuring
DialogOverlay/overlayContent render above the footer and can properly block
interaction.
In `@osu.Game/Users/Drawables/DrawableTeamFlag.cs`:
- Around line 49-50: The code calls textures.Get(team.FlagUrl) without guarding
the nullable FlagUrl; update DrawableTeamFlag to check the URL before lookup
(e.g. use if (!string.IsNullOrEmpty(team?.FlagUrl)) sprite.Texture =
textures.Get(team.FlagUrl); else sprite.Texture = null or a fallback texture) so
sprite.Texture is only set with a non-null, non-empty URL; reference the
sprite.Texture assignment and textures.Get(...) call in DrawableTeamFlag and the
team.FlagUrl property when making the change.
---
Outside diff comments:
In `@osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs`:
- Around line 55-61: The Debug.Assert(retries != null) must be replaced with a
runtime guard that throws when retries is uninitialized; in
ModEasyWithExtraLives, update the beginning of PerformFail() to check if retries
is null and throw an InvalidOperationException (or similar) with a clear message
that ApplyToPlayer() must be called before performing fails, then proceed with
the existing logic (if retries == 0 return true; reset health.Value, decrement
retries). This ensures failures surface in release builds instead of silently
misbehaving.
In `@osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs`:
- Around line 368-381: The early-return uses hasRelevantBank but that predicate
only checks HitSampleInfo.HIT_NORMAL while the update mutates samples with
either Name == HitSampleInfo.HIT_NORMAL or EditorAutoBank == true; update the
predicate used by SelectedItems.All(hasRelevantBank) (or the hasRelevantBank
implementation) to treat a hit object as relevant if any of its Samples or
NodeSamples satisfy (s.Name == HitSampleInfo.HIT_NORMAL || s.EditorAutoBank), so
the early-return only fires when no selected object needs the new-bank
assignment for either normal hits or editor-auto-bank samples.
---
Nitpick comments:
In `@osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderReversal.cs`:
- Around line 293-298: The two AddAssert calls have swapped assertion messages
vs the control-point indices; update the descriptive strings so they match the
indices being checked — when asserting
Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position,
selectedSlider.Path.PositionAt(0.5)) use a message stating "Last arc's control
is not at the slider's middle", and when asserting
Vector2.Distance(selectedSlider.Path.ControlPoints[1].Position,
selectedSlider.Path.PositionAt(0.5)) use "First arc's control is not at the
slider's middle" so the messages correspond to the actual ControlPoints indices
in TestSceneSliderReversal.
In `@osu.Game.Rulesets.Osu.Tests/Mods/TestSceneOsuModEasy.cs`:
- Around line 51-54: The empty catch should be narrowed to the expected
exception type so unexpected errors aren't swallowed: change the broad catch in
TestSceneOsuModEasy around the ApplyToPlayer call (testing ModEasyWithExtraLives
reapplication) to catch InvalidOperationException specifically and handle it the
same way (i.e., ignore or assert as intended), leaving other exceptions to
surface.
- Line 41: PassCondition currently dereferences Player and casts it to
ModEasyTestPlayer; add a defensive null check to ensure Player is not null
before accessing ((ModEasyTestPlayer)Player).FailuresSuppressed and before using
reapplied logic. Update the PassCondition implementation to first check Player
!= null (or use pattern matching like "Player is ModEasyTestPlayer pe &&
pe.FailuresSuppressed > 0") so the condition short-circuits when Player is null
and avoids a NullReferenceException while preserving the existing behavior when
a ModEasyTestPlayer is present.
In `@osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs`:
- Line 189: The test currently only checks the bookmark count; update the
assertion that uses AddAssert("created difficulty has bookmarks", () =>
EditorBeatmap.Bookmarks.Count == 2) to verify the actual bookmark values (e.g.,
assert EditorBeatmap.Bookmarks.SequenceEqual(new[] {500, 1000}) or check both
Contains(500) and Contains(1000) and order if relevant) so the test validates
contents not just count; apply same pattern to the other similar assertions
referenced (lines with AddAssert checks for bookmarks at the other test points).
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneFreeModSelectOverlay.cs`:
- Around line 81-85: The test currently locates the SelectAllModsButton globally
via this.ChildrenOfType<SelectAllModsButton>().Single(), which can return
ambiguous matches; change the lookup to scope to the overlay container (e.g. use
freeModSelectOverlay.ChildrenOfType<SelectAllModsButton>().Single() or otherwise
query from freeModSelectOverlay) in the AddStep that moves and clicks the button
so the test targets the overlay's button explicitly (keep the existing
InputManager.MoveMouseTo and InputManager.Click usage but obtain the target from
freeModSelectOverlay instead of this).
- Around line 242-246: Move the overlay registration disposal to occur before
the base class teardown: in the override Dispose(bool isDisposing) of
TestSceneFreeModSelectOverlay, call overlayRegistration?.Dispose() before
calling base.Dispose(isDisposing) so the overlay registration is torn down
deterministically prior to parent disposal and avoids expired overlay state
after the base class disposes.
In `@osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomSubScreen.cs`:
- Around line 252-256: The test currently presses Key.Down then aborts without
verifying the selection actually moved; modify the sequence around
AddStep("select next beatmap", () => InputManager.Key(Key.Down)) and before
calling playlistsScreen.Stack.CurrentScreen.Exit() to first capture the
pre-selection (Beatmap.Value.BeatmapInfo), then assert the selection changed
(e.g. via AddUntilStep or AddAssert that Beatmap.Value.BeatmapInfo != previous
or equals the expected next beatmap from importedSet.Beatmaps[1]) to ensure the
Key.Down had effect before aborting; reference symbols: AddStep,
InputManager.Key(Key.Down), Beatmap.Value.BeatmapInfo, importedSet.Beatmaps,
AddUntilStep/AddAssert, playlistsScreen.Stack.CurrentScreen.Exit().
In `@osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs`:
- Around line 575-589: In scopeBeatmap, avoid calling Single(...) on the enabled
SpreadDisplay collections; replace the Single( d => d.Enabled.Value ) usages for
both PanelBeatmapSet.SpreadDisplay and PanelBeatmapStandalone.SpreadDisplay with
SingleOrDefault(...) and then add an explicit assertion/guard that the result is
not null (and optionally that no duplicate enabled items exist) before calling
TriggerClick(), so test failures report a clear assertion instead of an
InvalidOperationException.
- Line 591: The helper findBeatmap currently uses First(...) which throws a
generic exception when no match is found; update findBeatmap to use
FirstOrDefault(...) on Beatmap.Value.BeatmapSetInfo.Beatmaps, then check for
null and throw a clear exception (or call Assert.Fail) that includes the
searched difficultySubstring and the available beatmap difficulty names to make
test failures actionable; reference the findBeatmap method and the
Beatmap.Value.BeatmapSetInfo.Beatmaps sequence when making this change.
- Around line 420-451: The test method TestUnscopeRevertsToOriginalSelection
contains an unnecessary cleanup AddStep that calls
Config.SetValue(OsuSetting.DisplayStarsMaximum, 10.1); remove that AddStep (the
step resetting the star difficulty filter) from the method so the test no longer
performs the irrelevant Config.SetValue call; locate the call by searching for
Config.SetValue and OsuSetting.DisplayStarsMaximum within
TestUnscopeRevertsToOriginalSelection and delete that single AddStep block.
In `@osu.Game.Tests/Visual/UserInterface/TestSceneFirstRunSetupOverlay.cs`:
- Around line 56-68: Reset lastNotification at the start of the "setup
dependencies" step to avoid cross-test state bleed: inside the AddStep lambda
that currently calls performer.Reset() and notificationOverlay.Reset(), set
lastNotification = null (or appropriate default) before configuring
performer.Setup(...) and notificationOverlay.Setup(...); reference symbols:
lastNotification, AddStep("setup dependencies", ...), performer.Reset,
notificationOverlay.Reset, performer.Setup, notificationOverlay.Setup.
- Line 218: Replace the publicly mutable field "Overlay" (type
FirstRunSetupOverlay) with a property that has a private setter to prevent
external reassignment; specifically change the declaration of Overlay from a
public field to a public property with a private set (keeping the null-forgiving
initializer) and update any assignments inside the nested screen to set the
property internally (leave external reads unchanged). Ensure references to
"Overlay" elsewhere still compile and that only the nested screen's code
performs the assignment.
In `@osu.Game.Tests/Visual/UserInterface/TestSceneLoadingLayer.cs`:
- Line 138: The nested test class PressableButton is unnecessarily public;
change its declaration inside TestSceneLoadingLayer from "public partial class
PressableButton" to "private sealed partial class PressableButton" (or "private
sealed class PressableButton" if partial isn't needed) to restrict visibility
and seal it, keeping the class usage the same but reducing the surface area.
In `@osu.Game/Online/API/Requests/Responses/APITeam.cs`:
- Line 21: The FlagUrl field is declared nullable but initialized to an empty
string; change this to be semantically consistent by either making FlagUrl
non-nullable (remove the '?') and keep the empty-string default, or keep it
nullable and set the default to null; update the APITeam class's FlagUrl
declaration accordingly (refer to the FlagUrl field in APITeam) so the type and
default value match the intended semantics.
In `@osu.Game/Online/Multiplayer/MultiplayerRoomUserRole.cs`:
- Around line 6-10: Assign explicit integer values to the
MultiplayerRoomUserRole enum to ensure MessagePack serialization stability:
update the MultiplayerRoomUserRole definition (used by MultiplayerRoomUser) so
each member has a fixed integer (e.g., Player = 0, Referee = 1) to prevent
future reordering/insertions from breaking compatibility.
In `@osu.Game/Overlays/Settings/Sections/Input/KeyBindingRow.KeyButton.cs`:
- Around line 145-156: The current assembly of the keys array can produce
duplicate modifier entries or multiple non-modifier keys (contradicting the
method doc); modify the logic around fullState.Keys, triggerKey and combination
creation so you (1) deduplicate identical keys (e.g. via a distinct step) to
prevent a modifier appearing twice, and (2) enforce "only a single non-modifier
key" by ensuring at most one non-modifier remains (if multiple non-modifiers are
present, drop all but the intended triggerKey or the last non-modifier) before
calling UpdateKeyCombination(new KeyCombination(...)); operate on the variables
shown (fullState.Keys, triggerKey, keys, KeyBinding.Value.RulesetName,
GetVirtualKey, KeyCombination) so the global-vs-gameplay virtual-key mapping
logic is preserved.
In `@osu.Game/Overlays/Settings/Sections/InputSubsection.cs`:
- Around line 121-128: The ToggleableHeader creation uses a hardcoded
MarginPadding Vertical = 12 which causes a visual mismatch with the base
CreateHeader() that uses the VERTICAL_PADDING constant (11.5f); update the
ToggleableHeader's headerText Margin to use VERTICAL_PADDING instead of 12 so
both ToggleableHeader and CreateHeader() share the same vertical spacing (locate
the headerText instantiation in InputSubsection.cs and replace the hardcoded
Vertical value with VERTICAL_PADDING).
- Around line 133-147: Replace the manual bidirectional wiring in LoadComplete
with a direct bind target to simplify the logic: instead of subscribing to
switchButton.Current.ValueChanged and using Current.BindValueChanged with
temporary switchButton.Current.Disabled toggles, call
Current.BindTarget(switchButton.Current) (or
switchButton.Current.BindTarget(Current)) on load to connect the two bindables,
and keep only a simple one-way update for the Disabled state controlled by the
existing toggleable flag (e.g. set switchButton.Current.Disabled = !toggleable
when toggleable changes). This eliminates the temporary disabling dance in
LoadComplete and uses the built‑in BindTarget mechanism to avoid feedback loops
while preserving the toggleable behavior.
In `@osu.Game/Overlays/Toolbar/ToolbarButton.cs`:
- Around line 160-169: The code is correct but the repeated identifier "Hotkey"
(the ToolbarButton property, the HotkeyDisplay.Hotkey property, and the Hotkey
record struct) is confusing; to clarify, capture the ToolbarButton's
Nullable<GlobalAction> into a local (e.g. var globalAction = Hotkey.Value)
inside the if (Hotkey != null) block and then set HotkeyDisplay.Hotkey using
that local (Hotkey = new Hotkey(globalAction)), leaving all other behavior
unchanged and keeping references to HotkeyDisplay and the Hotkey record type.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/Queue/QueueController.cs`:
- Around line 193-194: The Complete(MatchmakingRoomInvitationParams invitation)
method currently ignores its invitation parameter; either consume
invitation.Type where room-pool-specific behavior or explicitly discard the
parameter to document intent. Update the Complete method (in QueueController.cs)
to reference invitation.Type in the UX/path-selection logic if pool-specific
handling is required, otherwise add an explicit discard (or comment) for the
invitation parameter so static analysis and readers know the unused parameter is
intentional.
In `@osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchFreestyleSelect.cs`:
- Around line 59-60: The log message inside StartAction() uses the stale
nameof(OnStart) which is misleading; update the Logger.Log call in StartAction
(the line currently logging $"{nameof(OnStart)} aborted due to
{nameof(operationInProgress)}") to reference nameof(StartAction) instead (e.g.
$"{nameof(StartAction)} aborted due to {nameof(operationInProgress)}") so
diagnostics reflect the correct source.
In `@osu.Game/Tests/Visual/ScreenTestScene.cs`:
- Line 56: Remove or resolve the stray TODO comment inside the ScreenTestScene
composition path: either delete the line or replace it with a reference to a
tracked issue ID and brief rationale; update the ScreenTestScene class (where
the TODO sits) to reflect the decision (deleted comment if resolved or a comment
like "See ISSUE-1234: reason" if tracked) so the ambiguity is removed before
merging.
Comment on lines
+132
to
+135
| AddAssert("Middle control point is not at start or end", () => | ||
| Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldStartPos) > 1 && | ||
| Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldEndPos) > 1 | ||
| ); |
There was a problem hiding this comment.
Coordinate system mismatch in assertion.
ControlPoints[^2].Position is a path-relative position (small values relative to path start), while oldStartPos and oldEndPos are absolute playfield coordinates (~256, ~192). This comparison will always pass trivially due to the large coordinate difference, not because the middle control point is actually positioned correctly.
Proposed fix: Compare against path-relative positions
AddAssert("Middle control point is not at start or end", () =>
- Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldStartPos) > 1 &&
- Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldEndPos) > 1
+ Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, selectedSlider.Path.ControlPoints[0].Position) > 1 &&
+ Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, selectedSlider.Path.ControlPoints[^1].Position) > 1
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderReversal.cs` around lines
132 - 135, The assertion compares selectedSlider.Path.ControlPoints[^2].Position
(a path-relative coordinate) to oldStartPos/oldEndPos (absolute playfield
coordinates), causing a meaningless large-distance pass; fix it by converting
the absolute coordinates to path-relative before comparing — e.g. obtain
path-relativeStart = selectedSlider.Path.ToPathSpace(oldStartPos) and
path-relativeEnd = selectedSlider.Path.ToPathSpace(oldEndPos) (or the inverse:
convert the control point to playfield space) and then assert
Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position,
path-relativeStart) > 1 &&
Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position,
path-relativeEnd) > 1 so the comparison is done in the same coordinate system.
Comment on lines
+195
to
+198
| AddAssert("Middle control point is not at start or end", () => | ||
| Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldStartPos) > 1 && | ||
| Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldEndPos) > 1 | ||
| ); |
There was a problem hiding this comment.
Same coordinate system mismatch.
Same issue as noted above at lines 132-135 — path-relative positions compared to absolute coordinates.
Proposed fix
AddAssert("Middle control point is not at start or end", () =>
- Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldStartPos) > 1 &&
- Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, oldEndPos) > 1
+ Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, selectedSlider.Path.ControlPoints[0].Position) > 1 &&
+ Vector2.Distance(selectedSlider.Path.ControlPoints[^2].Position, selectedSlider.Path.ControlPoints[^1].Position) > 1
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderReversal.cs` around lines
195 - 198, The assertion is comparing a path-relative control point
(selectedSlider.Path.ControlPoints[^2].Position) against absolute coordinates
(oldStartPos/oldEndPos); convert one side so both use the same coordinate space.
Fix by transforming the control point position to the same absolute space as
oldStartPos/oldEndPos (e.g. add the slider's drawable/anchor position or call
the appropriate space-conversion helper on selectedSlider) or alternatively
convert oldStartPos/oldEndPos into the slider/path-relative space, then use
Vector2.Distance as before to assert the middle control point isn't equal to
start or end.
Comment on lines
+152
to
+153
| LoadTestRuleset.OnlineID = 2; | ||
|
|
There was a problem hiding this comment.
Static OnlineID mutation can leak across tests.
Line 152 changes a static value (Line 191) without restoring it. This makes test behavior order-dependent and can cause flaky outcomes in this fixture.
Suggested isolation fix
+ [SetUp]
+ public void SetUp()
+ {
+ LoadTestRuleset.OnlineID = -1;
+ LoadTestRuleset.HasImplementations = true;
+ LoadTestRuleset.Version = Ruleset.CURRENT_RULESET_API_VERSION;
+ }Also applies to: 191-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Database/RulesetStoreTests.cs` around lines 152 - 153, Tests
mutate the static LoadTestRuleset.OnlineID (e.g. setting to 2) which can leak
state between tests and cause order-dependent flakes; update the tests that
touch LoadTestRuleset.OnlineID to restore the original value after the change
(capture the original at start and set it back in a finally/teardown) or
refactor the test to avoid mutating the static at all (use a local instance or a
test-scoped subclass). Locate occurrences of LoadTestRuleset.OnlineID in this
file and either wrap the assignment in try/finally that reassigns the saved
original, or change the test to not write the static. Ensure every test that
touches OnlineID reverts it to prevent cross-test leakage.
Comment on lines
+213
to
+215
| [Resolved] | ||
| private IOverlayManager? overlayManager { get; set; } | ||
|
|
There was a problem hiding this comment.
Fail fast on missing IOverlayManager in test DI.
ScreenTestScene is expected to provide IOverlayManager; making it nullable and using ?. can silently skip blocking-overlay registration and mask wiring regressions.
Suggested change
-[Resolved]
-private IOverlayManager? overlayManager { get; set; }
+[Resolved]
+private IOverlayManager overlayManager { get; set; } = null!;
...
-overlayRegistration = overlayManager?.RegisterBlockingOverlay(Overlay);
+overlayRegistration = overlayManager.RegisterBlockingOverlay(Overlay);Also applies to: 231-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneFreeModSelectOverlay.cs` around
lines 213 - 215, The IOverlayManager test dependency should be required so
missing DI fails fast: change the [Resolved] private IOverlayManager?
overlayManager { get; set; } to a non-nullable [Resolved] private
IOverlayManager overlayManager { get; set; } (and do the same for the second
occurrence at the other declaration), then remove any null-conditional usage
(?.) that skips registration; this forces the test framework to throw if the
overlay manager is not provided rather than silently continuing.
Comment on lines
+78
to
+79
| AddStep("press left shift", () => InputManager.Key(Key.ShiftLeft)); | ||
| AddStep("release left shift", () => InputManager.ReleaseKey(Key.ShiftLeft)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code in the test file
cat -n osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.cs | sed -n '74,82p'Repository: winnerspiros/osu
Length of output: 493
🏁 Script executed:
# Find the InputManager class and Key() method implementation
fd -t f "InputManager" --type f | head -20Repository: winnerspiros/osu
Length of output: 817
🏁 Script executed:
# Search for the Key method implementation in the codebase
rg -t cs "public.*void.*Key\(Key" -A 5 | head -50Repository: winnerspiros/osu
Length of output: 577
🏁 Script executed:
# Search specifically in osu.Framework for InputManager
fd -t f -i inputmanagerRepository: winnerspiros/osu
Length of output: 817
🏁 Script executed:
# Check the test class declaration to see its base class
head -30 osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.csRepository: winnerspiros/osu
Length of output: 989
🏁 Script executed:
# Search for PressKey and Key methods in the codebase
rg "(?:void|InputManager)\s+(?:PressKey|Key)\(" -B 2 -A 3 --type cs | head -100Repository: winnerspiros/osu
Length of output: 42
🏁 Script executed:
# Look for InputManager usage in test base classes
rg "class.*OsuTestScene|class.*ScreenTestScene" osu.Game.Tests/Visual/ -A 5 --type cs | head -50Repository: winnerspiros/osu
Length of output: 4564
🏁 Script executed:
# Find OsuManualInputManagerTestScene definition
cat -n osu.Game.Tests/Visual/OsuManualInputManagerTestScene.cs | head -50Repository: winnerspiros/osu
Length of output: 147
🏁 Script executed:
# Search for the Key method in the codebase more broadly
rg "\.Key\(Key\." osu.Game.Tests/ -B 2 -A 2 --type cs | head -80Repository: winnerspiros/osu
Length of output: 8169
🏁 Script executed:
# Search for OsuManualInputManagerTestScene in all files
fd -t f "OsuManualInputManagerTestScene"Repository: winnerspiros/osu
Length of output: 115
🏁 Script executed:
# Look for the file that contains this class
rg "class OsuManualInputManagerTestScene" -t csRepository: winnerspiros/osu
Length of output: 195
🏁 Script executed:
# Search for InputManager class definition
rg "class InputManager" -t cs -A 10 | head -80Repository: winnerspiros/osu
Length of output: 42
🏁 Script executed:
# Read the OsuManualInputManagerTestScene file
cat osu.Game/Tests/Visual/OsuManualInputManagerTestScene.csRepository: winnerspiros/osu
Length of output: 6510
🏁 Script executed:
# Search for references to InputManager in test files to understand its usage pattern
rg "InputManager\." osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.cs -A 1 -B 1 | head -40Repository: winnerspiros/osu
Length of output: 2059
🏁 Script executed:
# Double-check by looking at what checkBinding expects
rg "checkBinding" osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.cs -B 2 -A 2 | head -40Repository: winnerspiros/osu
Length of output: 1560
🏁 Script executed:
# Compare the test methods more carefully
sed -n '61,81p' osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.csRepository: winnerspiros/osu
Length of output: 923
Use PressKey/ReleaseKey pattern to properly hold the modifier during binding capture.
Line 78 uses InputManager.Key(Key.ShiftLeft) which performs a complete key press+release in one operation. This means the modifier is released before the binding capture completes, preventing the binding from registering. This differs from the existing TestBindingSingleModifier() test which correctly uses InputManager.PressKey(Key.ShiftLeft) followed by InputManager.ReleaseKey(Key.ShiftLeft) to hold the key while the binding is captured.
Proposed fix
- AddStep("press left shift", () => InputManager.Key(Key.ShiftLeft));
+ AddStep("press left shift", () => InputManager.PressKey(Key.ShiftLeft));
AddStep("release left shift", () => InputManager.ReleaseKey(Key.ShiftLeft));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AddStep("press left shift", () => InputManager.Key(Key.ShiftLeft)); | |
| AddStep("release left shift", () => InputManager.ReleaseKey(Key.ShiftLeft)); | |
| AddStep("press left shift", () => InputManager.PressKey(Key.ShiftLeft)); | |
| AddStep("release left shift", () => InputManager.ReleaseKey(Key.ShiftLeft)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/Settings/TestSceneKeyBindingPanel.cs` around lines 78 -
79, The test uses InputManager.Key(Key.ShiftLeft) which does a press+release and
releases the modifier before the binding capture; update the AddStep call in
TestSceneKeyBindingPanel to use InputManager.PressKey(Key.ShiftLeft) to hold the
modifier during capture and ensure a subsequent AddStep uses
InputManager.ReleaseKey(Key.ShiftLeft) to release it (matching the pattern used
in TestBindingSingleModifier).
Comment on lines
+300
to
+318
| if (samplesInSelection.Length > 0) | ||
| { | ||
| bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName)); | ||
| } | ||
| foreach ((string sampleName, var bindable) in SelectionSampleStates) | ||
| { | ||
| bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName)); | ||
| } | ||
|
|
||
| foreach ((string bankName, var bindable) in SelectionBankStates) | ||
| { | ||
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName); | ||
| } | ||
| foreach ((string bankName, var bindable) in SelectionBankStates) | ||
| { | ||
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName); | ||
| } | ||
|
|
||
| SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL); | ||
| SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL); | ||
|
|
||
| foreach ((string bankName, var bindable) in SelectionAdditionBankStates) | ||
| { | ||
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL), | ||
| h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank)); | ||
| foreach ((string bankName, var bindable) in SelectionAdditionBankStates) | ||
| { | ||
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL), | ||
| h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank)); | ||
| } |
There was a problem hiding this comment.
Guard is checking sample-list count, not actual selected samples.
enumerateAllSamples() yields lists, so this guard still passes when selected objects have empty sample lists. The ternary updates then still run on an effectively empty sample set.
💡 Suggested fix
var samplesInSelection = SelectedItems.SelectMany(enumerateAllSamples).ToArray();
+var selectedSamples = samplesInSelection.SelectMany(s => s).ToArray();
-if (samplesInSelection.Length > 0)
+if (selectedSamples.Length > 0)
{
foreach ((string sampleName, var bindable) in SelectionSampleStates)
{
bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName));
}
foreach ((string bankName, var bindable) in SelectionBankStates)
{
- bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName);
+ bindable.Value = GetStateFromSelection(selectedSamples.Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName);
}
- SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL);
+ SelectionAdditionBanksEnabled.Value = selectedSamples.Any(o => o.Name != HitSampleInfo.HIT_NORMAL);
foreach ((string bankName, var bindable) in SelectionAdditionBankStates)
{
- bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL),
+ bindable.Value = GetStateFromSelection(selectedSamples.Where(o => o.Name != HitSampleInfo.HIT_NORMAL),
h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank));
}
}
+else
+{
+ SelectionAdditionBanksEnabled.Value = false;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (samplesInSelection.Length > 0) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName)); | |
| } | |
| foreach ((string sampleName, var bindable) in SelectionSampleStates) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName)); | |
| } | |
| foreach ((string bankName, var bindable) in SelectionBankStates) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName); | |
| } | |
| foreach ((string bankName, var bindable) in SelectionBankStates) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName); | |
| } | |
| SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL); | |
| SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL); | |
| foreach ((string bankName, var bindable) in SelectionAdditionBankStates) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL), | |
| h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank)); | |
| foreach ((string bankName, var bindable) in SelectionAdditionBankStates) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL), | |
| h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank)); | |
| } | |
| var selectedSamples = samplesInSelection.SelectMany(s => s).ToArray(); | |
| if (selectedSamples.Length > 0) | |
| { | |
| foreach ((string sampleName, var bindable) in SelectionSampleStates) | |
| { | |
| bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName)); | |
| } | |
| foreach ((string bankName, var bindable) in SelectionBankStates) | |
| { | |
| bindable.Value = GetStateFromSelection(selectedSamples.Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName); | |
| } | |
| SelectionAdditionBanksEnabled.Value = selectedSamples.Any(o => o.Name != HitSampleInfo.HIT_NORMAL); | |
| foreach ((string bankName, var bindable) in SelectionAdditionBankStates) | |
| { | |
| bindable.Value = GetStateFromSelection(selectedSamples.Where(o => o.Name != HitSampleInfo.HIT_NORMAL), | |
| h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank)); | |
| } | |
| } | |
| else | |
| { | |
| SelectionAdditionBanksEnabled.Value = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs` around
lines 300 - 318, The current guard uses samplesInSelection.Length > 0 which only
checks number of sample-list entries, not whether any selected objects actually
contain samples; change the condition to test the flattened sample collection
(e.g. samplesInSelection.SelectMany(s => s).Any()) so the subsequent updates to
SelectionSampleStates, SelectionBankStates, SelectionAdditionBanksEnabled and
SelectionAdditionBankStates (which call GetStateFromSelection) only run when
there are actual selected samples to evaluate.
Comment on lines
+112
to
+114
| int onlineId = initialItem.Beatmap.OnlineID; | ||
| beatmapInfo = beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId); | ||
| } |
There was a problem hiding this comment.
Guard online-ID lookup to avoid resolving unrelated beatmaps.
On Lines 112-114, querying when onlineId <= 0 can match arbitrary local beatmaps with unset online IDs.
🐛 Proposed fix
if (beatmapInfo == null)
{
int onlineId = initialItem.Beatmap.OnlineID;
- beatmapInfo = beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId);
+ if (onlineId > 0)
+ beatmapInfo = beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int onlineId = initialItem.Beatmap.OnlineID; | |
| beatmapInfo = beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId); | |
| } | |
| int onlineId = initialItem.Beatmap.OnlineID; | |
| if (onlineId > 0) | |
| beatmapInfo = beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs` around
lines 112 - 114, The query against beatmapManager.QueryBeatmap using
initialItem.Beatmap.OnlineID can return unrelated local beatmaps when onlineId
<= 0; update the logic in MultiplayerMatchSongSelect so that before calling
beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId) you check that onlineId
> 0 (or otherwise only query when OnlineID is valid) and fall back to the
existing local/hash-based lookup or skip the online-ID lookup when it's not
positive; change references around initialItem.Beatmap.OnlineID and beatmapInfo
to only perform the QueryBeatmap call when onlineId > 0.
Comment on lines
+56
to
+66
| double itemLength = 0; | ||
| int beatmapSetId = 0; | ||
|
|
||
| realm.Run(r => | ||
| { | ||
| int beatmapId = item.Beatmap.OnlineID; | ||
| BeatmapInfo? beatmap = r.All<BeatmapInfo>().FirstOrDefault(b => b.OnlineID == beatmapId); | ||
|
|
||
| itemLength = beatmap?.Length ?? 0; | ||
| beatmapSetId = beatmap?.BeatmapSet?.OnlineID ?? 0; | ||
| }); |
There was a problem hiding this comment.
Fallback restriction values can block valid selections.
On Lines 56-66, a failed realm lookup leaves itemLength/beatmapSetId as 0, which then applies restrictive criteria (Lines 69-74). This can prevent valid selections when the playlist beatmap is not locally available.
🐛 Proposed fix
- double itemLength = 0;
- int beatmapSetId = 0;
+ double itemLength = item.Beatmap.Length;
+ int beatmapSetId = item.Beatmap.BeatmapSet?.OnlineID ?? 0;
realm.Run(r =>
{
int beatmapId = item.Beatmap.OnlineID;
- BeatmapInfo? beatmap = r.All<BeatmapInfo>().FirstOrDefault(b => b.OnlineID == beatmapId);
-
- itemLength = beatmap?.Length ?? 0;
- beatmapSetId = beatmap?.BeatmapSet?.OnlineID ?? 0;
+ if (beatmapId <= 0)
+ return;
+
+ BeatmapInfo? beatmap = r.All<BeatmapInfo>().FirstOrDefault(b => b.OnlineID == beatmapId);
+ if (beatmap == null)
+ return;
+
+ itemLength = beatmap.Length;
+ beatmapSetId = beatmap.BeatmapSet?.OnlineID ?? beatmapSetId;
});Also applies to: 69-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/OnlinePlayFreestyleSelect.cs` around lines 56 -
66, The lookup fallback currently sets itemLength and beatmapSetId to 0 when
realm.Run fails to find a BeatmapInfo, which then causes downstream restrictive
checks to wrongly block valid selections; change itemLength and beatmapSetId to
nullable types (double? itemLength, int? beatmapSetId), assign them from the
realm lookup (beatmap?.Length and beatmap?.BeatmapSet?.OnlineID) without
defaulting to 0, and update the subsequent selection criteria (the checks that
use itemLength and beatmapSetId) to only apply when those nullable values have a
value (e.g. use HasValue or pattern matching) so missing local beatmaps don't
trigger restrictive behavior.
Comment on lines
+58
to
63
| overlayContent = new Container | ||
| { | ||
| RelativeSizeAxes = Axes.Both, | ||
| Child = DialogOverlay = new DialogOverlay() | ||
| }, | ||
| screenStackFooter = new ScreenStackFooter(Stack, backReceptor) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file and see the exact ordering
cat -n osu.Game/Tests/Visual/ScreenTestScene.cs | sed -n '50,70p'Repository: winnerspiros/osu
Length of output: 1131
🏁 Script executed:
# Search for patterns around blocking overlays and z-order in the codebase
rg -nP --type=cs 'RegisterBlockingOverlay|DialogOverlay' -B2 -A2 | head -60Repository: winnerspiros/osu
Length of output: 4487
🏁 Script executed:
# Look for ScreenStackFooter usage and ordering patterns
rg -nP --type=cs 'ScreenStackFooter' -B5 -A5Repository: winnerspiros/osu
Length of output: 5499
🏁 Script executed:
# Search for RegisterBlockingOverlay and understand blocking overlay behavior
rg -nP --type=cs 'RegisterBlockingOverlay' -B3 -A8Repository: winnerspiros/osu
Length of output: 20032
🏁 Script executed:
# Look at DialogOverlay implementation to understand if it's a blocking overlay
fd -e cs -x grep -l 'class DialogOverlay' {} \;Repository: winnerspiros/osu
Length of output: 96
🏁 Script executed:
# Check ScreenStackFooter to see if it's interactive/blocking
head -100 osu.Game/Screens/Footer/ScreenStackFooter.csRepository: winnerspiros/osu
Length of output: 3382
🏁 Script executed:
# Search for blocking overlay registration in ScreenTestScene or similar test scenes
rg -nP --type=cs 'ScreenTestScene|RegisterBlockingOverlay' osu.Game/Tests/Visual/ScreenTestScene.csRepository: winnerspiros/osu
Length of output: 620
🏁 Script executed:
# Look at DialogOverlay implementation to understand its nature
cat -n osu.Game/Overlays/DialogOverlay.cs | head -50Repository: winnerspiros/osu
Length of output: 1948
🏁 Script executed:
# Check if there's any depth management or z-order specification in ScreenTestScene
rg -nP --type=cs 'Depth|Z-Order|ZIndex' osu.Game/Tests/Visual/ScreenTestScene.csRepository: winnerspiros/osu
Length of output: 42
🏁 Script executed:
# Look at the complete ScreenTestScene initialization to understand the full context
cat -n osu.Game/Tests/Visual/ScreenTestScene.cs | sed -n '38,75p'Repository: winnerspiros/osu
Length of output: 2003
🏁 Script executed:
# Search for how other test scenes or similar components order overlays and footers
rg -nP --type=cs -B2 -A2 'overlayContent.*screenStackFooter|screenStackFooter.*overlayContent'Repository: winnerspiros/osu
Length of output: 42
🏁 Script executed:
# Check if ScreenStackFooter or BackButton has any Depth specification
rg -nP --type=cs 'Depth' osu.Game/Screens/Footer/ScreenStackFooter.csRepository: winnerspiros/osu
Length of output: 42
🏁 Script executed:
# Look at BackButton implementation
fd -e cs -x grep -l 'class BackButton' {} \;Repository: winnerspiros/osu
Length of output: 155
🏁 Script executed:
# Search for any tests or issues mentioning overlay blocking problems
rg -nP --type=cs 'overlayContent|blocking.*overlay' osu.Game/Tests/Visual/ScreenTestScene.cs -A3Repository: winnerspiros/osu
Length of output: 582
🏁 Script executed:
# Check the complete DialogOverlay to understand its blocking nature
cat -n osu.Game/Overlays/DialogOverlay.cs | head -80Repository: winnerspiros/osu
Length of output: 3252
🏁 Script executed:
# Check BackButton to see if it's interactive and could interfere
head -80 osu.Game/Graphics/UserInterface/BackButton.csRepository: winnerspiros/osu
Length of output: 1986
🏁 Script executed:
# Look for any Depth or Z-order related code that might override declaration order
rg -nP --type=cs 'Depth\s*=' osu.Game/Tests/Visual/ScreenTestScene.csRepository: winnerspiros/osu
Length of output: 42
🏁 Script executed:
# Check if there are any existing tests that would fail due to overlay z-order issues
rg -nP --type=cs 'RegisterBlockingOverlay|DialogOverlay.*Push' osu.Game/Tests/Visual/ScreenTestScene.cs -B5 -A10 | head -60Repository: winnerspiros/osu
Length of output: 1105
Fix blocking overlay z-order in ScreenTestScene.
overlayContent (lines 58–62) is declared before screenStackFooter (lines 63–66), causing blocking overlays to render underneath the interactive footer. Since children are rendered in declaration order, screenStackFooter sits on top with its full-screen size, preventing overlays from properly blocking interaction.
Move overlayContent after screenStackFooter to ensure blocking overlays render on top:
Proposed fix
content = new Container { RelativeSizeAxes = Axes.Both },
- overlayContent = new Container
- {
- RelativeSizeAxes = Axes.Both,
- Child = DialogOverlay = new DialogOverlay()
- },
screenStackFooter = new ScreenStackFooter(Stack, backReceptor)
{
BackButtonPressed = () => Stack.Exit()
- }
+ },
+ overlayContent = new Container
+ {
+ RelativeSizeAxes = Axes.Both,
+ Child = DialogOverlay = new DialogOverlay()
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| overlayContent = new Container | |
| { | |
| RelativeSizeAxes = Axes.Both, | |
| Child = DialogOverlay = new DialogOverlay() | |
| }, | |
| screenStackFooter = new ScreenStackFooter(Stack, backReceptor) | |
| screenStackFooter = new ScreenStackFooter(Stack, backReceptor) | |
| { | |
| BackButtonPressed = () => Stack.Exit() | |
| }, | |
| overlayContent = new Container | |
| { | |
| RelativeSizeAxes = Axes.Both, | |
| Child = DialogOverlay = new DialogOverlay() | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Tests/Visual/ScreenTestScene.cs` around lines 58 - 63, The
overlayContent is declared before screenStackFooter so the footer renders on top
and blocks overlays; change the initialization order so screenStackFooter = new
ScreenStackFooter(Stack, backReceptor) is created first and overlayContent = new
Container { RelativeSizeAxes = Axes.Both, Child = DialogOverlay = new
DialogOverlay() } is added afterward, ensuring DialogOverlay/overlayContent
render above the footer and can properly block interaction.
Comment on lines
+49
to
+50
| if (team != null) | ||
| sprite.Texture = textures.Get(team.FlagUrl); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding APITeam.cs =="
fd 'APITeam\.cs' -type f
echo "== Finding LargeTextureStore.cs =="
fd 'LargeTextureStore\.cs' -type f
echo "== Finding DrawableTeamFlag.cs =="
fd 'DrawableTeamFlag\.cs' -type fRepository: winnerspiros/osu
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding APITeam.cs =="
fd 'APITeam\.cs'
echo "== Finding LargeTextureStore.cs =="
fd 'LargeTextureStore\.cs'
echo "== Finding DrawableTeamFlag.cs =="
fd 'DrawableTeamFlag\.cs'Repository: winnerspiros/osu
Length of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== APITeam.cs content =="
cat -n "osu.Game/Online/API/Requests/Responses/APITeam.cs"
echo ""
echo "== Searching for LargeTextureStore =="
rg 'class LargeTextureStore' --type cs
echo ""
echo "== DrawableTeamFlag.cs context (lines 40-60) =="
sed -n '40,60p' "osu.Game/Users/Drawables/DrawableTeamFlag.cs"Repository: winnerspiros/osu
Length of output: 961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DrawableTeamFlag.cs (lines 1-70) =="
sed -n '1,70p' "osu.Game/Users/Drawables/DrawableTeamFlag.cs"
echo ""
echo "== Search for LargeTextureStore definition/usage =="
rg 'LargeTextureStore' --type cs -A 2 | head -50
echo ""
echo "== Search for where textures parameter comes from =="
rg 'textures\.Get' --type cs -B 2 -A 2 | head -80Repository: winnerspiros/osu
Length of output: 11757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Searching for LargeTextureStore definition =="
rg 'class LargeTextureStore' -g '*.cs'
echo ""
echo "== Checking DrawableAvatar similar pattern =="
sed -n '1,70p' "osu.Game/Users/Drawables/DrawableAvatar.cs"
echo ""
echo "== Checking UserCoverBackground pattern =="
sed -n '40,80p' "osu.Game/Users/UserCoverBackground.cs"
echo ""
echo "== Looking for null coalescing with textures.Get =="
rg 'textures\.Get.*\?\?' --type cs -A 1 -B 1 | head -30Repository: winnerspiros/osu
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full DrawableAvatar.cs =="
cat -n "osu.Game/Users/Drawables/DrawableAvatar.cs"
echo ""
echo "== Full UserCoverBackground.cs =="
cat -n "osu.Game/Users/UserCoverBackground.cs"Repository: winnerspiros/osu
Length of output: 5637
Guard FlagUrl before texture lookup.
Line 50 calls textures.Get(team.FlagUrl) when team is not null, but FlagUrl is a nullable property (string?) that can be null or empty even when the team exists. This will cause a runtime error if passed to textures.Get(). Similar drawable classes in the codebase (e.g., DrawableAvatar) use null coalescing or null checks when handling similar nullable URL properties.
Suggested fix
[BackgroundDependencyLoader]
private void load(LargeTextureStore textures)
{
- if (team != null)
- sprite.Texture = textures.Get(team.FlagUrl);
+ string? flagUrl = team?.FlagUrl;
+ if (!string.IsNullOrEmpty(flagUrl))
+ sprite.Texture = textures.Get(flagUrl);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Users/Drawables/DrawableTeamFlag.cs` around lines 49 - 50, The code
calls textures.Get(team.FlagUrl) without guarding the nullable FlagUrl; update
DrawableTeamFlag to check the URL before lookup (e.g. use if
(!string.IsNullOrEmpty(team?.FlagUrl)) sprite.Texture =
textures.Get(team.FlagUrl); else sprite.Texture = null or a fallback texture) so
sprite.Texture is only set with a non-null, non-empty URL; reference the
sprite.Texture assignment and textures.Get(...) call in DrawableTeamFlag and the
team.FlagUrl property when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Chores