Only allow seek to next/previous object via keybinding if there is no selection #29860
Conversation
|
Thoughts on making nudging position use ctrl-alt instead? To me it feels like a rarer operation so I'd prefer it was less accessible. Reason for not doing this is to match stable expectations, I guess. |
|
I'm a bit worried about the keybinds for nudging selection and seeking interfering in the first place, because this is limiting the available keybinds for selection as well. We already have to resort to quite complex modifier combinations for simple actions. We might want to change logic so it prioritizes keybinds for selection (nudging) whenever there is a selection, and do the more global keybinds (seeking) only if there is no selection or it is not handled by the selection. This way the hotkeys could be more context-aware in a sense. |
|
Contextually aware was also something I thought about. We could give it a try and see how it feels, for sure. |
I considered it but I dunno if we want to go there because it's putting layers upon layers again. If you both think that's worth a go then sure. |
bdach
changed the title
| @@ -721,10 +721,16 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e) | |||
| switch (e.Action) | |||
| { | |||
| case GlobalAction.EditorSeekToPreviousHitObject: | |||
| if (editorBeatmap.SelectedHitObjects.Any()) | |||
| return false; | |||
There was a problem hiding this comment.
Is there no way to catch if any event has already been handled by a selection? It'd be nice if this could leverage the input handling system in some way to handle this in a general way instead of making all these a conditionals.
Also I think it should be able to seek while there is a selection if you have bound the seek key to some unused key, which in this implementation is not possible.
There was a problem hiding this comment.
Is there no way to catch if any event has already been handled by a selection
No. Or at least no sane way.
Also I think it should be able to seek while there is a selection if you have bound the seek key to some unused key
I refuse to do such checks because it's weird and means that we have failed and made everything too customisable. Now things work or don't work depending on how users' bindings are set up. I am not willing to be on the hook for diagnosing weird failure modes like that.
There was a problem hiding this comment.
I don't see how more customisability amounts to a 'fail'. More customisability is often better.
I guess most of your issue lies with the implementation of my suggestion as it would probably be a pain.
Current implementation is still weird because this context awareness is only on the hitobject seek while there is none for the other seek types. The only distinction between the seek types is that the hitobject seek is a formerly conflicting keybind.
To me a makes a lot more sense if all the seek keybinds adhere to the same rules:
- No conflicted keybind -> works in all contexts
- Conflicted keybind -> only works if there is no selection
There was a problem hiding this comment.
To me a makes a lot more sense if all the seek keybinds adhere to the same rules:
- No conflicted keybind -> works in all contexts
- Conflicted keybind -> only works if there is no selection
No I do not accept this premise. Doing this is a failure of design.
There are two options:
- You make things customisable but also make sure to put enough guardrails to ensure the user can't shoot their foot off with the customisation options.
- You don't make things customisable at all.
I'm not having this halfway "you make things customisable but by doing so you allow other features to silently change behaviour for no observable reason" you're proposing here. If you insist you can try implementing it yourself but I will not put my hand to it by either trying to implement this or approving it during review.
bdach
added
the
next release
RFC. Closes #29803. cc/ @OliBomby.
New default is Ctrl-Alt-←/→ (in other words, Alt was added).
Passes tests so hopefully fine. Our code is notoriously ungreppable for this sort of thing. Additionally features extended test coverage for both clashing features which was not there.