element-hq / element-hq/element-ios

Restructure the `SiriIntents` target and cover it with tests

Open
#6,203 0 comments 0 reactions 0 assignees View on GitHub
T-Enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.