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

Add mod search #23414

Merged
merged 63 commits into from
Jun 18, 2023
Merged

Add mod search #23414

merged 63 commits into from
Jun 18, 2023

Conversation

Cootz
Copy link
Contributor

@Cootz Cootz commented May 6, 2023

As discussed in #22893 this PR adds search bar to mod overlay

Showcase

ModSearchShowcase.mp4

Need advice

I'm not a designer so I need some help with defining optimal location for the search bar on both UserModSelectOverlay and FreeModSelectOverlay (especially last one).
Also animation at this state are quite rude (panels and columns just disappear from overlay) so I would like to get some help there as well.

@cdwcgt
Copy link
Contributor

cdwcgt commented May 6, 2023

The shortcut key of “deselect all” is backspace, which should be blocked when using the search box.
and if I use deselect all when have filtered mod selected(not show in overlay but enabled), some mod will not disable.

osu.framework.running._osu-development_.2023-05-07.01-23-28_x264.mp4

ps: I changed DifficultyMultiplierDisplay to TopRight by myself, the rest of the code was unchanged

@Cootz
Copy link
Contributor Author

Cootz commented May 7, 2023

@cdwcgt this bug now fixed and have test coverage (see TestDeselectAllViaButton_WithSearchApplied). I'll investigate into shortcut keys bug when I have time

@peppy
Copy link
Sponsor Member

peppy commented May 17, 2023

For the UX of this feature, I'd expect a few things:

  • Escape should clear search, just like song select and settings. This will allow for rapidly finding the mods a user wants
  • Hitting enter after searching should toggle the first mod visible (maybe only if there's one result? maybe always?)
  • I'd really prefer the search was active by default. Obviously this won't work great with the default hotkeys for mods, so we'll have to think about this one a bit further (probably a setting which defaults to on, alongside changing the default hotkeys for mods to have a modifier key like ctrl).

@Cootz
Copy link
Contributor Author

Cootz commented May 17, 2023

  • Escape should clear search, just like song select and settings. This will allow for rapidly finding the mods a user wants

It already does

  • Hitting enter after searching should toggle the first mod visible (maybe only if there's one result? maybe always?)

I think it should be always first as sometimes mod search shows multiple modes even when you fully entered the mod name.

image

Also, It would be better to select/deselect on enter. (What about doing it on hitting search button as well? 🤔)

  • I'd really prefer the search was active by default. Obviously this won't work great with the default hotkeys for mods, so we'll have to think about this one a bit further (probably a setting which defaults to on, alongside changing the default hotkeys for mods to have a modifier key like ctrl).

This one is what I can't decide. I like the idea of having search always focused and just changing the hotkeys, as it's a better UI option. Yet changing the hotkeys makes the setting useless as all the hotkeys can be used with active search. Need to mention that the focus logic is already done (also have test coverage), so changing the search to always focused means that the logic needs to be reverted. And now it's either adding useless option to the settings or revert some progress. @peppy, any thoughts on this?

Current search focus logic showcase

searchBoxFocusShowcase.mp4

@peppy
Copy link
Sponsor Member

peppy commented May 25, 2023

Just to confirm, is this PR dependent on ppy/osu-framework#5772?

@Cootz
Copy link
Contributor Author

Cootz commented May 26, 2023

Just to confirm, is this PR dependent on ppy/osu-framework#5772?

Yes, unless everyone is ok with calling Update() manually.

Cootz and others added 14 commits June 12, 2023 13:09
`Bindable` has one of those already.
- Was on wrong ruleset, so the mod/free mod sets did literally nothing
- `assertHasFreeModButton` had a param that did nothing
- Was checking `MatchingFilter` rather than `Visible`
Universally disliked. `!= false` is preferred.
Was preventing mod preset panels from refiltering correctly on ruleset
change due to the `matchingFilter == value` guard.
@bdach
Copy link
Collaborator

bdach commented Jun 18, 2023

I've pushed a whole bunch of changes here. @Cootz please read through them and keep this sort of stuff in mind for future contributions. If there is something that is unclear in the changes that I've made then please let me know.

@peppy I think I've cleaned this up as best as I can, so that this can be considered for merging for next release. There's still a whole bunch of hacks here to work around various issues with SearchContainer and input handling idiosyncrasies and the like, but I have no better answers than what this branch is doing and have failed to come up with such for almost half a year now. Please check that they don't exceed your pain threshold.

bdach
bdach previously approved these changes Jun 18, 2023
@bdach bdach requested a review from peppy June 18, 2023 13:01
@peppy peppy merged commit f8b94ed into ppy:master Jun 18, 2023
13 of 17 checks passed
@Cootz Cootz deleted the add-mod-search-option branch June 18, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants