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

Prioritize selecting exact searched acronym with select keybind #30330

Merged
merged 5 commits into from
Oct 22, 2024

Conversation

jhk2601
Copy link
Contributor

@jhk2601 jhk2601 commented Oct 17, 2024

Addresses #30001

Nice QOL change, before searching "SY" for the synesthesia mod and pressing enter would select EZ, since selection was prioritized by column/descending order instead of accuracy. This interaction was actually pretty common and is not exclusive to EZ/SY. Videos attached to explain. (Why EZ appears when SY is searched I truly have no idea. Sy does not appear in the description. Dark magic is likely involved). Also added support for exact name searching though I'm 99 percent sure it isn't ever relevant.

Old.mp4
Fixed.mp4

@SchiavoAnto
Copy link
Contributor

(Why EZ appears when SY is searched I truly have no idea. Sy does not appear in the description. Dark magic is likely involved)

I would guess it appears because "sy" is contained in "Easy".

@jhk2601
Copy link
Contributor Author

jhk2601 commented Oct 17, 2024

(Why EZ appears when SY is searched I truly have no idea. Sy does not appear in the description. Dark magic is likely involved)

I would guess it appears because "sy" is contained in "Easy".

I'm going to pretend I knew this the whole time to preserve my faith in my ability to read.

smoogipoo
smoogipoo previously approved these changes Oct 18, 2024
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm okay with this. The only one I found maybe questionable is ad and da, but it's probably okay for users to have to write adj to select DifficultyAdjust in the former and day to select Daycore in the latter.

Requesting second opinion by @ppy/team-client

@bdach
Copy link
Collaborator

bdach commented Oct 18, 2024

I can get behind the idea. Not 100% on the execution. The trouble I have is that the consistency of "first result will be selected" is a bit gone with this change. If I were to suggest anything to address that it'd maybe be adding some sort of "preselect" indicator like e.g. outer glow to the mod that will be selected if you press enter.

Not sure. Going to tag in @peppy specifically as is the custom when UX discussions are taking place (especially so that this entire enter x search interaction was his idea in the first place).

@bdach bdach requested a review from peppy October 18, 2024 10:23
@peppy
Copy link
Member

peppy commented Oct 18, 2024

I think I'm okay with this, but I do agree that some kind of glow / indicator would make it absolutely amazing. If that can be added easily then it should be done here IMO. Worth spending 20-30 minutes on it if anyone wants to give it a try.

Only other consideration i'd have is whether you need to use upper case for this to work. Probably fine without case considerations though.

@bdach
Copy link
Collaborator

bdach commented Oct 18, 2024

Only other consideration i'd have is whether you need to use upper case for this to work. Probably fine without case considerations though.

Casing is explicitly ignored, e.g. "da" would prefer Difficulty Adjust over Daycore as stated above. Would you be expecting that to be case-sensitive, e.g. "da" to select Daycore, but "DA" to select Difficulty Adjust?

I imagine the latter would be a bit annoying for users.

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 18, 2024

My anecdotal experience with DT (so it's not really applicable here because it's in a realm of its own), is that I type dt exactly. I don't like typing dou (have to rejig my brain to do that) and also don't like reaching for the shift key which is potentially difficult to do with one hand for some mods (as my other hand is usually holding my pen).

@peppy
Copy link
Member

peppy commented Oct 18, 2024

Yeah that's fine, is what I was trying to say (case insensitive is probably the correct approach, just wanted to raise it as a point of discussion).

@bdach
Copy link
Collaborator

bdach commented Oct 18, 2024

Alright then I guess I'll have a go at the indicator thing?

@bdach
Copy link
Collaborator

bdach commented Oct 18, 2024

Alright I've done something:

2024-10-18.13-24-34.mp4

Suggestions, if any?

@jhk2601
Copy link
Contributor Author

jhk2601 commented Oct 18, 2024

Alright I've done something:

2024-10-18.13-24-34.mp4
Suggestions, if any?

Looks great to me, unobtrusive but clear what is selected, thanks for the addition. Would it be going too far to include mod presets here? (I.e. I search "1337" and that preset gets highlighted/prioritized for select). I'm not familiar enough with how presets are handled to determine if that has any edge case implications (Is naming your preset "DT" allowed?) but I think that would be a nice addition here if possible.

@bdach
Copy link
Collaborator

bdach commented Oct 18, 2024

Would it be going too far to include mod presets here?

Wouldn't do it in this pull. I'm not sure how to mix presets into all this. As in you should probably be able to quick select both a preset and a mod.

I'd like to get this in in the current state if deemed viable without going down that particular discussion.

@jhk2601
Copy link
Contributor Author

jhk2601 commented Oct 18, 2024

Would it be going too far to include mod presets here?

Wouldn't do it in this pull. I'm not sure how to mix presets into all this. As in you should probably be able to quick select both a preset and a mod.

I'd like to get this in in the current state if deemed viable without going down that particular discussion.

alright valid, I took a quick look and it definitely feels out of scope, another time.

@bdach bdach added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Oct 22, 2024
peppy
peppy previously approved these changes Oct 22, 2024
@bdach bdach merged commit 84e08d9 into ppy:master Oct 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:overlay-mod-select next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants