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

Drawable multiplayer room #824

Merged
merged 26 commits into from May 25, 2017

Conversation

3 participants
@DrabWeb
Contributor

DrabWeb commented May 22, 2017

screen shot 2017-05-22 at 1 22 08 am

Based off of and supersedes #442

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 22, 2017

Member

Let's rename this to DrawableRoom. Currently quite a mouthful.

Member

peppy commented May 22, 2017

Let's rename this to DrawableRoom. Currently quite a mouthful.

Show outdated Hide outdated osu.Desktop.VisualTests/Tests/TestCaseDrawableMultiplayerRoom.cs
AddStep(@"change state", () =>
{
p.Room.Value.Status = MultiplayerRoomStatus.Playing;
p.Room.TriggerChange();

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

this is generally not how bindables should be used. In this case, each property of a room would be exposed as a bindable, and we would be binding to each one we were interested in (then applying changes for that bindable, or just doing a full update if that's easier).

@peppy

peppy May 22, 2017

Member

this is generally not how bindables should be used. In this case, each property of a room would be exposed as a bindable, and we would be binding to each one we were interested in (then applying changes for that bindable, or just doing a full update if that's easier).

Show outdated Hide outdated osu.Game/Online/Multiplayer/MultiplayerRoom.cs
public string Name { get; set; }
public User Host { get; set; }
public MultiplayerRoomStatus Status { get; set; }
public BeatmapMetadata CurrentBeatmap { get; set; }

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

i'd just call this Beatmap

@peppy

peppy May 22, 2017

Member

i'd just call this Beatmap

Show outdated Hide outdated osu.Game/Online/Multiplayer/MultiplayerRoom.cs
namespace osu.Game.Online.Multiplayer
{
public class MultiplayerRoom

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

Let's go with "Room"

@peppy

peppy May 22, 2017

Member

Let's go with "Room"

Show outdated Hide outdated osu.Game/Online/Multiplayer/MultiplayerRoom.cs
{
public string Name { get; set; }
public User Host { get; set; }
public MultiplayerRoomStatus Status { get; set; }

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

RoomStatus etc.

@peppy

peppy May 22, 2017

Member

RoomStatus etc.

},
};
Room.Name.ValueChanged += displayName;

This comment has been minimized.

@peppy

peppy May 24, 2017

Member

Directly binding to bindables of other objects is generally frowned upon. For now it should be okay because this is a view of a model, but at very least you need to unbind these events in a dispose override

@peppy

peppy May 24, 2017

Member

Directly binding to bindables of other objects is generally frowned upon. For now it should be okay because this is a view of a model, but at very least you need to unbind these events in a dispose override

@peppy

as mentioned

@peppy

peppy approved these changes May 25, 2017

@peppy peppy merged commit 95c03b7 into ppy:master May 25, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment