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

Move beatmap collections to realm #19430

Merged
merged 18 commits into from
Jul 28, 2022
Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Jul 28, 2022

Sorry for the size of this diff. Collections were written in a very convoluted way and I ended up having to touch basically everything they touched. I'd recommend ignoring the diff where it's overwhelming and checking the files as if they are new implementations.

  • To simplify, I've removed a lot of bindable flow. This means that some UI elements are doing full refreshes rather than partial updates (ie. when a collection is renamed). We can add such things back if required, but it doesn't feel too bad for now, even with tens of collections. And it's a rare operation.
  • Still using hashes, reasoning is mentioned inline https://github.com/ppy/osu/compare/master...peppy:osu:realm-collections?expand=1#diff-862af16e9f3547129791348268ee0b26e35bf8b8df70999b03cc9b0b3e354b6eR29-R36. I'm open to migrating to beatmap references in a future if that's seen as something we need.
  • Realm version bump isn't actually required for adding new models, but I did bump to (ab)use the migration flow to handle migrating away from the local collection.db.
  • Fixed some weird edge cases along the way, such as song select re-filtering when clicking the "Manage collections" item in the filter dropdown.
  • The structure of the dropdown and its menu items was pretty bad, and still is. I think this needs a re-think at the framework side to resolve.
  • I've left the "manage collections" dialog allowing ordering, but this isn't currently saved out. I figure people will complain if they have been actually using the rearrangeability, else we can consider removing it in the future.
    This is a somewhat necessary precursor to having the import-update flow handle collections properly.

@peppy peppy added the realm deals with local realm database label Jul 28, 2022
@smoogipoo
Copy link
Contributor

Right clicking on playlist panels in multiplayer crashes with:

System.NotSupportedException: The method 'Cast' is not supported
   at Realms.RealmResultsVisitor.VisitMethodCall(MethodCallExpression node)
   at Realms.RealmResults`1.GetOrCreateHandle()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Realms.RealmCollectionBase`1.get_Count()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at osu.Game.Screens.OnlinePlay.DrawableRoomPlaylistItem.get_ContextMenuItems() in /home/smgi/Repos/osu/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs:line 502
   at osu.Framework.Graphics.Cursor.ContextMenuContainer.<>c.<OnMouseDown>b__9_0(IHasContextMenu t)
   at System.Linq.Enumerable.SelectListIterator`2.MoveNext()
   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
   at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
   at osu.Framework.Graphics.Cursor.ContextMenuContainer.OnMouseDown(MouseDownEvent e)
   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)

@peppy
Copy link
Member Author

peppy commented Jul 28, 2022

Right clicking on playlist panels in multiplayer crashes with:

Should be fixed. A case of copy-paste implementations (not new in this PR) and only applying a fix in one of two places 😓 .

smoogipoo
smoogipoo previously approved these changes Jul 28, 2022
smoogipoo
smoogipoo previously approved these changes Jul 28, 2022
@smoogipoo smoogipoo merged commit 5003eb5 into ppy:master Jul 28, 2022
@peppy peppy deleted the realm-collections branch July 31, 2022 07:00
This was referenced Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
realm deals with local realm database size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants