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

Change mapping delete key to shift+delete to allow binding delete key #2375

Merged
merged 8 commits into from May 14, 2018

Conversation

2 participants
@aQaTL
Contributor

aQaTL commented Apr 9, 2018

This is my solution for Issue #2363. Delete key still deletes key binding, but you can now bind it by alt+delete key combination. In addition to that, now it is possible to assign any key combination with Delete key except alt+delete.

One thing I'm not sure about is to where it should be documented.

@peppy

This comment has been minimized.

Member

peppy commented Apr 10, 2018

If anything, this is backwards and removing existing bindings should be done using Shift+Del.

We may be better off designing the UI to have a button to unbind as well, for cases where a keyboard is not available.

@peppy

This comment has been minimized.

Member

peppy commented Apr 11, 2018

You didn't update the label to match.

@aQaTL

This comment has been minimized.

Contributor

aQaTL commented Apr 11, 2018

Label updated. I also adjusted the overlay width. Not sure how you would have preferred to do that - I changed the width only of KeyBindingOverlay instead of SettingsOverlay.

@peppy peppy changed the title from Allow mapping delete key via alt+delete key combination to Change mapping delete key to shift+delete to allow binding delete Apr 18, 2018

@@ -198,18 +198,22 @@ protected override bool OnKeyDown(InputState state, KeyDownEventArgs args)
if (!HasFocus)
return false;
switch (args.Key)

This comment has been minimized.

@peppy

peppy Apr 18, 2018

Member

switch back to switch and use state.Keyboard.ShiftPressed, i think it's neater.

aQaTL and others added some commits Apr 18, 2018

@@ -12,6 +12,8 @@ namespace osu.Game.Overlays
{
public class KeyBindingOverlay : SettingsOverlay
{
protected const float WIDTH = 430;

This comment has been minimized.

@peppy

peppy May 10, 2018

Member

why is this required?

This comment has been minimized.

@aQaTL

aQaTL May 10, 2018

Contributor

I needed to make KeyBindingOverlay a bit wider to make the pressAKey label fit. Otherwise the label text would be truncated at the end.

This comment has been minimized.

@aQaTL

aQaTL May 10, 2018

Contributor

I added this here to not expand the whole SettingsOverlay

This comment has been minimized.

@peppy

peppy May 14, 2018

Member

This cannot be done this way.

peppy added some commits May 14, 2018

@peppy peppy changed the title from Change mapping delete key to shift+delete to allow binding delete to Change mapping delete key to shift+delete to allow binding delete key May 14, 2018

@peppy

This comment has been minimized.

Member

peppy commented May 14, 2018

In the process of fixing that last piece, I discovered a framework-level issue (ppy/osu-framework#1552) but as this is completely irrelevant to this PR and already existed before it, this should be fine to go through with 👍.

@peppy

peppy approved these changes May 14, 2018

@peppy peppy merged commit 15d6c1f into ppy:master May 14, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment