Show count of beatmaps in collections in manage dialog

[peppy]

Touched on in #22930

[bdach]

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.

[peppy]

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).

[Joehuu]

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

[bdach]

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.

[bdach]

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...

[bdach]

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.

[Joehuu]

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

[bdach]

I've wanted to revive this for a while now that realm has become unstuck. In trying to apply the realm team guidance linked above, I realised that... there is actually no need to add any extra subscriptions to make this work over what's already in master. Thus, my humble proposal - along with the explanation as to why no extra subscriptions are required - is 884fa20.

Additionally, I removed the contentious 'beatmaps' terminology in d8b3c28.

@peppy is this something you'd be happy with? I can't imagine having (count of user collections) less active at all times for literally free to be a bad thing, but I figure I should double-check.

[peppy]

I guess this does work for now yeah.

Although it also shows items as including items which aren't present in the local database (in other words not downloaded). Do you think we should somehow show this, something like "12 items (10 downloaded)"? Or maybe "10 items (+2 not downloaded)"?

Or do we just turn a blind eye?

2025-02-11.19.20.11.mp4

[bdach]

Do you think we should somehow show this, something like "12 items (10 downloaded)"? Or maybe "10 items (+2 not downloaded)"?

I'm a little worried that query will kill things performance-wise, although BeatmapInfo.MD5Hash is [Indexed]... I'm sorta tempted to just let this slide until someone notices, but I guess we could try and see if it kills performance.

[peppy]

I'm also fine with leaving it until someone brings it up, but I think that might happen quite fast (it was the first thing I noticed during testing)

[bdach]

Yeah I'd just merge at this point... except I'm a bit more worried about the test failures here.

[peppy]

I believe this would fix it

diff --git a/osu.Game/Collections/DrawableCollectionListItem.cs b/osu.Game/Collections/DrawableCollectionListItem.cs
index f2b00004e2..b0dd70227c 100644
--- a/osu.Game/Collections/DrawableCollectionListItem.cs
+++ b/osu.Game/Collections/DrawableCollectionListItem.cs
@@ -255,7 +255,7 @@ protected override bool OnClick(ClickEvent e)
             private void deleteCollection() => collection.PerformWrite(c => c.Realm!.Remove(c));
         }
 
-        public IEnumerable<LocalisableString> FilterTerms => [(LocalisableString)Model.Value.Name];
+        public IEnumerable<LocalisableString> FilterTerms => Model.PerformRead(m => m.IsValid ? new[] { (LocalisableString)m.Name } : []);
 
         private bool matchingFilter = true;
 

tl;dr because collections aren't soft delete, we need to check on every access. doubt it's related to this PR, just very good luck?

[bdach]

Yeah my conclusion is similar. Unless something horrendous happens on CI I'll just merge this.