refactor: extract Binding equality to original classes #17668
Conversation
| return false | ||
| } | ||
|
|
||
| return screen.screenEquals(other.screen) | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| // don't include the modifierKeys or mSide |
There was a problem hiding this comment.
I haven't included the modifier keys because this should be a non functional change.
But why the modifier keys are checked in equals but aren't added in the hash code? @david-allison
There was a problem hiding this comment.
Fallout from combining 'unicode' and 'keycode' lookups into the same HashMap
If we have "@" mapped, we convert the user input to a class and look it up in a hashmap to see if there is a command attached.
From memory: We lookup both based on keycode, and the system-defined unicode symbol from the KeyEvent
If using unicode:
- We do NOT want to match: [Ctrl || Alt] + Shift + 2
- We want to match
Shift + 2 - We want to match a keyboard which has a dedicated
@key:!Shift + @
This is implemented by overriding shiftMatches to return true, meaning multiple input values would match.
This is easily handled by overriding shiftMatches. Harder to handle with getHashCode, so trade off a theoretical bucket collision with ease of implementation.
There was a problem hiding this comment.
We want to match Shift + 2
At least in my ABNT2 keyboards, this doesn't happen. @, ! aren't recognized at all when pressing shift +1, 2, etc
There was a problem hiding this comment.
KeyEvent.getUnicodeChar may return @: https://developer.android.com/reference/android/view/KeyEvent#getUnicodeChar(int)
| @@ -35,6 +36,10 @@ sealed interface Binding { | |||
| append(GESTURE_PREFIX) | |||
| append(gesture) | |||
| } | |||
|
|
|||
| override fun equals(other: Any?): Boolean = other is GestureInput && gesture == other.gesture | |||
There was a problem hiding this comment.
I’m sorry but I don’t understand why you implement them.
According to the documentation the equals and hashCode are directly implemented using the values of the primary constructor for data class.
So you are recreating functions that are generated by default in a similar way.
Am I missing something?
There was a problem hiding this comment.
equals and hashCode are directly implemented using the values of the primary constructor for data class.
Forgot about that. Thanks a lot!
There was a problem hiding this comment.
Had to keep the hashCode method for the KeyBindings because of #17668 (comment)
BrayanDSO
force-pushed
the
ref-binding
branch
from
8dd9330 to
f8b28ff
Compare
BrayanDSO
added
the
Needs Author Reply
BrayanDSO
force-pushed
the
ref-binding
branch
from
f8b28ff to
e7b7ae8
Compare
BrayanDSO
added
Needs Review
and removed
Needs Author Reply
| @@ -179,6 +188,10 @@ sealed interface Binding { | |||
| ) | |||
| } | |||
|
|
|||
| override fun equals(other: Any?): Boolean = other is ModifierKeys && semiStructuralEquals(other) | |||
|
|
|||
| override fun hashCode(): Int = Objects.hash(ctrl, alt, shift, shiftMatches(true), shiftMatches(false)) | |||
There was a problem hiding this comment.
object 1:
- shiftMatches(true) => true
- shiftMatches(false) => true
object 2:
- shiftMatches(true) => true
- shiftMatches(false) => false
The hashcodes would not be equal, but this.shiftMatches(true) == keys.shiftMatches(true) would be true, implying equality
Probably warrants a regression test
david-allison
added
the
Needs Author Reply
so it's possible to compare bindings without a MappableBinding
How Has This Been Tested?
I'm trusting the unit tests. Went around and tested some keybinds and gestures and they still work.
I couldn't test some keybinds like
*(mark default) and!(suspend default) even before these changes. They are only recognized asShift+1andShift+8hereChecklist
Please, go through these checks before submitting the PR.