-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix playlist room status resetting on enter #30805
Conversation
@@ -264,6 +265,18 @@ public RoomAvailability Availability | |||
set => SetField(ref availability, value); | |||
} | |||
|
|||
[OnDeserialized] | |||
private void onDeserialised(StreamingContext context) |
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.
Not 100% on fixing this using newtonsoft magic. For one thing, I was surprised that this did anything for unit testing, upon which I stumbled upon
osu/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs
Lines 288 to 294 in 2c0140f
private Room cloneRoom(Room source) | |
{ | |
var result = JsonConvert.DeserializeObject<Room>(JsonConvert.SerializeObject(source)); | |
Debug.Assert(result != null); | |
// When serialising, only beatmap IDs are sent to the server. | |
// When deserialising, full beatmaps and IDs are expected to arrive. |
and then
osu/osu.Game/Online/Rooms/PlaylistItem.cs
Lines 46 to 68 in 2c0140f
/// <summary> | |
/// Used for deserialising from the API. | |
/// </summary> | |
[JsonProperty("beatmap")] | |
private APIBeatmap apiBeatmap | |
{ | |
// This getter is required/used internally by JSON.NET during deserialisation to do default-value comparisons. It is never used during serialisation (see: ShouldSerializeapiBeatmap()). | |
// It will always return a null value on deserialisation, which JSON.NET will handle gracefully. | |
get => (APIBeatmap)Beatmap; | |
set => Beatmap = value; | |
} | |
/// <summary> | |
/// Used for serialising to the API. | |
/// </summary> | |
[JsonProperty("beatmap_id")] | |
private int onlineBeatmapId | |
{ | |
get => Beatmap.OnlineID; | |
// This setter is only required for client-side serialise-then-deserialise operations. | |
// Serialisation is supposed to emit only a `beatmap_id`, but a (non-null) `beatmap` is required on deserialise. | |
set => Beatmap = new APIBeatmap { OnlineID = value }; | |
} |
upon reading which I concluded that maybe this is comparably not that bad anymore and at least it has test coverage so I'm prolly gonna look away...
Fixes #30792
The issue is that
Room.Status
is populated after deserialisation only byGetRoomsRequest
(in the lounge) and not byGetRoomRequest
(in the room). This PR applies a direct fix by moving the post-processing to a method that's called when any JSON deserialisation takes place, covering both requests.It's not the best possible fix because there's probably no realistic use for this entire thing to be a class in the first place, as opposed to for example a helper method, enum, or even being applied in the existing property setters. But I don't consider this an important thing to change at the current moment.