Add Authentication Service and first 2 screens for new flow. #6048

pixlwave · 2022-04-20T08:47:48Z

This PR adds an AuthenticationService and RegistrationWizard based on the Android implementations. It additionally adds the Registration screen and Server Selection screen. It also

Renames AuthenticationCoordinator to LegacyAuthenticationCoordinator.
Adds extensions to MXRestClient and MXHTTPClient with async variants of the used methods to help the implementation more closely align with Kotlin's suspending functions.
Adds global colours of ems and white to the themes.

Closes #5161. Closes #5648. Requires matrix-org/matrix-ios-sdk#1443

Registration Screen	Dark Mode	Server Selection Screen	Dark Mode

Registration Errors	Server Selection Errors

4" Device

iPad	iPad

pixlwave · 2022-04-20T10:28:37Z

DesignKit/Source/Colors.swift

+    /// Global color: The color white.
+    var white: ColorType { get }
+
+    /// Global color: The EMS brand's purple colour.
+    var ems: ColorType { get }
+


I'm semi wondering if these should be grouped in a theme.globalColors.ems property or perhaps theme.colors.global.ems.

github-actions · 2022-04-21T12:47:44Z

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/VwEmm3

pixlwave · 2022-04-28T12:22:52Z

Updated screenshot of the server selection screen with Cancel and Confirm buttons.

Anderas

There is quite a lot of code in this PR, which makes it harder to review and see how all the bits fit together. Additionally some of the classes (e.g. the coordinators) are quite large, which makes them harder to understand and maintain in the long run. I wrote some suggestions below of how some of the code could be extracted out and tested.

DesignKit/Source/ColorValues.swift

Riot/Modules/Onboarding/AuthenticationCoordinator.swift

Riot/Modules/Onboarding/OnboardingCoordinatorBridgePresenter.swift

RiotSwiftUI/Modules/Authentication/Common/Service/MatrixSDK/RegistrationFlowHandling.swift

RiotSwiftUI/Modules/Authentication/Registration/AuthenticationRegistrationModels.swift

...wiftUI/Modules/Authentication/Registration/AuthenticationRegistrationViewModelProtocol.swift

RiotSwiftUI/Modules/Authentication/Registration/Test/UI/AuthenticationRegistrationUITests.swift

Anderas

Looking good, just have a few more questions

Riot/Modules/Onboarding/SessionVerificationListener.swift

Riot/Modules/Authentication/LegacyAuthenticationCoordinator.swift

Riot/Modules/Onboarding/AuthenticationCoordinator.swift

Riot/Modules/Onboarding/AuthenticationCoordinatorState.swift

DesignKit/Source/ColorValues.swift

Anderas · 2022-05-04T10:14:22Z

RiotSwiftUI/Modules/Authentication/Registration/AuthenticationRegistrationViewModel.swift

+    // MARK: - Public
+
+    override func process(viewAction: AuthenticationRegistrationViewAction) {
+        Task {


Doesn't Task and await MainActor.run right after equal to just not using any concurrency? Or is the process method simetimes called from non-main thread? I wonder if that should be allowed if it is mean to accept view actions, i.e. actions triggered from the main thread.

This is a side effect of marking completion as @MainActor where you get a compile error unless Swift can see that the call is also coming from the main thread. The right answer here is that process should also be @MainActor but that breaks the protocol conformance, and so without updating the protocol and all of the SwiftUI view models this is the best "on-ramp" onto the MainActor I could come up with.

In reality the View Model (and Coordinator and View) should be annotated @MainActor and then none of the methods/properties would need annotating individually. Would be nice to get this right early on in EIX to avoid async calls like this.

A couple of options that I could do to make this cleaner:

Add a @MainActor private func processOnMain(viewAction: AuthenticationRegistrationViewAction) and call it from here.

Add a MainActorTask/TaskOnMain that hides away the await MainActor.run the removes one level of closure.
Happy to consider either of these (or other ideas) if you think they make sense.

@MainActor private func seems clean enough, though happy to go along the current code for this PR

Perfect, will do that alongside renaming the completions.

...wiftUI/Modules/Authentication/Registration/AuthenticationRegistrationViewModelProtocol.swift

RiotSwiftUI/Modules/Authentication/Registration/Test/UI/AuthenticationRegistrationUITests.swift

Anderas · 2022-05-04T10:36:13Z

RiotSwiftUI/Modules/Authentication/Registration/View/AuthenticationRegistrationScreen.swift

+            VStack(spacing: 0) {
+                header
+                    .padding(.top, OnboardingMetrics.topPaddingToNavigationBar)
+                    .padding(.bottom, 36)


Optional for future PR: might be good to extract these hardcoded padding constants into some internal struct / enum for more control and consistency.

I'm slowing pulling out common constants into OnboardingMetrics, but I have wondered about local constants too. So far my conclusion was that unless some of these constants are shared, then the benefit is small and is no different to entering constraint values into a xib/storyboard. But definitely something I'd like to talk about more.

That is true. The main issue with constants is that it is not whether different values are different deliberately or we just have not considered unifiying them (same goes for font sizes etc). But we can improve this later.

Anderas

Thanks for addressing all the feedback

- Add Registration Screen. - Add Server Selection Screen. - Rename AuthenticationCoordinator to LegacyAuthenticationCoordinator. - Add AuthenticationService and RegistrationWizard. - Async extensions. - Add global white and EMS colors to the themes. - Add tests for server selection and registration screens. - Accessibility and iPad layout tweaks. - Remove MainActor from Auth Coordinators/VMs/Views. (It broke the protocol conformances so now the methods and properties are marked individually.)

Stop using the homeserver from user defaults.

Log errors before throwing. Remove white colour. Remove AuthenticationCoordinatorState added during rebase.

pixlwave commented Apr 20, 2022

View reviewed changes

pixlwave force-pushed the doug/authentication branch 2 times, most recently from e6d4809 to 4774cf7 Compare April 21, 2022 12:18

pixlwave force-pushed the doug/authentication branch from 4774cf7 to 64c4fa9 Compare April 21, 2022 14:09

pixlwave marked this pull request as ready for review April 21, 2022 14:10

pixlwave requested review from a team and Anderas and removed request for a team April 21, 2022 15:23

pixlwave mentioned this pull request Apr 21, 2022

Add AuthenticationService and RegistrationWizard. #6056

Merged

pixlwave force-pushed the doug/authentication branch from 64c4fa9 to ee07144 Compare April 27, 2022 16:38

Anderas reviewed Apr 29, 2022

View reviewed changes

pixlwave requested a review from Anderas May 3, 2022 10:09

Anderas reviewed May 4, 2022

View reviewed changes

pixlwave requested a review from Anderas May 4, 2022 12:26

Anderas approved these changes May 4, 2022

View reviewed changes

pixlwave added 6 commits May 4, 2022 18:36

Fix authentication screens for updated service implementation.

5366f4e

Allow registration on SSO only servers.

56559c9

Stop using the homeserver from user defaults.

Add SessionVerificationListener.

d74c7a1

Remove RegistrationFlowHandling, handling the dummy stage in the wizard.

2dcd941

Address PR comments.

d89da7a

Log errors before throwing. Remove white colour. Remove AuthenticationCoordinatorState added during rebase.

pixlwave force-pushed the doug/authentication branch from 0e86a96 to d89da7a Compare May 4, 2022 17:37

pixlwave merged commit 757244a into develop May 4, 2022

pixlwave deleted the doug/authentication branch May 4, 2022 18:26

This was referenced May 4, 2022

Fix UI tests action and Auth test. #6119

Merged

Rename completion to callback and simplify actor usage. #6141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Authentication Service and first 2 screens for new flow. #6048

Add Authentication Service and first 2 screens for new flow. #6048

pixlwave commented Apr 20, 2022 •

edited

Loading

pixlwave Apr 20, 2022

github-actions bot commented Apr 21, 2022 •

edited

Loading

pixlwave commented Apr 28, 2022

Anderas left a comment

Anderas left a comment

Anderas May 4, 2022

pixlwave May 4, 2022

pixlwave May 4, 2022 •

edited

Loading

Anderas May 4, 2022

pixlwave May 4, 2022

Anderas May 4, 2022

pixlwave May 4, 2022

Anderas May 4, 2022

Anderas left a comment

Add Authentication Service and first 2 screens for new flow. #6048

Add Authentication Service and first 2 screens for new flow. #6048

Conversation

pixlwave commented Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 21, 2022 • edited Loading

pixlwave commented Apr 28, 2022

Anderas left a comment

Choose a reason for hiding this comment

Anderas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixlwave May 4, 2022 • edited Loading

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

Anderas left a comment

Choose a reason for hiding this comment

pixlwave commented Apr 20, 2022 •

edited

Loading

github-actions bot commented Apr 21, 2022 •

edited

Loading

pixlwave May 4, 2022 •

edited

Loading