-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove RoomManager
, centralise listing management to lounge
#31866
Conversation
I believe once upon a time the `SelectedRoom` bindable used to be bound to `RoomManager.JoinedRoom` or similar. But now it's effectively private to the lounge subscreen and so a lease is unnecessary.
@@ -112,37 +107,5 @@ protected override void OpenNewRoom(Room room) | |||
|
|||
base.OpenNewRoom(room); | |||
} | |||
|
|||
private partial class MultiplayerListingPollingComponent : ListingPollingComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class was removed with no replacement. The general idea is that things generally handle fine if the API is online but the multiplayer client isn't connected.
It gets into this sort of state, where the create button is faded and trying to join the rooms has no effect:
We could potentially add an additional check for MultiplayerClient.IsConnected
to the main menu button with perhaps an error notification, but I didn't see that as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for now.
68670ee
to
446718f
Compare
Force-pushed out one commit fixing a broken test, turns out it's broken in master: #32215 |
No major issues with this PR, but I'd like to apply the following to scope down diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneLoungeRoomsContainer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneLoungeRoomsContainer.cs
index 772eb91174..b43433fe8d 100644
--- a/osu.Game.Tests/Visual/Multiplayer/TestSceneLoungeRoomsContainer.cs
+++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneLoungeRoomsContainer.cs
@@ -21,7 +21,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
public partial class TestSceneLoungeRoomsContainer : OnlinePlayTestScene
{
private BindableList<Room> rooms = null!;
- private RoomsContainer container = null!;
+ private RoomListing container = null!;
public override void SetUpSteps()
{
@@ -36,7 +36,7 @@ public override void SetUpSteps()
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Width = 0.5f,
- Child = container = new RoomsContainer
+ Child = container = new RoomListing
{
RelativeSizeAxes = Axes.Both,
Rooms = { BindTarget = rooms },
diff --git a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsLoungeSubScreen.cs b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsLoungeSubScreen.cs
index 35bf6dc28a..ceb3a32402 100644
--- a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsLoungeSubScreen.cs
+++ b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsLoungeSubScreen.cs
@@ -26,7 +26,7 @@ public override void SetUpSteps()
AddUntilStep("wait for present", () => loungeScreen.IsCurrentScreen());
}
- private RoomsContainer roomsContainer => loungeScreen.ChildrenOfType<RoomsContainer>().First();
+ private RoomListing roomListing => loungeScreen.ChildrenOfType<RoomListing>().First();
[Test]
public void TestManyRooms()
@@ -41,13 +41,13 @@ public void TestScrollByDraggingRooms()
createRooms(GenerateRooms(30));
- AddStep("move mouse to third room", () => InputManager.MoveMouseTo(roomsContainer.DrawableRooms[2]));
+ AddStep("move mouse to third room", () => InputManager.MoveMouseTo(roomListing.DrawableRooms[2]));
AddStep("hold down", () => InputManager.PressButton(MouseButton.Left));
- AddStep("drag to top", () => InputManager.MoveMouseTo(roomsContainer.DrawableRooms[0]));
+ AddStep("drag to top", () => InputManager.MoveMouseTo(roomListing.DrawableRooms[0]));
AddAssert("first and second room masked", ()
- => !checkRoomVisible(roomsContainer.DrawableRooms[0]) &&
- !checkRoomVisible(roomsContainer.DrawableRooms[1]));
+ => !checkRoomVisible(roomListing.DrawableRooms[0]) &&
+ !checkRoomVisible(roomListing.DrawableRooms[1]));
}
[Test]
@@ -55,10 +55,10 @@ public void TestScrollSelectedIntoView()
{
createRooms(GenerateRooms(30));
- AddStep("select last room", () => roomsContainer.DrawableRooms[^1].TriggerClick());
+ AddStep("select last room", () => roomListing.DrawableRooms[^1].TriggerClick());
- AddUntilStep("first room is masked", () => !checkRoomVisible(roomsContainer.DrawableRooms[0]));
- AddUntilStep("last room is not masked", () => checkRoomVisible(roomsContainer.DrawableRooms[^1]));
+ AddUntilStep("first room is masked", () => !checkRoomVisible(roomListing.DrawableRooms[0]));
+ AddUntilStep("last room is not masked", () => checkRoomVisible(roomListing.DrawableRooms[^1]));
}
private bool checkRoomVisible(DrawableRoom room) =>
diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs
index 207e0bdf55..c9d8365852 100644
--- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs
+++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs
@@ -204,7 +204,7 @@ private void scrollToSelection()
ScrollContainer.ScrollIntoView(drawableItem);
}
- #region Key selection logic (shared with BeatmapCarousel and RoomsContainer)
+ #region Key selection logic (shared with BeatmapCarousel and RoomListing)
public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
{
diff --git a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomListing.cs
similarity index 91%
rename from osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs
rename to osu.Game/Screens/OnlinePlay/Lounge/Components/RoomListing.cs
index 65f969bc7b..1c3db87aaf 100644
--- a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs
+++ b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomListing.cs
@@ -21,12 +21,25 @@
namespace osu.Game.Screens.OnlinePlay.Lounge.Components
{
- public partial class RoomsContainer : CompositeDrawable, IKeyBindingHandler<GlobalAction>
+ public partial class RoomListing : CompositeDrawable, IKeyBindingHandler<GlobalAction>
{
+ /// <summary>
+ /// Rooms which should be displayed. Should be managed externally.
+ /// </summary>
public readonly BindableList<Room> Rooms = new BindableList<Room>();
- public readonly Bindable<Room?> SelectedRoom = new Bindable<Room?>();
+
+ /// <summary>
+ /// The current filter criteria. Should be managed externally.
+ /// </summary>
public readonly Bindable<FilterCriteria?> Filter = new Bindable<FilterCriteria?>();
+ /// <summary>
+ /// The currently user-selected room.
+ /// </summary>
+ public IBindable<Room?> SelectedRoom => selectedRoom;
+
+ private readonly Bindable<Room?> selectedRoom = new Bindable<Room?>();
+
public IReadOnlyList<DrawableRoom> DrawableRooms => roomFlow.FlowingChildren.Cast<DrawableRoom>().ToArray();
private readonly ScrollContainer<Drawable> scroll;
@@ -35,7 +48,7 @@ public partial class RoomsContainer : CompositeDrawable, IKeyBindingHandler<Glob
// handle deselection
public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => true;
- public RoomsContainer()
+ public RoomListing()
{
InternalChild = scroll = new OsuScrollContainer
{
@@ -158,7 +171,7 @@ private void addRooms(IEnumerable<Room> rooms)
{
foreach (var room in rooms)
{
- var drawableRoom = new DrawableLoungeRoom(room) { SelectedRoom = SelectedRoom };
+ var drawableRoom = new DrawableLoungeRoom(room) { SelectedRoom = selectedRoom };
roomFlow.Add(drawableRoom);
@@ -177,7 +190,7 @@ private void removeRooms(IEnumerable<Room> rooms)
// selection may have a lease due to being in a sub screen.
if (SelectedRoom.Value == r && !SelectedRoom.Disabled)
- SelectedRoom.Value = null;
+ selectedRoom.Value = null;
}
}
@@ -187,13 +200,13 @@ private void clearRooms()
// selection may have a lease due to being in a sub screen.
if (!SelectedRoom.Disabled)
- SelectedRoom.Value = null;
+ selectedRoom.Value = null;
}
protected override bool OnClick(ClickEvent e)
{
if (!SelectedRoom.Disabled)
- SelectedRoom.Value = null;
+ selectedRoom.Value = null;
return base.OnClick(e);
}
@@ -240,7 +253,7 @@ private void selectNext(int direction)
// we already have a valid selection only change selection if we still have a room to switch to.
if (room != null)
- SelectedRoom.Value = room;
+ selectedRoom.Value = room;
}
#endregion
diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs
index 12c0bb12e2..c1c65a744a 100644
--- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs
+++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs
@@ -38,7 +38,7 @@ public abstract partial class LoungeSubScreen : OnlinePlaySubScreen, IOnlinePlay
protected override BackgroundScreen CreateBackground() => new LoungeBackgroundScreen
{
- SelectedRoom = { BindTarget = roomsContainer.SelectedRoom }
+ SelectedRoom = { BindTarget = roomListing.SelectedRoom }
};
protected override UserActivity InitialActivity => new UserActivity.SearchingForLobby();
@@ -74,7 +74,7 @@ public abstract partial class LoungeSubScreen : OnlinePlaySubScreen, IOnlinePlay
private readonly Bindable<bool> hasListingResults = new Bindable<bool>();
private readonly IBindable<bool> operationInProgress = new Bindable<bool>();
private readonly IBindable<bool> isIdle = new BindableBool();
- private RoomsContainer roomsContainer = null!;
+ private RoomListing roomListing = null!;
private LoungeListingPoller listingPoller = null!;
private PopoverContainer popoverContainer = null!;
private LoadingLayer loadingLayer = null!;
@@ -106,7 +106,7 @@ private void load()
Horizontal = WaveOverlayContainer.WIDTH_PADDING,
Top = Header.HEIGHT + controls_area_height + 20,
},
- Child = roomsContainer = new RoomsContainer
+ Child = roomListing = new RoomListing
{
RelativeSizeAxes = Axes.Both,
Filter = { BindTarget = filter },
@@ -185,7 +185,7 @@ protected override void LoadComplete()
filter.BindValueChanged(_ =>
{
- roomsContainer.Rooms.Clear();
+ roomListing.Rooms.Clear();
hasListingResults.Value = false;
listingPoller.PollImmediately();
});
@@ -195,11 +195,11 @@ protected override void LoadComplete()
private void onListingReceived(Room[] result)
{
- Dictionary<long, Room> localRoomsById = roomsContainer.Rooms.ToDictionary(r => r.RoomID!.Value);
+ Dictionary<long, Room> localRoomsById = roomListing.Rooms.ToDictionary(r => r.RoomID!.Value);
Dictionary<long, Room> resultRoomsById = result.ToDictionary(r => r.RoomID!.Value);
// Remove all local rooms no longer in the result set.
- roomsContainer.Rooms.RemoveAll(r => !resultRoomsById.ContainsKey(r.RoomID!.Value));
+ roomListing.Rooms.RemoveAll(r => !resultRoomsById.ContainsKey(r.RoomID!.Value));
// Add or update local rooms with the result set.
foreach (var r in result)
@@ -207,7 +207,7 @@ private void onListingReceived(Room[] result)
if (localRoomsById.TryGetValue(r.RoomID!.Value, out Room? existingRoom))
existingRoom.CopyFrom(r);
else
- roomsContainer.Rooms.Add(r);
+ roomListing.Rooms.Add(r);
}
hasListingResults.Value = true;
Can we apply that here or is it going to be conflict-central for futher PRs? |
So basically changing from |
and the rename of |
RoomManager
class)This is the removal of
RoomManager
that I hinted towards in the PR above. I would also like to removeTestRoomRequestsHandler
to simplify further, but I don't think it's immediately necessary.Of particular importance:
RoomsContainer
).Room
directly with its associatedPlaylistsRoomUpdater
.RoomManager
used to have a method to ignore rooms that threw an exception while updating. This is a legacy thing that I think we should no longer keep around.