Improve handling of beatmap collection changes in CollectionDropdown

[frenzibyte]

• Depends on Add proper handling for BindableList collection changes in dropdown osu-framework#6061
• Depends on framework bump
• Closes Collections dropdown shouldn't scroll to top when adding a beatmap to a collection #24315

Preview:

CleanShot.2023-11-25.at.02.37.38-converted.mp4

[bdach]

I do not know how to fix the failing tests here.

They are failing because they were relying on drawable menu items being iterated through and returned by .ChildrenOfType<> in the same order as they are displayed. Due to the usage of {Get,Set}LayoutPosition() framework-side to reorder the items, this is not the case anymore.

The following abomination "fixes the test":

diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs
index 00a0d4a849..f4c39af297 100644
--- a/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs
+++ b/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs
@@ -250,6 +250,11 @@ private IconButton getAddOrRemoveButton(int index)
         });
 
         private IEnumerable<Dropdown<CollectionFilterMenuItem>.DropdownMenu.DrawableDropdownMenuItem> getCollectionDropdownItems()
-            => control.ChildrenOfType<CollectionDropdown>().Single().ChildrenOfType<Dropdown<CollectionFilterMenuItem>.DropdownMenu.DrawableDropdownMenuItem>();
+        {
+            var dropdown = control.ChildrenOfType<CollectionDropdown>().Single();
+            var dropdownMenu = dropdown.ChildrenOfType<SearchContainer<Menu.DrawableMenuItem>>().Single();
+            return dropdown.ChildrenOfType<Dropdown<CollectionFilterMenuItem>.DropdownMenu.DrawableDropdownMenuItem>()
+                           .OrderBy(drawableItem => dropdownMenu.GetLayoutPosition(drawableItem));
+        }
     }
 }

but I will not be caught alive committing such atrocities to source, even if for test purposes. To state that this violates encapsulation would be a understatement the size of Jupiter.

I am rather disappointed that this was apparently either not caught at development time, or not signposted and warned against appropriately. I would consider running tests that cover the specific modified dropdown to be a bare minimum.

@frenzibyte Any bright ideas?

[frenzibyte]

I do not believe this is easily resolvable framework-side. It is expected behaviour from the lack of inserting a drawable between a container's children.

Given that specific code snippet, I would've queried for FillFlowContainer<DrawableMenuItem> and used the FlowingChildren property. But that being said, Menu already exposes a Children property, and that is not ordered by layout currently, so... I'll push framework-side changes accordingly.

EDIT: Menu does expose Children, but it's protected.

[bdach]

I do not believe this is easily resolvable framework-side. (...) I'll push framework-side changes accordingly.

Which is it? And what would the changes be?

If it's going to be sorting Children in any way, then I dunno. I might just prefer the other thing instead, namely

Given that specific code snippet, I would've queried for FillFlowContainer<DrawableMenuItem> and used the FlowingChildren property.

[frenzibyte]

Please disregard what I said about making a framework-side change, I misunderstood your original comment/intent. The FlowingChildren method should do for now until something major comes off.

[bdach]

I wrote this

        private IEnumerable<Dropdown<CollectionFilterMenuItem>.DropdownMenu.DrawableDropdownMenuItem> getCollectionDropdownItems()
            => control.ChildrenOfType<CollectionDropdown>().Single().ChildrenOfType<SearchContainer<Menu.DrawableMenuItem>>().Single().FlowingChildren
                      .Cast<Dropdown<CollectionFilterMenuItem>.DropdownMenu.DrawableDropdownMenuItem>();

and it still fails 4 tests while my original workaround above fails none.

I'll leave it to you to figure out as I do not wish to spend any more time on this at present.

[frenzibyte]

Tests should be fixed with the above change. However, it contains a work around for an issue that is resolved by ppy/osu-framework#6079. I've made it a workaround as to not require an extra framework release, but the workaround can be reverted by just following the comment there and use Items instead of ItemsSource (once the mentioned PR is merged).