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

Remove default keybind for "Toggle FPS counter" #30264

Merged
merged 5 commits into from
Oct 16, 2024

Conversation

LBlend
Copy link
Contributor

@LBlend LBlend commented Oct 13, 2024

Closes #30169

Referring to #30252. As per peppy's suggestion I changed the keybind for toggling the FPS counter. As per peppy's new suggestion I removed the default keybind for toggling the FPS counter. This is done in order to avoid conflicts with the keybind for converting a slider to a stream in the editor.

Why Ctrl-Alt-C?

  • Ctrl-Shift-C is copying without formatting
  • Same goes for Ctrl-Shift-X
  • Ctrl-Alt-F is used to fullscreen windows in certain desktop environments
  • The other combinations that are in reach are in use by the game
  • Ctrl-Alt-C holds some semantic meaning in the sense that "C" is short for "counter".

@peppy
Copy link
Member

peppy commented Oct 13, 2024

Just set it to have no default. Please let's not try and rationalise absolutely arbitrary keys. It's not worth it.

@LBlend LBlend changed the title Change "Toggle FPS" keybind Remove default keybind for "Toggle FPS counter" Oct 13, 2024
@bdach
Copy link
Collaborator

bdach commented Oct 14, 2024

This doesn't do anything to fix the issue for users whose databases have already been populated with default keybindings as they will continue to be bound to the conflicting keybind...?

I'm not sure why this was deemed the correct course of action in light of that, it feels as much of a cop-out solution as #30252 did.

@peppy
Copy link
Member

peppy commented Oct 14, 2024

I'm not sure why this was deemed the correct course of action in light of that, it feels as much of a cop-out solution as #30252 did.

I guess we could also unbind it for the case it's set to the default? I'm not interested in fixing it for new installs or when a user resets bindings.

I can't think of another game existing with a hotkey for fps display, so I don't think osu! should have it.

@bdach
Copy link
Collaborator

bdach commented Oct 14, 2024

I guess we could also unbind it for the case it's set to the default?

That is what we've historically done for hotkey conflicts, but it will require a realm migration to be included here.

var keyBindings = migration.NewRealm.All<RealmKeyBinding>();

var toggleFpsBind = keyBindings.FirstOrDefault(bind => bind.ActionInt == (int)GlobalAction.ToggleFPSDisplay);
if (toggleFpsBind != null && toggleFpsBind.KeyCombination.Keys.SequenceEqual(new[] { InputKey.Control, InputKey.Shift, InputKey.F }))
Copy link
Collaborator

@bdach bdach Oct 16, 2024

Choose a reason for hiding this comment

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

Was this tested at all? The .SequenceEqual() here didn't work and should have been

Suggested change
if (toggleFpsBind != null && toggleFpsBind.KeyCombination.Keys.SequenceEqual(new[] { InputKey.Control, InputKey.Shift, InputKey.F }))
if (toggleFpsBind != null && toggleFpsBind.KeyCombination.Keys.SequenceEqual(new[] { InputKey.Shift, InputKey.Control, InputKey.F }))

I've fixed it (f8a13b0) but really this should not have gone through if this code was tested.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

I guess this works, if this is what we're going to do

@bdach bdach requested a review from peppy October 16, 2024 06:25
@peppy peppy merged commit c9519dc into ppy:master Oct 16, 2024
11 of 13 checks passed
@LBlend LBlend deleted the change-fps-toggle-keybind branch October 16, 2024 09:08
@SupDos SupDos mentioned this pull request Nov 24, 2024
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.

Editor shortcut for transforming slider to stream is conflicting with "show fps" setting
3 participants