Add Swift refinements to several classes

[avery-pierce]

This pull request adds Swift language refinements to the APIs in MXRestClient and MXSession.

NOTE: Swift requires iOS 8.0 or above. Therefore this PR breaks compatibility with iOS 7.0.

Summary:

• APIs with success+failure callbacks are simplified into a single callback that handles both cases (more on this technique below)
• URLs are typed as URL instead of String, where possible.
• "Stringly-typed" enums (i.e. parameters that expect one of several string constants, like kMXLoginFlowTypePassword) are now available as Swift enums. Situations where a custom constant may be provided are handled with a .custom(String) case (or similar).
• This PR should have no impact on the Obj-C API

before

let addressUrlString = "http://127.0.0.1:8008"
let client = MXRestClient(homeServer: addressUrlString, andOnUnrecognizedCertificateBlock: nil)
_ = client?.login(withLoginType: kMXLoginFlowTypePassword, username: "alice", password: "securePassword", success: { (credentials) in

    // Stop the loading animation
    self.progressIndicator.stopAnimation(nil)
    let credentials = credentials!

    // Cache the credentials for later, and complete the setup process...

}, failure: { (error) in

    // Stop the loading animation
    self.progressIndicator.stopAnimation(nil)
    let error = error!

    // An error has occurred...
    print("An error has occurred:", error)
})

Issues

1. The MXRestClient initializer returns an optional.
2. Swift expects the programmer to handle the MXHTTPOperation returned from login(). If the return value is unused, the compiler will throw a warning unless the programmer explicitly ignores it using the _ = syntax.
3. Multiple callbacks in a single method is not very Swifty.
4. Swift interprets the values passed into the callbacks as optionals (even though the SDK should never return nil)
5. Cleanup code (eg. stop a loading spinner) must be written twice – not very DRY.

after

let addressUrl = URL(string: "http://127.0.0.1:8008")!
let client = MXRestClient(homeServer: addressUrl, unrecognizedCertificateHandler: nil)
client.login(type: .password, username: "alice", password: "securePassword") { response in

    // Stop the animation
    self.progressIndicator.stopAnimation(nil)

    switch response {
    case .success(let credentials):
        // Cache the credentials for later, and complete the setup process...
        break

    case .failure(let error):
        // An error has occurred...
        print("An error has occurred:", error)
        break
    }
}

Improvements

1. MXRestClient initializer no longer returns an optional
2. The API uses the @discardableresult attribute, which permits the programmer to ignore the return value by default (the API still returns a HTTPOperation, if it's desired)
3. type: is now an enum, enabling more concise usage. (In fact, .password is the default value, so that parameter could be omitted altogether)
4. A single callback handles both success and failure cases in a type-safe yet flexible way (see below). Swift's trailing closure syntax makes more sense in this case.
5. Cleanup code is only written once.

Handling Callbacks in Swift: MXResponse<T>

public enum MXResponse<T> {
    case success(T)
    case failure(Error)

    /* ... */
}

This enum enables Swift to return a single value that encapsulates both success and error conditions. The associated type for each case (either T or Error) is guaranteed to be non-nil.

Usage:

Use a switch statement to handle both a success and an error:

mxRestClient.publicRooms { response in
    switch response {
    case .success(let rooms):
        // Do something useful with these rooms
        break

    case .failure(let error):
        // Handle the error in some way
        break
    }
}

Silently ignore the failure case:

mxRestClient.publicRooms { response in
    guard let rooms = response.value else { return }
    // Do something useful with these rooms
}

Signed-off-by: Avery Pierce aapierce0@gmail.com

[manuroe]

Running pod lib lint on your branch fails. There is no issue when running it on develop.
Could you please check it?

[avery-pierce]

It passes validation for me.

This is from a fresh clone of aapierce0/matrix-ios-sdk.git after pod install:

bash-3.2$ git remote -v
origin	https://aapierce0@github.com/aapierce0/matrix-ios-sdk.git (fetch)
origin	https://aapierce0@github.com/aapierce0/matrix-ios-sdk.git (push)
bash-3.2$ git status
On branch swiftyAPI
Your branch is up-to-date with 'origin/swiftyAPI'.
nothing to commit, working tree clean
bash-3.2$ git log -1
commit 335d1e5b006df84810d787f581cc46119b1d1370
Author: Avery Pierce <aapierce0@gmail.com>
Date:   Mon Feb 20 09:04:32 2017 -0600

    Swift: Move all .swift files to a dedicated directory - MatrixSDK/Contrib/Swift
bash-3.2$ pod lib lint

 -> MatrixSDK (0.7.7)

MatrixSDK passed validation.

Does pod lib lint give any description of the errors for you? If so, could you send them to me so I can take a look?

[manuroe]

Failures are not in the swift files themselves:

Emmanuels-MBP:matrix-ios-sdk-swift manu$ git status
On branch master
Your branch is ahead of 'origin/master' by 77 commits.
  (use "git push" to publish your local commits)
nothing to commit, working tree clean
Emmanuels-MBP:matrix-ios-sdk-swift manu$ git log -1
commit 335d1e5b006df84810d787f581cc46119b1d1370
Author: Avery Pierce <aapierce0@gmail.com>
Date:   Mon Feb 20 09:04:32 2017 -0600

    Swift: Move all .swift files to a dedicated directory - MatrixSDK/Contrib/Swift
Emmanuels-MBP:matrix-ios-sdk-swift manu$ pod lib lint

 -> MatrixSDK (0.7.7)
    - ERROR | xcodebuild: Returned an unsuccessful exit code. You can use `--verbose` for more information.
    - NOTE  | xcodebuild:  <module-includes>:1:9: note: in file included from <module-includes>:1:
    - NOTE  | [iOS] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release-iphonesimulator/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:32:9: note: in file included from /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release-iphonesimulator/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:32:
    - ERROR | [iOS] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release-iphonesimulator/MatrixSDK/MatrixSDK.framework/Headers/MXOlmInboundGroupSession.h:23:9: error: include of non-modular header inside framework module 'MatrixSDK.MXOlmInboundGroupSession'
    - NOTE  | [iOS] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release-iphonesimulator/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:36:9: note: in file included from /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release-iphonesimulator/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:36:
    - ERROR | [iOS] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release-iphonesimulator/MatrixSDK/MatrixSDK.framework/Headers/MXCryptoStore.h:25:9: error: include of non-modular header inside framework module 'MatrixSDK.MXCryptoStore'
    - NOTE  | xcodebuild:  <unknown>:0: error: could not build Objective-C module 'MatrixSDK'
    - NOTE  | [OSX] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:32:9: note: in file included from /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:32:
    - ERROR | [OSX] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release/MatrixSDK/MatrixSDK.framework/Headers/MXOlmInboundGroupSession.h:23:9: error: include of non-modular header inside framework module 'MatrixSDK.MXOlmInboundGroupSession'
    - NOTE  | [OSX] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:36:9: note: in file included from /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release/MatrixSDK/MatrixSDK.framework/Headers/MatrixSDK-umbrella.h:36:
    - ERROR | [OSX] xcodebuild:  /Users/manu/Library/Developer/Xcode/DerivedData/App-cvhjkhxrsygveiegazgexywqpzoq/Build/Products/Release/MatrixSDK/MatrixSDK.framework/Headers/MXCryptoStore.h:25:9: error: include of non-modular header inside framework module 'MatrixSDK.MXCryptoStore'

[!] MatrixSDK did not pass validation, due to 5 errors.
[!] The validator for Swift projects uses Swift 3.0 by default, if you are using a different version of swift you can use a `.swift-version` file to set the version for your Pod. For example to use Swift 2.3, run: 
    `echo "2.3" > .swift-version`.
You can use the `--no-clean` option to inspect any issue.

Do you use a .swift-version file?

[avery-pierce]

No, I don't use a .swift-version file.

That error message looks familiar though. It's complaining about #import <OLMKit/OLMKit.h> in some of the headers.

As a quick sanity check, could you clean the build folder from Xcode and try again? (option+shift+⌘+K, or hold option and select Product > Clean Build Folder)

I think the last time this came up for me, I resolved it by changing the scope of the problem header from public to project. The current branch should have those headers scoped correctly, but you may have a stale build hanging around in your derived data directory (that has happened to me before too).

[manuroe]

The Xcode shortcut and UI for cleaning the build folder did not work for me. I had to remove the folder by hand.
Anyway, pod lib lint now works.

I have another issue: I cannot run unit tests.
I have this error when trying to launch them:

Does it work on your side?

[avery-pierce]

Yes, I'm getting that error message too.

I'm looking at this right now: http://stackoverflow.com/questions/32223965/xcode-7-code-coverage-data-generation-failed

I'll let you know if I find a way to resolve it.

[manuroe]

I saw this SO post too but I did not want to add cocoapod scripts.
I tried to remove code coverage check from the project but with no success.

[manuroe]

Copyright is not the same as in other files.

[manuroe]

MXRestClient tries to be as close to the matrix CS API as possible. The username is optional in the CS API which is translated into a nullable parameter in this class.

[manuroe]

The push rule API will be updated in short/mid terms. It does not worth to start a refactoring now.

[manuroe]

presence is a string because the spec is open on possible values. Some use "unavailable" for example.

[avery-pierce]

The testing issue should be resolved now. By adding a swift file to the MatrixSDKTests bundle, Xcode automatically generated a Bridging Header file and changed some project settings.

[manuroe]

Sorry, it still does not work. I tried on 2 different macs.
Did your changes only affect your workspace? So that it is has not effect when creating a new xcworkspace from pod install.

[avery-pierce]

It definitely changed something in MatrixSDK.xcodeproj/project.pbxproj, but that's evidently not enough to resolve the issue by itself.

It can't be solely the workspace either. I can delete the workspace file, do a pod install, and it still works. 😵

Anyways, I've tried several scenarios now, and it looks like simply having a swift file in the test bundle is enough to get Xcode and/or Cocoapods to run the tests consistently.

[manuroe]

Thanks. It works.
I am doing some tests on your branch.

[manuroe]

Do you know a project with a pod that exposes both objc and swift api?
I did not find one.

My problem is that with this modified podspec file, when you do a pod install in the riot-ios source folder, CocoaPods considers the matrix-ios-sdk as a swift pod and asks to change the riot-ios/Podfile (to add use_frameworks!).
Then, Xcode asks for updating some things because there is a swift project into the workspace. Then, it fails to build...
This is quite boring for full objc apps.

I am wondering if it would be better to have 2 pods. MatrixSDK, the full objc pod as we have now and SwiftMatrixSDK, a copy of your MatrixSDK.podspec.
But again, I do not find a project with such approach.

What do you think?

[avery-pierce]

I'm not opposed to the idea of having a second pod that targets Swift specifically, but it should be possible to publish a pod that cleanly supports both languages. I can't think of any other projects that take a dual-pod approach like this.

I'm going to fork riot-ios to see if I can get a feel for what the issue is.

[avery-pierce]

Okay, I was eventually able to get riot-ios to compile with the use_frameworks! setting in the podfile, but I had to break a few things to get to that point. I think all of the problems are caused by differences between Frameworks and Libraries. Frameworks are more strict about what you can and can't do.

I believe all of the issues I encountered are solvable in a reasonable amount of time. My recommendation is to keep a single pod architecture. You could leave this PR open until the downstream issues are resolved, close it without merging, or pull it into a separate branch (called "Swift" or "Frameworks", or something). In the meantime, if users want to develop a client in Swift, they can reference my fork directly (or the "Swift" branch of this repository, if you choose to go that route).

Read on for a summary of the issues I encountered.

Google Analytics in MatrixSDK

I think this was the most confusing one. Here's the error message:

Undefined symbols for architecture i386:
  "_OBJC_CLASS_$_GAIDictionaryBuilder", referenced from:
      objc-class-ref in MXSession-AE322CA77250B862.o
  "_OBJC_CLASS_$_GAI", referenced from:
      objc-class-ref in MXSession-AE322CA77250B862.o
ld: symbol(s) not found for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I think this is caused by the Framework trying to initialize a class from a header file it doesn't own. The Google Analytics classes are available in the runtime (they're loaded in Riot), but they aren't linked directly to the framework. I got around this by using NSClassFromString() to reference GAI and GAIDictionaryBuilder.

A better solution might be to create a MXAnalyticsDelegate protocol that exposes these events to the client app (riot-ios, or some other app). This way you're not prescribing Google Analytics to other users of the SDK.

GHMarkdownParser - MMIOT conflict

(see also: matrix-org/matrix-ios-kit#188)

The discount library used by GHMarkdownParser has two separate definitions of MMIOT. One is typedef void MMIOT, and the other is typedef struct mmiot { /* ... */ } MMIOT. Evidently this is allowed in Obj-C Libraries, but not Frameworks. 🤔

I don't know how to get around this one other than to remove GHMarkdownParser dependency from the MatrixKit podspec, and remove its usage from the project. There are other markdown parsers out there. You could probably find another one that will build as a Framework.

Ambiguous Reference to ReadReceiptAlignment

The compiler thinks that the ReadReceiptAlignment enum is defined twice, even though both instances are the same line of code. What's actually happening is that the file is being imported twice.

Here's how to fix it: Some of the classes in riot-ios inherit from classes in MatrixKit (e.g. RoomOutgoingTextMsgBubbleCell inherits from MXKRoomOutgoingTextMsgBubbleCell). In those header files, the superclass is imported directly with #import "MXKRoomOutgoingTextMsgBubbleCell.h". If you #import <MatrixKit/MatrixKit.h> instead, the compiler won't double-import the header that contains ReadReceiptAlignment.

MXJingleCallStack

This is similar to the Google Analytics issue above. The MatrixSDK is depending on a framework it doesn't own (WebRTC). This could be resolved by moving the MXJingleCallStack out of MatrixSDK and either implement it in riot-ios directly (which actually imports Google WebRTC), or possibly spin it off into its own pod that clients can import if they desire.

[manuroe]

Thanks for this report. This is even worse than I saw at first.

It looks like that swift wins over objc in a dual pod approach :/.
It barely works for objc apps as it creates restrictions in their pods dependencies and in the code to use them. This is really annoying to break developers freedom in their choice for pods. This is probably why we cannot find such mixing pod.

I agree that we could get Riot build with this dual pod but Riot is not the only app using this sdk. The sdk was written in objc to make it usable in as many projects as possible.

It looks like to me that having 2 pods could be a quick win:

• it does not break the existing pod for objc projects.
• swift and objc codes live in the same repo which makes maintenance much more easier.
  Before MXJingleCallStack you mentioned, we used OpenWebRTC for VoIP. I put the implementation in a separate pod but it was a pain to maintain. It created useless effort to keep all libs in sync. Release was also boring.
• we are almost there.

[avery-pierce]

Okay, you make a good point. The two pod approach is fine.

However, the above issues will probably need to be addressed at some point. For example, the pod is currently incompatible with Google Analytics when use_frameworks! is enabled, regardless of language (Obj-C works around this limitation because it supports Libraries in addition to Frameworks. Swift doesn't support Libraries). Example here.

I don't know much about publishing pods. How would you create a second pod from this repository? Is it just a matter of updating the existing podspec file with the second pod description?

[manuroe]

I would go with a 2nd podspec file called like SwiftMatrixSDK.podspec, which will be a copy of your current Matrix.podspec. You will probably need to change the pod name in the file but pod lib lint SwiftMatrixSDK.podspec will guide you.

Cocoapods manages several podspec files in the same folder very well.

[manuroe]

A huge thanks for this awesome work!