Target `SiriIntents`: Split `IntentHandler` into smaller files (#6203) #6365

wtimme · 2022-06-30T19:36:07Z

Introduction

This branch splits the IntentHandler and moves the functionality into dedicated classes.
This makes the code easier to read and maintain, and prepares the structure to add unit tests.

The intent handlers depend on ContactResolving, a protocol for what was previously a private method. By moving the shared functionality to this dedicated class, the dependency can later be injected and mocked for unit tests. As for the implementation of the handlers, I have merely moved the code to another place.

Altogether, this restructuring brings the number of lines for IntentHandler down from 450 to 92.

Signed-off-by: Wolfgang Timme element-signoff@milchbartstrasse.de

Pull Request Checklist

I read the contributing guide
UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
Accessibility has been taken into account.

Pull request is based on the develop branch

Pull request contains a changelog file in ./changelog.d
You've made a self review of your PR
Pull request includes screenshots or videos of UI changes
Pull request includes a sign off

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

…er (#6203)

…6203)

…entsHandler` (#6203)

…cated class (#6203)

…entsHandler` (#6203)

…cated class (#6203)

…sHandler` (#6203)

…ed class (#6203)

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

codecov-commenter · 2022-06-30T20:21:20Z

Codecov Report

Merging #6365 (6e761f8) into develop (730eafc) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #6365      +/-   ##
===========================================
+ Coverage     6.19%    6.21%   +0.02%     
===========================================
  Files         1440     1444       +4     
  Lines       155105   155530     +425     
  Branches     62362    62500     +138     
===========================================
+ Hits          9608     9672      +64     
- Misses      145093   145453     +360     
- Partials       404      405       +1

Impacted Files	Coverage Δ
...SplashScreen/OnboardingSplashScreenViewModel.swift	`11.53% <0.00%> (-42.31%)`	⬇️
...les/Common/UserIndicators/UserIndicatorStore.swift	`27.50% <0.00%> (-20.33%)`	⬇️
...Modules/Common/ViewModel/StateStoreViewModel.swift	`30.55% <0.00%> (-19.45%)`	⬇️
...dules/Common/SwiftUI/VectorHostingController.swift	`71.87% <0.00%> (-8.13%)`	⬇️
...Common/UserIndicators/UserIndicatorPresenter.swift	`8.88% <0.00%> (-4.45%)`	⬇️
...ing/SplashScreen/View/OnboardingSplashScreen.swift	`54.66% <0.00%> (-4.00%)`	⬇️
Riot/Managers/Settings/RiotSettings.swift	`87.82% <0.00%> (-2.36%)`	⬇️
Riot/Modules/Application/AppCoordinator.swift	`51.67% <0.00%> (-1.60%)`	⬇️
Riot/Modules/TabBar/TabBarCoordinator.swift	`30.24% <0.00%> (-1.24%)`	⬇️
Riot/Modules/SplitView/SplitViewCoordinator.swift	`34.64% <0.00%> (-1.05%)`	⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 730eafc...6e761f8. Read the comment docs.

pixlwave

Looking good to me, thanks for the PR. A couple of style comments and a question inline, otherwise nothing major.

Altogether, this restructuring brings the number of lines for IntentHandler down from 450 to 92.

Niiice 😎

pixlwave · 2022-07-01T14:40:09Z

SiriIntents/ContactResolver/ContactResolver.m

+@implementation ContactResolver
+
+- (void)resolveContacts:(nullable NSArray<INPerson *> *)contacts
+         withCompletion:(void (^)(NSArray<INPersonResolutionResult *> * _Nonnull))completion {


Our objc style is to have { on the next line.

Suggested change

withCompletion:(void (^)(NSArray<INPersonResolutionResult *> * _Nonnull))completion {

withCompletion:(void (^)(NSArray<INPersonResolutionResult *> * _Nonnull))completion

{

Thanks for pointing that out! Nowadays, I do not spend time thinking about code formatting, since the projects that I am working on usually have automatic code formatting set up. Maybe that's something that could be incorporated into this repository, even though the Objective-C code will likely be discarded once Element-X sees mass adoption.

Yeah that would be nice. We're currently adding swiftformat to ElementX which should make life much better over there!

BTW, I don't think this one was resolved by your last commit.

SiriIntents/IntentHandlers/SendMessage/SendMessageIntentHandler.m

pixlwave · 2022-07-01T14:43:19Z

SiriIntents/IntentHandlers/SendMessage/SendMessageIntentHandler.m

+
+- (instancetype)init {
+    if (self = [super init]) {
+        _contactResolver = [[ContactResolver alloc] init];


Do you think it makes sense to have multiple contact resolvers, or might it be an idea to create one in IntentHandler to pass into the initialisers?

Thanks for the question! There are three options that I see:

Let every intent handler have their own instance of ContactResolver (as is the case in this branch right now)

Let the IntentHandler create one single instance and pass it to the intent handlers during initialization. The intent handlers keep a strong reference, so the IntentHandler does not need to manage the instance of ContactResolver

Add a singleton property (ContactResolver.shared) to ContactResolver

What is your preference? Is there a fourth option that is even better? Thanks in advance for your input!

I would lean towards option 2 in this instance. If the resolver ever gained a cache or stored properties it would help keep the memory usage down in the extension (very early optimisation, I know 😅).

SiriIntents/IntentHandlers/StartAudioCall/StartAudioCallIntentHandler.m

SiriIntents/IntentHandlers/StartVideoCall/StartVideoCallIntentHandler.m

wtimme · 2022-07-02T21:11:51Z

Do you have an idea why the SonarCloud analysis might be failing, @pixlwave? I had a look at the coverage reports, but they indicate a decrease in files that my commits did not touch. 🤷

pixlwave

Do you have an idea why the SonarCloud analysis might be failing, @pixlwave? I had a look at the coverage reports, but they indicate a decrease in files that my commits did not touch. 🤷

Ah don't worry about those. SonarCloud isn't configured for external repos so it fails due to a missing secret. And the test coverage is new on EI - from what I can guess, it fluctuates based on how long the splash screen is shown for whilst the tests are running.

pixlwave · 2022-07-04T16:58:16Z

SiriIntents/IntentHandlers/SendMessage/SendMessageIntentHandler.m

+
+- (instancetype)init {
+    if (self = [super init]) {
+        _contactResolver = [[ContactResolver alloc] init];


I would lean towards option 2 in this instance. If the resolver ever gained a cache or stored properties it would help keep the memory usage down in the extension (very early optimisation, I know 😅).

pixlwave · 2022-07-04T17:00:03Z

SiriIntents/ContactResolver/ContactResolver.m

+@implementation ContactResolver
+
+- (void)resolveContacts:(nullable NSArray<INPerson *> *)contacts
+         withCompletion:(void (^)(NSArray<INPersonResolutionResult *> * _Nonnull))completion {


Yeah that would be nice. We're currently adding swiftformat to ElementX which should make life much better over there!

BTW, I don't think this one was resolved by your last commit.

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>

@pixlwave

…zation 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

wtimme · 2022-07-05T10:54:18Z

BTW, I don't think this one was resolved by your last commit.

My bad, I am still getting used to batch-commits in GitHub's pull request process.

I would lean towards option 2 in this instance.

I have adjusted the code accordingly.

Let me know if there's anything else I can do to further this pull request.

pixlwave

Thanks for the updates. I've added one last comment that I missed during the previous review, otherwise I'm happy to merge this.

SiriIntents/IntentHandlers/SendMessage/SendMessageIntentHandler.h

@pixlwave

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.

pixlwave

LGTM, thanks for the contribution 😎

wtimme added 15 commits June 30, 2022 21:13

Add protocol ContactResolving

2c44e1b

Let the IntentHandler implement ContactResolving (#6203)

0fda5d7

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 handl…

02a56db

…er (#6203)

Move the implementation of ContactResolving to a dedicated class (#…

f414324

…6203)

Move ContactResolver to a dedicated file (#6203)

c21e8e3

Prepare the separation of the StartAudioCallIntentHandler from `Int…

0880e91

…entsHandler` (#6203)

Move the implementation of INStartAudioCallIntentHandling to a dedi…

6977afb

…cated class (#6203)

Prepare the separation of the StartVideoCallIntentHandler from `Int…

6abb4ff

…entsHandler` (#6203)

Move the implementation of INStartVideoCallIntentHandling to a dedi…

6ca57b6

…cated class (#6203)

Prepare the separation of the SendMessageIntentHandler from `Intent…

52114cc

…sHandler` (#6203)

Move the implementation of INSendMessageIntentHandling to a dedicat…

4507b66

…ed class (#6203)

Remove unused property (#6203)

e4b12b9

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

3a3b1a9

Initialize the intent handlers _after_ everything else is configured (#…

b6ff75b

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

Add changelog entry

5a63be1

pixlwave self-requested a review July 1, 2022 14:26

pixlwave requested changes Jul 1, 2022

View reviewed changes

pixlwave reviewed Jul 4, 2022

View reviewed changes

wtimme and others added 2 commits July 5, 2022 12:45

Move curly braces in Objective-C to dedicated lines

3ac7f0a

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 initiali…

4cf16e9

…zation 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

pixlwave reviewed Jul 5, 2022

View reviewed changes

SiriIntents/IntentHandlers/SendMessage/SendMessageIntentHandler.h Outdated Show resolved Hide resolved

pixlwave approved these changes Jul 6, 2022

View reviewed changes

pixlwave merged commit 2babca6 into element-hq:develop Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target `SiriIntents`: Split `IntentHandler` into smaller files (#6203) #6365

Target `SiriIntents`: Split `IntentHandler` into smaller files (#6203) #6365

wtimme commented Jun 30, 2022

codecov-commenter commented Jun 30, 2022 •

edited

Loading

pixlwave left a comment

pixlwave Jul 1, 2022

wtimme Jul 2, 2022

pixlwave Jul 4, 2022

pixlwave Jul 1, 2022

wtimme Jul 2, 2022

pixlwave Jul 4, 2022

wtimme commented Jul 2, 2022

pixlwave left a comment

pixlwave Jul 4, 2022

pixlwave Jul 4, 2022

wtimme commented Jul 5, 2022

pixlwave left a comment

pixlwave left a comment

	withCompletion:(void (^)(NSArray<INPersonResolutionResult > _Nonnull))completion {
	withCompletion:(void (^)(NSArray<INPersonResolutionResult > _Nonnull))completion
	{

Target SiriIntents: Split IntentHandler into smaller files (#6203) #6365

Target SiriIntents: Split IntentHandler into smaller files (#6203) #6365

Conversation

wtimme commented Jun 30, 2022

Introduction

Pull Request Checklist

codecov-commenter commented Jun 30, 2022 • edited Loading

Codecov Report

pixlwave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtimme commented Jul 2, 2022

pixlwave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtimme commented Jul 5, 2022

pixlwave left a comment

Choose a reason for hiding this comment

pixlwave left a comment

Choose a reason for hiding this comment

Target `SiriIntents`: Split `IntentHandler` into smaller files (#6203) #6365

Target `SiriIntents`: Split `IntentHandler` into smaller files (#6203) #6365

codecov-commenter commented Jun 30, 2022 •

edited

Loading