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

Fix bug: Shift modifier incorrectly added to filter shortcuts #3999

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Oct 16, 2022


Description:
See above issue for problem description.

Summary / conclusions
This terrifying block of code appears to be the culprit:

extension NSEvent {
  var readableKeyDescription: (String, String) {
    get {

      let rawKeyCharacter: String
      if let char = NSEventKeyCodeMapping[Int(self.keyCode)] {
        rawKeyCharacter = char
      } else {
        let inputSource = TISCopyCurrentASCIICapableKeyboardLayoutInputSource().takeUnretainedValue()
        if let layoutData = TISGetInputSourceProperty(inputSource, kTISPropertyUnicodeKeyLayoutData) {
          let dataRef = unsafeBitCast(layoutData, to: CFData.self)
          let keyLayout = unsafeBitCast(CFDataGetBytePtr(dataRef), to: UnsafePointer<UCKeyboardLayout>.self)
          var deadKeyState = UInt32(0)
          let maxLength = 4
          var actualLength = 0
          var actualString = [UniChar](repeating: 0, count: maxLength)
          let error = UCKeyTranslate(keyLayout,
                                     UInt16(self.keyCode),
                                     UInt16(kUCKeyActionDisplay),
                                     UInt32((0 >> 8) & 0xFF),
                                     UInt32(LMGetKbdType()),
                                     OptionBits(kUCKeyTranslateNoDeadKeysBit),
                                     &deadKeyState,
                                     maxLength,
                                     &actualLength,
                                     &actualString)
          if error == 0 {
            rawKeyCharacter = String(utf16CodeUnits: &actualString, count: maxLength).uppercased()

          } else {
            rawKeyCharacter = KeyCodeHelper.keyMap[self.keyCode]?.0 ?? ""
          }
        } else {
          rawKeyCharacter = KeyCodeHelper.keyMap[self.keyCode]?.0 ?? ""
        }
      }

      return (([(.control, ""), (.option, ""), (.shift, ""), (.command, "")] as [(NSEvent.ModifierFlags, String)])
        .map { self.modifierFlags.contains($0.0) ? $0.1 : "" }
        .joined()
        .appending(rawKeyCharacter), rawKeyCharacter)
    }
  }
}

The value it returns will always be in uppercase, which is fine for display purposes, but it shouldn't have been using it to store the "raw" key value (The code was already grabbing the raw value from the user and putting it in currentKey, and it was far simpler to just use that). It has another problem: it seems to add 3 null characters to the string it returns. The debugger shows that t is stored as T\0\0\0.

This is not good, but I'm going to punt and declare it to be outside the scope of this defect.

After this fix, readableKeyDescription is only used in two places:

  1. In FilterWindowController for display. At least this doesn't seem to be breaking anything.
  2. In KeyCodeHelper.mpvKeyCode(), where it is checked for inequality against another string, causing a block of code to always evaluate to true - which is ok because that block should always be executed.

This should be cleaned up at some point. Rather than try to debug that block, a more prudent solution would be to remove all use of it and find a simpler substitute if needed. At least this PR makes strides in that direction.

@svobs svobs changed the title Fix bug: Shift key was being incorrectly added to filter bindings Fix bug: Shift modifier incorrectly added to filter shortcuts Oct 16, 2022
@uiryuu uiryuu added the bug label Oct 18, 2022
Copy link
Contributor

@low-batt low-batt left a comment

Choose a reason for hiding this comment

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

Reproduced problem. Pulled PR locally, built, confirmed it fixed the problem.

@uiryuu uiryuu merged commit 3de15d0 into iina:develop Apr 11, 2023
@uiryuu
Copy link
Member

uiryuu commented Apr 11, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saved filter key shortcuts: lowercase letters have shift modifier added
3 participants