search

thunderbird / thunderbird-android

Thunderbird for Android – Open Source Email App for Android (fka K-9 Mail)
https://thunderbird.net/mobile
Apache License 2.0
11.05k stars 2.51k forks source link

Use brand name in `AppTitleTopHeader` #8332

Closed cketti closed 1 month ago

cketti commented 1 month ago

Add BrandNameProvider to provide a brand name in addition to the app name. E.g. even when the app name is "Thunderbird Beta" the brand name is "Thunderbird".

The brand name is then used in AppTitleTopHeader to implement one part of the change outlined in https://github.com/thunderbird/thunderbird-android/issues/8257#issuecomment-2413607685

This also changes the code to pass BrandNameProvider directly to AppTitleTopHeader. That way a similar change like this in the future can be accomplished by changing a lot fewer places.

cketti commented 1 month ago

I think we need to figure out a better way to allow deeply nested injections within composables. Koin doesn't provide a good way to allow deep injections yet. This new approach is harder to debug and can lead to silent failures in preview rendering if not used with the correct wrapper.

I don't think this approach is harder to debug. With this approach one could introduce preview failures. They are silent in that you won't know about them until you try to view the preview. However, the preview failure itself is reported by Android Studio and it's only a few clicks to get to the stack trace that allows finding what caused the failure. Previews are nice to have, but not critical to the app. So I think the occasional failure is more than acceptable.

Keep in mind that Koin is not compile time safe and requires additional effort to validate that injections are satisfied and won't cause runtime crashes.

The current approach has the same problem when it comes to the actual app. With the additional downside of being a lot more code being required to pass dependencies down the composable hierarchy.

The existing approach bubbles down the injected class's values and function references, rather than calling the injected class's function directly. This approach comes with boilerplate, but it's a worthwhile trade-off considering the benefits of compile-time error detection.

It is a lot more boilerplate. The only benefit is that it's less likely there will be runtime problems with previews. But if we forget to provide the dependency in the app module, all test will work fine, all previews will show the right thing, yet the app will crash at runtime. Neither the current approach nor the one proposed in this PR change that.

This is an architecture decision that probably needs some experimentation to arrive at a good compromise. It shouldn't be coupled to the AppTitleTopHeader change. So I'm closing this pull request and open another one (#8341) to only switch from using the app name to the brand name without changing the current approach.

In the future I'll open a separate PR with a suggestion on how we could change the way we pass/inject dependencies in composables. We can then use that PR to discuss and iterate on the approach.