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

Restructure the SiriIntents target and cover it with tests #6203

Open
wtimme opened this issue May 25, 2022 · 0 comments
Open

Restructure the SiriIntents target and cover it with tests #6203

wtimme opened this issue May 25, 2022 · 0 comments
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements

Comments

@wtimme
Copy link
Contributor

wtimme commented May 25, 2022

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

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

@wtimme wtimme added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label May 25, 2022
pixlwave added a commit that referenced this issue Jul 6, 2022
* Add protocol `ContactResolving`

* Let the `IntentHandler` implement `ContactResolving` (#6203)

Nothing has changed about the implementation itself; this prepares
the separation of this logic into a dedicated unit.

* Prepare the separation of the contact resolver from the intents handler (#6203)

* Move the implementation of `ContactResolving` to a dedicated class (#6203)

* Move `ContactResolver` to a dedicated file (#6203)

* Prepare the separation of the `StartAudioCallIntentHandler` from `IntentsHandler` (#6203)

* Move the implementation of `INStartAudioCallIntentHandling` to a dedicated class (#6203)

* Prepare the separation of the `StartVideoCallIntentHandler` from `IntentsHandler` (#6203)

* Move the implementation of `INStartVideoCallIntentHandling` to a dedicated class (#6203)

* Prepare the separation of the `SendMessageIntentHandler` from `IntentsHandler` (#6203)

* Move the implementation of `INSendMessageIntentHandling` to a dedicated class (#6203)

* Remove unused property (#6203)

* Return `nil` if the requested intent cannot be handled (#6203)

* Initialize the intent handlers _after_ everything else is configured (#6203)

In `init()`, there might be some configuration being done that is
required for the handlers.

* Add changelog entry

* Move curly braces in Objective-C to dedicated lines

This ensures that the code follows the style that is present in other Objective-C files.

Co-authored-by: Doug <6060466+pixlwave@users.noreply.github.com>

* Inject the `ContactResolver` into the intent handlers during initialization

In #6365, @pixlwave pointed out that

> If the resolver ever gained a cache or stored properties it would
> help keep the memory usage down in the extension

* Prefer forward-declaration over import in Objective-C header files

Per @pixlwave, this helps prevent build failures:

> We had random cycle errors while building a while back and it was
> eventually solved by removing all imports of `GeneratedInterface-Swift.h`
> in every [Objective-C header] file.

Co-authored-by: Doug <6060466+pixlwave@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

No branches or pull requests

1 participant