Implement dropdown searching #6072

frenzibyte · 2023-12-02T19:36:23Z

Framework-side implementation for Add dropdown searching support osu#21784

RFC. The implementation is a bit heavy, commit-by-commit reviewing is advised.

There are a few hacks with this implementation, mainly the use of a local input manager to allow the search text box to be focused while the menu is focused as well. I tried not to pay too much attention to the details of this implementation to not waste time on it.

CleanShot.2023-12-02.at.22.31.18.mp4

bdach · 2023-12-04T20:13:29Z

osu.Framework/Graphics/UserInterface/TextBox.cs

+        /// <summary>
+        /// A fixed size for the text displayed in this <see cref="TextBox"/>. If left unset, text size will be computed based on the dimensions of the <see cref="TextBox"/>.
+        /// </summary>
+        public float TextSize
+        {
+            init => customTextSize = value;
+        }
+
+        protected float DisplayedTextSize => customTextSize ?? (TextFlow.DrawSize.Y - (TextFlow.Padding.Top + TextFlow.Padding.Bottom));


Is there any reason this is this weird? Why is the public member init-only when it looks like it would work when set at any time? Can the "displayed" text size just be plainly publically gettable? And can we name this FontSize?

Making it non-init means adding extra logic to handle updating font size of existing characters, so better keep it init-only.

bdach · 2023-12-04T20:17:03Z

osu.Framework/Graphics/UserInterface/Menu.cs

@@ -912,7 +942,7 @@ protected override bool OnClick(ClickEvent e)

        #endregion

-        private partial class ItemsFlow : FillFlowContainer<DrawableMenuItem>
+        private partial class ItemsFlow : SearchContainer<DrawableMenuItem>


This is somewhat far-reaching...? Every single menu out there is now searchable? Not sure what the performance implications of this are, have you checked?

Moved locally to DropdownMenu.

bdach · 2023-12-04T20:17:45Z

osu.Framework/Graphics/UserInterface/Menu.cs

+        /// <summary>
+        /// Search terms to filter items displayed in this menu.
+        /// </summary>
+        public string SearchTerm
+        {
+            get => itemsFlow.SearchTerm;
+            set => itemsFlow.SearchTerm = value;
+        }
+
+        public bool AllowNonContiguousMatching
+        {
+            get => itemsFlow.AllowNonContiguousMatching;
+            set => itemsFlow.AllowNonContiguousMatching = value;
+        }
+
+        public event Action FilterCompleted
+        {
+            add => itemsFlow.FilterCompleted += value;
+            remove => itemsFlow.FilterCompleted -= value;
+        }


I don't know about this either... Very far-reaching API surface for this singular feature.

Moved locally to DropdownMenu.

bdach · 2023-12-04T20:18:46Z

osu.Framework/Graphics/UserInterface/Menu.cs

        protected override bool OnKeyDown(KeyDownEvent e)
        {
-            if (e.Key == Key.Escape && !TopLevelMenu)
+            if (e.Key == Key.Escape && CloseOnEscape)


Can we not add this virtual? Can this be handled more locally to the dropdowns somehow?

Yes, but it felt odd to handle locally in a general component like Dropdown, as opposed to modify Menu. Here's the diff:

diff --git a/osu.Framework/Graphics/UserInterface/Dropdown.cs b/osu.Framework/Graphics/UserInterface/Dropdown.cs index 585c97bbf..d93c1af9c 100644 --- a/osu.Framework/Graphics/UserInterface/Dropdown.cs +++ b/osu.Framework/Graphics/UserInterface/Dropdown.cs @@ -609,55 +609,56 @@ protected override bool OnHover(HoverEvent e) #endregion - // we'll handle closing the menu in Dropdown instead, - // since a search bar may be active and we want to reset it rather than closing the menu. - protected override bool CloseOnEscape => false; - protected override bool OnKeyDown(KeyDownEvent e) { var visibleMenuItemsList = VisibleMenuItems.ToList(); - if (!visibleMenuItemsList.Any()) - return base.OnKeyDown(e); - - var currentPreselected = PreselectedItem; - int targetPreselectionIndex = visibleMenuItemsList.IndexOf(currentPreselected); - switch (e.Key) + if (visibleMenuItemsList.Any()) { - case Key.Up: - PreselectItem(targetPreselectionIndex - 1); - return true; - - case Key.Down: - PreselectItem(targetPreselectionIndex + 1); - return true; - - case Key.PageUp: - var firstVisibleItem = VisibleMenuItems.First(); - - if (currentPreselected == firstVisibleItem) - PreselectItem(targetPreselectionIndex - VisibleMenuItems.Count()); - else - PreselectItem(visibleMenuItemsList.IndexOf(firstVisibleItem)); - return true; - - case Key.PageDown: - var lastVisibleItem = VisibleMenuItems.Last(); + var currentPreselected = PreselectedItem; + int targetPreselectionIndex = visibleMenuItemsList.IndexOf(currentPreselected); - if (currentPreselected == lastVisibleItem) - PreselectItem(targetPreselectionIndex + VisibleMenuItems.Count()); - else - PreselectItem(visibleMenuItemsList.IndexOf(lastVisibleItem)); - return true; + switch (e.Key) + { + case Key.Up: + PreselectItem(targetPreselectionIndex - 1); + return true; + + case Key.Down: + PreselectItem(targetPreselectionIndex + 1); + return true; + + case Key.PageUp: + var firstVisibleItem = VisibleMenuItems.First(); + + if (currentPreselected == firstVisibleItem) + PreselectItem(targetPreselectionIndex - VisibleMenuItems.Count()); + else + PreselectItem(visibleMenuItemsList.IndexOf(firstVisibleItem)); + return true; + + case Key.PageDown: + var lastVisibleItem = VisibleMenuItems.Last(); + + if (currentPreselected == lastVisibleItem) + PreselectItem(targetPreselectionIndex + VisibleMenuItems.Count()); + else + PreselectItem(visibleMenuItemsList.IndexOf(lastVisibleItem)); + return true; + + case Key.Enter: + var preselectedItem = VisibleMenuItems.ElementAt(targetPreselectionIndex); + PreselectionConfirmed?.Invoke((DropdownMenuItem<T>)preselectedItem.Item); + return true; + } + } - case Key.Enter: - var preselectedItem = VisibleMenuItems.ElementAt(targetPreselectionIndex); - PreselectionConfirmed?.Invoke((DropdownMenuItem<T>)preselectedItem.Item); - return true; + // we'll handle closing the menu in Dropdown instead, + // since a search bar may be active and we want to reset it rather than closing the menu. + if (e.Key == Key.Escape) + return false; - default: - return base.OnKeyDown(e); - } + return base.OnKeyDown(e); } public bool OnPressed(KeyBindingPressEvent<PlatformAction> e)

If you find it more acceptable this way, I'll apply accordingly.

For the sake of moving forward, I applied this.

bdach · 2023-12-04T20:20:54Z

osu.Framework/Graphics/UserInterface/Menu.cs

+            public abstract bool MatchingFilter { get; set; }
+
+            public abstract bool FilteringActive { set; }


This is an API break. Consumer classes will have to implement this. Even if they won't / can't do anything with it.

I'd argue we should avoid that if possible.

Moved to DropdownMenu and changed to a singular virtual method.

bdach · 2023-12-04T20:24:50Z

osu.Framework/Graphics/UserInterface/DropdownHeader.cs

+        public bool AlwaysShowSearchBar
+        {
+            get => SearchBar.AlwaysDisplayOnFocus;
+            set => SearchBar.AlwaysDisplayOnFocus = value;
+        }


AlwaysShowSearchBar, AlwaysDisplayOnFocus... can we pick one naming convention and stick with it?

AlwaysShowSearchBar is the general name for the flag, AlwaysDisplayOnFocus is local to the search bar component to clearly define what it would do. Should I just omit the OnFocus part, or do you have a better shared name to propose?

bdach · 2023-12-04T20:27:02Z

osu.Framework/Graphics/UserInterface/Dropdown.cs

+        ///  - If the dropdown search bar contains text, this method will reset it.
+        ///  - If the dropdown is open, this method wil close it.
+        /// </summary>
+        public bool Back()


This is a weird method and should probably just be inlined at its only usage.

This method is intentionally separated as it'll be called from OsuDropdown when it handles the Back action. Naming is open for suggestions, I thought of TryClose at first, maybe go fully explicit and name it ClearSearchOrClose.

bdach · 2023-12-04T20:28:51Z

osu.Framework/Graphics/UserInterface/Dropdown.cs

-
-            Header.Action = Menu.Toggle;
+            Header.ToggleMenu = Menu.Toggle;
            Header.ChangeSelection += selectionKeyPressed;
+
+            Header.SearchTerm.ValueChanged += t => Menu.SearchTerm = t.NewValue;
+
+            Header.State.BindTo(State);
+
+            Menu.RelativeSizeAxes = Axes.X;
            Menu.PreselectionConfirmed += preselectionConfirmed;
+            Menu.FilterCompleted += menuFilterCompleted;
+            Menu.StateChanged += state => State.Value = state;
+
+            State.ValueChanged += state => Menu.State = state.NewValue;


Oh jesus christmas. My brain just shut down at this number of callbacks. I'm not sure how I'm ever to review that this all coordinates correctly.

Can this be somehow simplified to not require this level of coordination?

I've removed the existence of State in Dropdown to keep things simpler, but beyond that, I'm not sure anything else can ever be changed without changing non-bindable properties to become bindables (like SearchTerm) and cause breaking changes.

frenzibyte · 2023-12-12T18:34:15Z

Comparison before/after latest changes, to ensure visual correctness:

	dropdown	text box	`TestBrowserTextBox`
before
after

Notice caret is placed and sized correctly on the dropdown search bar. Also notice caret got shorter in TestBrowserTextBox after changes, that is actually the correct size for it, since the text box was previously using a smaller font indirectly (by modifying the height of TextFlow), thus the caret was taller than the text height.

frenzibyte added 10 commits November 29, 2023 16:42

Expose dropdown menu state from Dropdown

372faa8

Add implementation for dropdown search bars

43ad6b1

Hook up dropdown search bar implementation

33e2e46

Change search bar display implementation to work better

6d215cf

Rename menu item properties in Dropdown to make sense with searching

09a14e1

Move ManualTextInput to a separate class

54a23fb

Add test coverage

739d7d6

Allow specifying custom text size in TextBox

eae6d67

Make text size of search bar match dropdown header

a399c1b

Set correct default state of matchingFilter

a385e2a

frenzibyte added the area:UI label Dec 2, 2023

frenzibyte requested a review from a team December 2, 2023 19:36

pull-request-size bot added the size/XL label Dec 2, 2023

Add basic test case

c6c97ee

bdach reviewed Dec 4, 2023

View reviewed changes

frenzibyte added 5 commits December 5, 2023 00:56

Move filtering entirely from Menu to DropdownMenu

0200dfa

Revert existence of general bindable for dropdown state

d5744b5

Remove CloseOnEscape and add local handling in DropdownMenu

1833630

Convert to auto-property

5bfe0a5

Simplify custom textbox font size logic

f0df44d

frenzibyte requested a review from bdach December 4, 2023 22:07

frenzibyte mentioned this pull request Dec 5, 2023

Add support for searching in dropdown menus (e.g. skins) ppy/osu#25682

Merged

2 tasks

peppy self-requested a review December 11, 2023 02:53

frenzibyte added 7 commits December 12, 2023 20:53

Fix text box layout when using arbitrary font sizes

393a87d

Merge branch 'master' into dropdown-search

4e543e0

Fix compile error

bb9f45e

Fix automatic textbox font size broken by layout changes

0958965

Update test border caret

1876a71

Remove unused using directive

fe0b532

Enforce centre-left anchor/origin on placeholder text

d5c2e65

peppy approved these changes Dec 13, 2023

View reviewed changes

peppy merged commit ba569bf into ppy:master Dec 13, 2023
18 of 20 checks passed

frenzibyte deleted the dropdown-search branch December 13, 2023 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dropdown searching #6072

Implement dropdown searching #6072

frenzibyte commented Dec 2, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

frenzibyte Dec 4, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023 •

edited

Loading

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

bdach Dec 4, 2023

frenzibyte Dec 4, 2023

frenzibyte commented Dec 12, 2023

		public abstract bool MatchingFilter { get; set; }

		public abstract bool FilteringActive { set; }

Implement dropdown searching #6072

Implement dropdown searching #6072

Conversation

frenzibyte commented Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte commented Dec 12, 2023

frenzibyte Dec 4, 2023 •

edited

Loading