Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add owner avatar to multiplayer playlist items #15801

Merged
merged 9 commits into from
Nov 26, 2021

Conversation

smoogipoo
Copy link
Contributor

2021-11-25.23-27-07.mp4

room.Playlist[i].ID = currentPlaylistItemId++;
room.Playlist[i].OwnerID = room.Host.Value.OnlineID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As evidenced by the test failures, it appears some tests create rooms without a host.

@@ -191,12 +203,21 @@ protected override Drawable CreateContent()
{
new Drawable[]
{
ownerAvatar = new OwnerAvatar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding the avatar at this level means that it will also appear in playlists with the room creator as the owner of all playlist items, which feels unintended / somewhat of a waste of space:

osu_2021-11-25_20-34-47

Are there any plans regarding that going forward, or is that just an oversight?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Feels like it definitely shouldn't be present in all but the multiplayer queue context.

});
}

public LocalisableString TooltipText => User == null ? "loading user..." : $"queued by {User.Username}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "loading user" text appears incorrectly in some contexts. For instance, the screenshot below was taken during creation of a new multiplayer room:

osu_2021-11-25_20-39-46

@bdach
Copy link
Collaborator

bdach commented Nov 25, 2021

Also, not to put too much of a point on what is probably mostly a temporary design, but I'm a bit weirded out by the avatars being on the left. It feels like they are not nearly important enough information to be the the first thing on a playlist item. Maybe they would fare better as the rightmost thing on the item?

2021-11-25-204922_685x184_scrot

@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 26, 2021
@smoogipoo
Copy link
Contributor Author

All above issues should be fixed. Avatars will also only be displayed in the subscreen's playlist and no longer in Playlists or the settings overlay.

Should probably be replaced with a loading spinner in the future, don't
really like "loading" tooltips.
peppy
peppy previously approved these changes Nov 26, 2021
Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Seems fine for an initial release

@peppy peppy merged commit d570054 into ppy:master Nov 26, 2021
@smoogipoo smoogipoo deleted the playlist-item-add-owner branch September 11, 2023 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants