pharo-project / pharo

Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk.
http://pharo.org
Other
1.22k stars 356 forks source link

Fix repeat events #17301

Closed iglosiggio closed 1 month ago

iglosiggio commented 1 month ago

https://github.com/libsdl-org/SDL/commit/61cd57d378e6bf5a62a8bf49e02164df63924c56 made SDL2 trigger new repeat keydown events in order to notify the library user that modifiers may have changed.

This change messed up the way we did keybind matching. Doing sequences like Cmd+Shift+W, E never matched because those new keydown events broke everything.

After pursuing multiple solutions this one that I found worked best. Still, it feels like a hack and I'm open for alternatives.

Solutions tried

Solution proposed

When trying to match a keybinding we now try to match it against the buffered sequence of events. If that fails we remove the repeated events and try again.

Why did you do this?

This change restores most of the window tiling keybindings (for some reason Cmd+Shift+W, Q does not work).

Thanks @PalumboN and @guillep for their help on diagnosing this :)

MarcusDenker commented 1 month ago

There are lots of tests failing that look related of the kind

Error Message

proceed for truth.
Stacktrace

NonBooleanReceiver
proceed for truth.
UndefinedObject(ProtoObject)>>mustBeBooleanIn:
UndefinedObject(ProtoObject)>>mustBeBoolean
OrderedCollection>>removeAllSuchThat:
[ :targetToDispatch |
        | sequence |
        sequence := KMBuffer uniqueInstance buffer copy.
        targetToDispatch dispatch: sequence.
        aKeyboardEvent wasHandled ifTrue: [ ^self ].
        "Let's try to match this sequence of events again.
        This time ignoring any repeated events."
        sequence removeAllSuchThat: [ :ev | ev isRepeat ].
        sequence isNotEmpty ifTrue: [
            targetToDispatch dispatch: sequence.
            aKeyboardEvent wasHandled ifTrue: [ ^self ] ]
    ] in KMDispatchChain>>dispatch:
iglosiggio commented 1 month ago

There are lots of tests failing that look related of the kind

Error Message

proceed for truth.
Stacktrace

NonBooleanReceiver
proceed for truth.
UndefinedObject(ProtoObject)>>mustBeBooleanIn:
UndefinedObject(ProtoObject)>>mustBeBoolean
OrderedCollection>>removeAllSuchThat:
[ :targetToDispatch |
      | sequence |
      sequence := KMBuffer uniqueInstance buffer copy.
      targetToDispatch dispatch: sequence.
      aKeyboardEvent wasHandled ifTrue: [ ^self ].
      "Let's try to match this sequence of events again.
      This time ignoring any repeated events."
      sequence removeAllSuchThat: [ :ev | ev isRepeat ].
      sequence isNotEmpty ifTrue: [
          targetToDispatch dispatch: sequence.
          aKeyboardEvent wasHandled ifTrue: [ ^self ] ]
  ] in KMDispatchChain>>dispatch:

🤦 isRepeat ends up with nil as its default value. That's the reason I broke a bunch of tests. I'll update the PR so it is false by default.

Also, this PR needs some tests 😅.

jecisc commented 1 month ago

Restarting the CI after a random failure (zlib)

Ducasse commented 1 month ago

I think that the tests are passing now. The broken ones seem unlreated

Zinc.HTTP.Examples/ZnImageExampleDelegateTest/unix_64_Tests_unix64testUpload/) Windows64.UnifiedFFI.Tests.FFIAutoReleaseOptionCalloutTest.testExternalStructure]17Windows64.UnifiedFFI.Tests.FFIAutoReleaseOptionCalloutTest.testVoidPointer]stReport/junit/Windows64.UnifiedFFI.Tests/FFIAutoReleaseOptionCalloutTest/windows_64_Tests_windows64testVoidPointer/)

welcome[bot] commented 1 month ago

Congrats on merging your first pull request! Do another one! We try to have a list of (relatively) easy issues here: https://github.com/orgs/pharo-project/projects/8.