Show count of beatmaps in collections in manage dialog #22932
Conversation
| Colour = colours.Yellow | ||
| }); | ||
|
|
||
| itemCountSubscription = realm.SubscribeToPropertyChanged(r => r.Find<BeatmapCollection>(collection.ID), c => c.BeatmapMD5Hashes, _ => |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Is the supposed performance concern still blocking this? I thought the flow was pushing stuff until someone complained.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
| // 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"; |
There was a problem hiding this comment.
As discussed in #23000, "beatmaps" is incorrect here. I would suggest "items" if we going with the same direction here.
Touched on in #22930