SwiftUI Version of Room Notification Settings V2

[langleyd]

This PR introduces a SwiftUI implementation of Room Notification Settings V2

What's in this PR

• SwiftUI Theme support
  • VectorContentModifier a SwiftUI modifier that can be used to apply any application level configuration at the top of the SwiftUI hierarchy.
  • VectorHostingViewController hosts a SwiftUI view and applies theming to any container NavigationController and applies the VectorContentModifier.
• SwiftUI AvatarService
  • AvatarSize an enum to match the sizes in Design System. (Size naming TBD).
  • AvatarService a simple api that retrieves and caches avatars. Also a mock service.
  • VectorAvatarView a view for rendering avatars consistently by AvatarSize.
• Implements Room Notification Settings
  • Shares the RoomNotificationSettingsViewModel with the UIKit implementation. This is generally a bit awkward with with the @available attribute. @available can't be used with stored properties for example. For the moment we plan on using SwiftUI for new features so won't face this problem in the general case. If we do find more areas we are trying to support both UIKit + SwiftUI and avoid code duplication we may want to look into this further.
  • Originally implemented the feature using SwiftUI List component which is backed by UITableView. List does not give you access to style all parts of the tableview like the background, nor does it give you a reference to the underlying UITableVIew. To implement the avatar header for example I would have had to put the header inside a Section with no content in order to get the grouped UITableView to style correctly. Rather than fighting it I decided to use a VStack which gives you full control but you have to do a bit more of the styling yourself. You can mitigate this by creating your own styles. I think in future this is probably give us more re-use and better flexibility.
  • Using mocked dependencies in our ViewModel (previously done to make view models more testable and decoupled) had the nice side-effect that we can always run our live preview without a network connection/MxSession.

Update 2021-9-17:

• Added an AvatarViewModel owned by the AvatarView via a @StateObject so that only mxContentUri is needed as input to the view.
• To do this dependency management was added via DependencyContainer and the EnvironmentKey DependencyContainerKey
• Inject property wrapper, Injectable protocol and InjectableObject were also added to remove some boilerplate.
• To simplify things further I removed the dependency on AvatarGenerator and we just render that part in SwiftUI.

Screenshots

Todos

• Disable BuildSetting
• Remove Podfile changes
• Fix live theme updating.
• Fix padding below avatar
• Use private on navigation buttons in RoomNotificationSettingsView
• Fix avatar service not returning generated image on network error
• Add progress spinner during save.
• Clear warnings for Clear easy Xcode warnings in Modules #4677
• Add Dependency Management and AvatarViewModel
• Do avatar placeholder rendering in SwiftUI rather then AvatarGenerator

resolves #4669

[pixlwave]

This looks really great! So good how much the VStack looks and feels like a List 😄. Sorry I added quite so many comments. I think it would be nice to have a few more doc comments (autocompletion hints for the new view types would be super helpful going forward).

Two general points I'm wondering about that are worth discussing tomorrow:

• Should we have a naming convention to distinguish SwiftUI views from UIViews? For example following the base names like VectorForm, VectorAvatarImage and FormSectionFooterText?
• It might be worth making our view text properties let text: LocalizedStringKey to work with SwiftUI's i18n features down the road - genuinely no idea if that would ever be useful or if it would impact in any way right now.

[langleyd]

+1 @pixlwave regarding naming. Also something I'd like to discuss. Also things like DesignKit and what ends up going in there. Main reason I had VectorAvatarImage was because there was already an AvatarView in out UIKit code. But yea would be good to discuss good practices/conventions in this regard. And thanks for the comments, keep them coming! 😄

I think if we start using LocalizedStringKey it will look for strings in resources with that key which wont work with the current SwiftGen approach (Isn't using english as the key). But yea probably a few SwiftUI features like that we can discuss.

[pixlwave]

Updates look good to me. Wondering if it should be VectorHostingController to match UIHostingController now I've seen it written in Manu's comment above?

[langleyd]

👀 at build errors

[langleyd]

Ok looks like build is fixed, test and alpha failed due to timeouts.

[github-actions]

📱 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/VeWcc3

[manuroe]

This injection module is very nice!

I wonder if we should separate pure SwiftUI code and UIKit code right now. I am afraid that we will mix or miss things if we continue to put everything into Modules. The RoomNotificationSettings code is hybrid and can stay there but pure SwiftUI code should not be not mixed with UIKit.

Options:

• keep current Modules and SwiftUIModules folders as the same level
• add another layer like Modules/UIKit with current code and Modules/SwiftUI
• or ...

[pixlwave]

Looking good, especially the dependency injection stuff! More thoughts on naming 🙄

• Injectable vs InjectableObject are very close in meaning. When I see Injectable by itself, it's hard to reason about whether it's to be injected or can receive injections. But I'm struggling to think of something better so might simply be something to learn.
• I was clearly wrong suggesting never using View suffix at all. I think it still makes sense for reusable components, but RoomNotificationSettingsView was probably clearer for the entire composition? 😕

[langleyd]

@pixlwave and I went over apple usage and seems they use View when removing it would cause confusion, so will go with that as a rule of thumb for the moment.

@pixlwave I had the exact same thoughts regarding Injectable. I will review some other DI tooling and find a better match. Will update in a separate PR i think when I iterate on this stuff a bit.

[pixlwave]

All looks good from my perspective. That switch on the view model is wayyyy cleaner to read too 😄