element-hq / element-hq/element-ios
Restructure the `SiriIntents` target and cover it with tests
- Dominant language
- Swift
- Stars
- 1.8k
- Forks
- 544
- PR merge metrics
- PR metrics pending
Description
### Your use case
#### What would you like to do?
The code that handles the Intents (the "Siri" requests) is not covered by unit tests. I want to change that, in order to be able to properly resolve bugs like #4860, and add new features.
Once the functionality is covered by tests, we can more easily
- migrate the target to Swift
- resolve the deprecation warnings (e.g. "`INStartAudioCallIntent` is deprecated: first deprecated in iOS 13.0 - `INStartAudioCallIntent` is deprecated. Please adopt `INStartCallIntent` instead")
- add more intents (e.g. `INShareFocusStatusIntent` for #4908)
#### Why would you like to do it?
First of all, I was working on a similar assignment during my day-job. Back then, we only tested the Siri functionality manually, which (especially during that time when Siri was still in Beta) can get quite tedious. So I have some experience with this sort of refactoring. I also like the fact that I get to work with "Apple technology" such as Siri, which is always refreshing.
In addition, I am very certain that the code of the extension will be re-used for "Element-X", so my effort will not be for nothing once the original "Element iOS" (_this_ repository) is deleted.
#### How would you like to achieve it?
##### Splitting up the `IntentHandler.{h,m}`
First of all, I suggest we split up the large `IntentHandler.{h,m}` into several smaller files:
- `IntentHandler`: `handlerForIntent:` and references to the intent handlers
- `StartAudioCallIntentHandler`, `StartVideoCallIntentHandler` and `SendMessageIntentHandler`: Contain the intent-specific code
- `PersonResolver`: Contains the shared `resolveContacts:withCompletion:` method
##### Adding tests
In order for the code to be in a state that allows for unit testing, the dependencies (such as the `MXKAccountManager`) need to be mocked, so that during the tests, their behaviour can be controlled. Ideally, the dependencies implement protocols (e.g. `MXKAccountManaging`) so that during initialization of the `IntentHandling` classes, we can pass mocked implementations.
### Have you considered any alternatives?
An alternative could be rewriting the thing from scratch, this time in Swift. However, even then, one would probably orient themself on the old Objective-C implementation. Splitting up the code, covering it with tests and only _then_ rewriting it bears the least risk, since it does not break existing functionality.
### Additional context
_No response_
Contributor guide
Research direction
Start by reading IntentHandler.{h,m} and tracing the current Siri intent handling. The work is done when the named intent handlers and PersonResolver are split out, dependencies can be mocked through protocols, and the Siri functionality has unit-test coverage.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- ios, objective-c, swift
- Domain
- mobile-dev, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100