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.

Sign up for GitHub

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

Show count of beatmaps in collections in manage dialog #22932

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Show count of beatmaps in collections in manage dialog #22932

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63e0768
Show count of beatmaps in collections in manage dialog
peppy Mar 22, 2023
954be12
Debounce updates to ensure event isn't fired too often after much col…
peppy Mar 22, 2023
01e489e
Merge branch 'master' into collection-counts
peppy Apr 3, 2023
2567891
Remove redundant type specification
peppy Apr 3, 2023
b99dc26
Merge branch 'master' into collection-counts
bdach Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 61 additions & 6 deletions osu.Game/Collections/DrawableCollectionListItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using osu.Game.Database;
using osu.Game.Graphics;
using osu.Game.Graphics.Containers;
using osu.Game.Graphics.Sprites;
using osu.Game.Graphics.UserInterface;
using osu.Game.Overlays;
using osuTK;
Expand All @@ -25,7 +26,7 @@ namespace osu.Game.Collections
/// </summary>
public partial class DrawableCollectionListItem : OsuRearrangeableListItem<Live<BeatmapCollection>>
{
private const float item_height = 35;
private const float item_height = 45;
private const float button_width = item_height * 0.75f;

/// <summary>
Expand Down Expand Up @@ -87,13 +88,11 @@ private void load()
Padding = new MarginPadding { Right = collection.IsManaged ? button_width : 0 },
Children = new Drawable[]
{
textBox = new ItemTextBox
textBox = new ItemTextBox(collection)
{
RelativeSizeAxes = Axes.Both,
Size = Vector2.One,
CornerRadius = item_height / 2,
RelativeSizeAxes = Axes.X,
Height = item_height,
CommitOnFocusLost = true,
PlaceholderText = collection.IsManaged ? string.Empty : "Create a new collection"
},
}
},
Expand Down Expand Up @@ -124,11 +123,67 @@ private partial class ItemTextBox : OsuTextBox
{
protected override float LeftRightPadding => item_height / 2;

private const float count_text_size = 12;

[Resolved]
private RealmAccess realm { get; set; } = null!;

private readonly Live<BeatmapCollection> collection;

private OsuSpriteText countText = null!;

private IDisposable? itemCountSubscription;

public ItemTextBox(Live<BeatmapCollection> collection)
{
this.collection = collection;

CornerRadius = item_height / 2;
}

[BackgroundDependencyLoader]
private void load(OsuColour colours)
{
BackgroundUnfocused = colours.GreySeaFoamDarker.Darken(0.5f);
BackgroundFocused = colours.GreySeaFoam;

if (collection.IsManaged)
{
TextContainer.Height *= (Height - count_text_size) / Height;
TextContainer.Margin = new MarginPadding { Bottom = count_text_size };

TextContainer.Add(countText = new OsuSpriteText
{
Anchor = Anchor.BottomLeft,
Origin = Anchor.TopLeft,
Depth = float.MinValue,
Font = OsuFont.Default.With(size: count_text_size, weight: FontWeight.SemiBold),
Margin = new MarginPadding { Top = 2, Left = 2 },
Colour = colours.Yellow
});

itemCountSubscription = realm.SubscribeToPropertyChanged(r => r.Find<BeatmapCollection>(collection.ID), c => c.BeatmapMD5Hashes, _ =>
Copy link
Collaborator

@bdach bdach Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is instant alarm bells for me. One subscription for each item on the list? So potentially N subscriptions for N items? There's no pooling or list virtualisation anywhere here, so that means one subscription for every collection in db. The collections dialog is also registered at OsuGame level, which means that the disposal of the subscription isn't going to save us any as it just doesn't fire until the game is closed.

I fear this is going to fall over badly for power users with monstrous beatmap collections.

  • Do we need this to be a subscription, even? The manage collections dialog is modal. There's not really any way for the counts to change while it's open except for background imports - but maybe it's fine not to support that?
  • If we do need this to be a subscription, it should probably be handled a lot more economically. Maybe by eagerly acquiring on show and eagerly disposing on hide. Or by pooling/DLUW.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be pretty efficient with realm/realm-dotnet#3121, which is why I went for this.

I'd be more interested in moving it once we see it having an adverse effect. And definitely not by pooling (maybe one subscription at the dialog level, although that may have higher overhead due to no keypath filtering).

Do we need this to be a subscription, even? The manage collections dialog is modal. There's not really any way for the counts to change while it's open except for background imports - but maybe it's fine not to support that?

It's modal, but unless the counts are refreshed each time it's shown, there's plenty of other ways the counts can change (right click menu at song select; collections dropdown at song select).

Copy link
Member

Choose a reason for hiding this comment

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

Is the supposed performance concern still blocking this? I thought the flow was pushing stuff until someone complained.

Copy link
Collaborator

@bdach bdach Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not merging without testing. Someone linked a database with a bunch of huge collections so I might give this a try using that and see how hard it dies.

I thought the flow was pushing stuff until someone complained.

This is not how I operate. If there's a clear concern at review time I don't see why we would "push it anyway". That would make my reviewing a lot easier but also useless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well I tested using database from #27322 (comment), which has ~80 collections. Opening the collections dialog increases the number of active subscriptions five-fold. I didn't see signs of falling over, surprisingly - even on mutations - but as predicted the subscriptions stay alive for the entirety of the remaining gameplay and the only thing that gets rid of them is deleting the collections. And 80 is probably not even that many collections.

I dunno, I'm still too skeptical to feel confident pushing this to users. It just feels way too shoddy to be keeping these subscriptions around when the dialog isn't even visible...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I asked about this in a discussion and the realm team guidance is that this might be fine but can be improved upon, by either only watching for changes when the dialog is open, or using that new feature mentioned there and only ever using a single subscription in total.

Scheduler.AddOnce(() =>
{
int count = collection.PerformRead(c => c.BeatmapMD5Hashes.Count);

countText.Text = count == 1
// Intentionally not localised until we have proper support for this (see https://github.com/ppy/osu-framework/pull/4918
// but also in this case we want support for formatting a number within a string).
? $"{count:#,0} beatmap"
: $"{count:#,0} beatmaps";
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in #23000, "beatmaps" is incorrect here. I would suggest "items" if we going with the same direction here.

}));
}
else
{
PlaceholderText = "Create a new collection";
}
}

protected override void Dispose(bool isDisposing)
{
base.Dispose(isDisposing);
itemCountSubscription?.Dispose();
}
}

Expand Down
Loading