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 KeyBindingContainer.SendRepeats #4896

Merged
merged 5 commits into from
Nov 18, 2021

Conversation

peppy
Copy link
Member

@peppy peppy commented Nov 16, 2021

Reasoning mentioned in #4773.

See a pretty shocking oversight in db22119 which went unnoticed until now as tests were focused on the non-event-based repeat handling (which is removed in this PR). Existing tests cover the fail case - revert the commit to confirm.

Breaking Changes

KeyBindingContainer.SendRepeats no longer exists

Use the KeyBindingPressEvent provided in OnPressed delegates to identify (and ignore or handle) repeats instead.

@peppy peppy added the blocked label Nov 16, 2021
@peppy
Copy link
Member Author

peppy commented Nov 16, 2021

Blocking temporarily. A couple of oversights I'm working on still.

@peppy
Copy link
Member Author

peppy commented Nov 17, 2021

Turns out that key repeat (via event flow) wasn't really handled correctly in KeyBindingContainer until now. I've restructured things to work roughly as expected.

The existing test coverage seems to be enough to cover this behaviour.

@peppy peppy removed the blocked label Nov 17, 2021
@bdach
Copy link
Collaborator

bdach commented Nov 17, 2021

I've fired off a nitpick commit to correct a slightly strange test step name.

In general this diff looks sane to me. One thing I'm not sure of, is that when SimultaneousBindingMode is not None, repeats are maybe a touch counterintuitive in that when I press and hold Ctrl+A in the key bindings grid test scene, the initial press activates A, Ctrl, and Ctrl_A actions, but then repeats only get fired for whichever single key was pressed last, which is maybe a little counter-intuitive. But I'm not really super sure what the behaviour should be there either, so maybe best to leave the simple case working as-is and tackle that edge case if it ever comes up...

@bdach bdach requested a review from smoogipoo November 17, 2021 21:51
@peppy
Copy link
Member Author

peppy commented Nov 18, 2021

Yeah, it isn't a 1:1 with expectations but I'd rather address that as a follow up, I think. Keep in mind it's working with the last action, not bindings, so in a simple case where Ctrl-A is bound to an action that will still get correctly repeated.

On macOS (and windows from my recollection), the last non-modifier is the key to be repeated, but modifiers are not included in the equation. So the true behaviour if we're looking for parity would be for the modifier Ctrl-A to repeat even in your scenario. We can make this happen if/when the requirement comes up.


var pressEvent = new KeyBindingPressEvent<T>(state, action, true);

return keyRepeatInputQueue.FirstOrDefault(d => triggerKeyBindingEvent(d, pressEvent)) != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a few more conditions, as per

// Only drawables that can still handle input should handle the repeat
var drawables = ButtonDownInputQueue.Intersect(InputQueue).Where(t => t.IsAlive && t.IsPresent);

But I understand that's a bit complex to get done in this PR, so looks fine as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw that. But those conditionals also weren't applied to the other path as far as I can see (I guess it's less important because it's an immediate firing?) so I didn't think too much of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other path being what? In both cases it's the non-repeat path, and the expectation is that it doesn't contain !IsAlive and !IsPresent items in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that part. I'll add the conditionals in a follow-up PR.

@smoogipoo smoogipoo merged commit d6943f5 into ppy:master Nov 18, 2021

var pressEvent = new KeyBindingPressEvent<T>(state, action, true);

return keyRepeatInputQueue.FirstOrDefault(d => triggerKeyBindingEvent(d, pressEvent)) != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this is just incorrect because the input queues are per-keybinding...

Copy link
Member Author

Choose a reason for hiding this comment

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

Which case would be incorrect? The idea is that the repeat is only sent to the last pressed key binding, where this queue is taken from directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I see. Seems okay on a second think...

@smoogipoo
Copy link
Contributor

Going to need osu!-side fixes for this as well, fwiw.

@peppy
Copy link
Member Author

peppy commented Nov 18, 2021

Yeah. I did a quick testing pass and nothing was too broken (and the places this intends to fix were fixed), but there's going to be multiple places we need to block repeats. Will look at that today.

@peppy peppy deleted the remove-send-key-repeats branch November 18, 2021 04:24
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.

3 participants