2
\$\begingroup\$

This is my first program in Swift/Xcode. The full source code and xcode project is available in GitHub.

The part I would like reviewed is IOKeyEventMonitor.swift.

//Copyright [2016] Jean Helou
//
//Licensed under the Apache License, Version 2.0 (the "License");
//you may not use this file except in compliance with the License.
//You may obtain a copy of the License at
//
//http://www.apache.org/licenses/LICENSE-2.0
//
//Unless required by applicable law or agreed to in writing, software
//distributed under the License is distributed on an "AS IS" BASIS,
//WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
//See the License for the specific language governing permissions and
//limitations under the License.
import Foundation
import Carbon
import IOKit
import IOKit.usb
import IOKit.hid


struct IOKeyEventMonitorContext {
  var lastSeenSender: String
  init(lastSeenSender: String) {
    self.lastSeenSender = lastSeenSender
  }
}

class IOKeyEventMonitor {
  private
  let hidManager: IOHIDManager
  let notificationCenter: CFNotificationCenter
  let match: CFMutableDictionary

  var lastActiveKeyboard: String = ""
  var kb2is: [String: TISInputSource] = [String: TISInputSource]()


  private class func createDeviceMatchingDictionary( usagePage: Int, usage: Int) -> CFMutableDictionary {
    let dict = [
      kIOHIDDeviceUsageKey: usage,
      kIOHIDDeviceUsagePageKey: usagePage
    ] as NSDictionary

    return dict.mutableCopy() as! NSMutableDictionary;
  }

  init? ( usagePage: Int, usage: Int) {
    hidManager = IOHIDManagerCreate( kCFAllocatorDefault, IOOptionBits(kIOHIDOptionsTypeNone));
    notificationCenter = CFNotificationCenterGetDistributedCenter();
    match = IOKeyEventMonitor.createDeviceMatchingDictionary(usagePage: usagePage, usage: usage);
    IOHIDManagerSetDeviceMatching( hidManager, match);
  }

  deinit {
    // FIXME find out how to pass nil as an IOKit.IOHIDValueCallback to unregister the callback
    let context = UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque());
    //IOHIDManagerRegisterInputValueCallback( hidManager, nil , context);
    CFNotificationCenterRemoveObserver(notificationCenter, context, CFNotificationName(kTISNotifySelectedKeyboardInputSourceChanged), nil);
  }

  func restoreInputSource(keyboard: String) -> Void {
    if let targetIs = kb2is[keyboard] {
      //print("set input source to \(targetIs) for keyboard \(keyboard)");
      TISSelectInputSource(targetIs)
    } else {
      self.storeInputSource(keyboard: keyboard);
    }
  }

  func storeInputSource(keyboard: String) -> Void {
    let currentSource: TISInputSource = TISCopyCurrentKeyboardInputSource().takeUnretainedValue();
    kb2is[keyboard] = currentSource;
  }

  func onInputSourceChanged() -> Void {
    self.storeInputSource(keyboard: self.lastActiveKeyboard);
  }

  func onKeyboardEvent(keyboard: String) -> Void {
    if(self.lastActiveKeyboard != keyboard) {
      //print("Active keyboard changed from \(self.lastActiveKeyboard) to \(keyboard)");
      self.restoreInputSource(keyboard: keyboard);
      self.lastActiveKeyboard = keyboard;
    }
  }


  func start() -> Void {
    let context = UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque());
    let myHIDKeyboardCallback: IOHIDValueCallback = {
      (context, ioreturn, sender, value) in
      let selfPtr = Unmanaged<IOKeyEventMonitor>.fromOpaque(context!).takeUnretainedValue();
      let senderDevice = Unmanaged<IOHIDDevice>.fromOpaque(sender!).takeUnretainedValue();

      let vendorId = String(describing: IOHIDDeviceGetProperty(senderDevice, kIOHIDVendorIDKey as CFString));
      let productId = String(describing: IOHIDDeviceGetProperty(senderDevice, kIOHIDProductIDKey as CFString));
      let product = String(describing: IOHIDDeviceGetProperty(senderDevice, kIOHIDProductKey as CFString));
      let keyboard = "\(product)[\(vendorId)-\(productId)]";
      selfPtr.onKeyboardEvent(keyboard: keyboard);

    }
    let inputSourceChanged: CFNotificationCallback = {
      (center, observer, name, notif, userInfo) in
      let selfPtr = Unmanaged<IOKeyEventMonitor>.fromOpaque(observer!).takeUnretainedValue();
      selfPtr.onInputSourceChanged()
    }

    CFNotificationCenterAddObserver(notificationCenter,
                                    context, inputSourceChanged,
                                    kTISNotifySelectedKeyboardInputSourceChanged, nil,
                                    CFNotificationSuspensionBehavior.deliverImmediately);


    IOHIDManagerRegisterInputValueCallback( hidManager, myHIDKeyboardCallback, context);
    IOHIDManagerScheduleWithRunLoop( hidManager, CFRunLoopGetMain(), CFRunLoopMode.defaultMode!.rawValue);
    IOHIDManagerOpen( hidManager, IOOptionBits(kIOHIDOptionsTypeNone));
  }

}

How would you rewrite this to be idiomatic Swift? How can I improve the start() function in particular?

\$\endgroup\$
1
\$\begingroup\$

Well, you write better code than many (myself probably included) that have been Swifting for a while. So, for your first program, it's impressive.

However, code is kinda bunched up (for my taste), perhaps a few more blank lines? Also, runs way too far out in a few places... See how I "wrapped" those long function prams.

Also, some nitpicks I found:

parenthesis consistency. ( x) (x) ( x )

func declaration spacing inconsistency:

func x(){}

func y() {}


func z() {}

forgot : in fixme

using ;

using -> Void

no space after //

using self when not in a closure on initializer

used private on its own line, not sure if you wanted all members private or just the first field. (First field in class)

if(self.lastActiveKeyboard != keyboard) no space in between? And parenthesis can be saved for more complicated statements: if lastActiveKeyboard != keyboard {

no // MARK: for easy navigation. I added one so you can quickly hop between your properties + initializers and your functions, since you already separated them out into halves :) which is good! Make a keyboard shortcut for Show Document Items if you havent already, and you will see all of your FIXME: and MARK: in a list

Missed one procedural style... used storeInputSource before it was declared... (inside restore) NOTE: this isn't "required" but it helps your logic flow (to yourself and readers) but sometimes you will have to break this depending on your style.

closures typically inline up to in, and you have inconsistencies with this regardless. I would only newline it if you go past the 90th column.

inconsistent closing brace } (sometimes have empty line, sometimes no space)

no comments or documentation (will you understand this code a month from now? What about people that might look at it? Or people you ask for help with it?)

So mostly nitpicks. Nothing particularly un-swifty about this code. For no particular reason, I want to make sure you know about guard because you do have one if let statement in there, but it's a good use of if let. Just wanted to make sure you knew about guard too.

Here is my quick-redraft of your code, and I added my personal touch to start() since you asked :P

// Copyright [2016] Jean Helou
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import Foundation
import Carbon
import IOKit
import IOKit.usb
import IOKit.hid


struct IOKeyEventMonitorContext {
  var lastSeenSender: String
  init(lastSeenSender: String) {
    self.lastSeenSender = lastSeenSender
  }
}

class IOKeyEventMonitor {
  private let hidManager: IOHIDManager

  let notificationCenter: CFNotificationCenter
  let match: CFMutableDictionary

  var lastActiveKeyboard: String = ""
  var kb2is: [String: TISInputSource] = [String: TISInputSource]()

  private class func createDeviceMatchingDictionary(usagePage: Int, usage: Int) -> CFMutableDictionary {
    let dict = [
      kIOHIDDeviceUsageKey: usage,
      kIOHIDDeviceUsagePageKey: usagePage
    ] as NSDictionary

    return dict.mutableCopy() as! NSMutableDictionary
  }

  init? (usagePage: Int, usage: Int) {
    hidManager = IOHIDManagerCreate(kCFAllocatorDefault, IOOptionBits(kIOHIDOptionsTypeNone))
    notificationCenter = CFNotificationCenterGetDistributedCenter()
    match = IOKeyEventMonitor.createDeviceMatchingDictionary(usagePage: usagePage, usage: usage)
    IOHIDManagerSetDeviceMatching( hidManager, match)
  }

  deinit {
    // FIXME: Find out how to pass nil as an IOKit.IOHIDValueCallback to unregister the callback
    let context = UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())
    // IOHIDManagerRegisterInputValueCallback( hidManager, nil , context)
    CFNotificationCenterRemoveObserver(notificationCenter,
                                       context,
                                       CFNotificationName(kTISNotifySelectedKeyboardInputSourceChanged),
                                       nil)
  }

// MARK: - Funcs:

  func storeInputSource(keyboard: String) {
    let currentSource: TISInputSource = TISCopyCurrentKeyboardInputSource().takeUnretainedValue();
    kb2is[keyboard] = currentSource
  }

  func restoreInputSource(keyboard: String) {
    if let targetIs = kb2is[keyboard] {
      // print("set input source to \(targetIs) for keyboard \(keyboard)")
      TISSelectInputSource(targetIs)
    } else {
      storeInputSource(keyboard: keyboard)
    }
  }

  func onInputSourceChanged() {
    storeInputSource(keyboard: self.lastActiveKeyboard);
  }

  func onKeyboardEvent(keyboard: String) {
    if lastActiveKeyboard != keyboard {
      // print("Active keyboard changed from \(self.lastActiveKeyboard) to \(keyboard)")
      restoreInputSource(keyboard: keyboard)
      lastActiveKeyboard = keyboard
    }
  }

  func start() {
    let context = UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())

    let myHIDKeyboardCallback: IOHIDValueCallback = { (context, ioreturn, sender, value) in

      let
      selfPtr      = Unmanaged<IOKeyEventMonitor>.fromOpaque(context!).takeUnretainedValue(),

      senderDevice = Unmanaged<IOHIDDevice>.fromOpaque(sender!).takeUnretainedValue(),

      vendorId     = String(describing: IOHIDDeviceGetProperty(senderDevice,
                                                               kIOHIDVendorIDKey  as CFString)),
      productId    = String(describing: IOHIDDeviceGetProperty(senderDevice,
                                                               kIOHIDProductIDKey as CFString)),
      product      = String(describing: IOHIDDeviceGetProperty(senderDevice,
                                                               kIOHIDProductKey   as CFString)),
      keyboard     = "\(product)[\(vendorId)-\(productId)]"

      selfPtr.onKeyboardEvent(keyboard: keyboard)
    }

    let inputSourceChanged: CFNotificationCallback = { (center, observer, name, notif, userInfo) in
      let selfPtr = Unmanaged<IOKeyEventMonitor>.fromOpaque(observer!).takeUnretainedValue()
      selfPtr.onInputSourceChanged()
    }

    CFNotificationCenterAddObserver(notificationCenter,
                                    context,
                                    inputSourceChanged,
                                    kTISNotifySelectedKeyboardInputSourceChanged,
                                    nil,
                                    CFNotificationSuspensionBehavior.deliverImmediately)

    IOHIDManagerRegisterInputValueCallback(hidManager, myHIDKeyboardCallback, context)
    IOHIDManagerScheduleWithRunLoop(hidManager, CFRunLoopGetMain(), CFRunLoopMode.defaultMode!.rawValue)
    IOHIDManagerOpen(hidManager, IOOptionBits(kIOHIDOptionsTypeNone))
  }
}

NOTE: I couldn't compile this due to the imports, so I may have deleted a few too many self perhaps in a closure, but I don't think that I did...

The only other thing I would do would be to typealias some of those ridiculously long IO / Carbon types, which would help keep things closer to the left, and make chaining somewhat readable.. For example,

typealias CFNCGDC = CFNotificationCenterGetDistributedCenter
// Or:
typealias CFNCGetDistCenter = CFNotificationCenterGetDistributedCenter
// Or if it is a function:
extension CrazyCarbonClass {
  public func CFNSShortFunc(/* all the prams */) {
    CFNotificationCenterLongAsHellFunc(/* prams */)
  }
}

But that is just my preference.

\$\endgroup\$
4
  • \$\begingroup\$ Hi thanks for the review :) I come from java where IDEs format and indent the code consistently. I tried using the format feature of xcode but it's not as powerful. Since I posted the review I found and used swimat to solve all these. regarding private I did want all the instance variable to be private as I see no reason to expose them. I'll dig in the rest of the remarks to get a better grasp :) \$\endgroup\$
    – Jean
    Nov 30 '16 at 12:17
  • \$\begingroup\$ @jean Yes, Xcode does suck in many regards.. Jetbrains does have one for Swift though, AppCode, but it's Swift support isn't quite as good as it's Java support yet... I use it for formatting / folding alongside Xcode :) \$\endgroup\$
    – Fluidity
    Nov 30 '16 at 13:51
  • \$\begingroup\$ oh, also, if you do check out AppCode, it was only the latest EAP that introduced a bunch of new Swift features.. so I'd pick that one up over the stable channel. \$\endgroup\$
    – Fluidity
    Nov 30 '16 at 13:57
  • 1
    \$\begingroup\$ I haven't applied everything from your review (yet) but thanks again for taking the time. \$\endgroup\$
    – Jean
    Dec 24 '16 at 10:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.