Ensure that Presenters do not depend on other presenters.
#3618
Conversation
Also do some renaming since DirectLogoutPresenter interface can be removed.
…sPresenter. Also do some renaming since FullScreenIntentPermissionsPresenter interface can be removed.
…nter. Move canDisplayModerationActions from presenter API to the state it emits.
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
|
There was a problem hiding this comment.
LGTM, just a couple of questions.
Also, it's a shame we can't use some typealias and keep using the @ContributeBinding annotation, having to manually define the bindings in modules instead, which is a bit of a PITA. But it's trivial compared to building the fake presenters, so I think the change is for the better.
Also maybe @ganfra should take a look too?
| .withoutAnnotationOf(Ignore::class) | ||
| .size | ||
| println("Number of unit tests: $numberOfTests") | ||
| assertThat(numberOfTests).isGreaterThan(2000) |
There was a problem hiding this comment.
Won't this be obsolete if we keep creating tests?
There was a problem hiding this comment.
Yes, actually, it's just a way to ensure that tests are detected and also a way to show that we have many tests :)
|
|
||
| package io.element.android.libraries.fullscreenintent.api | ||
|
|
||
| fun aFullScreenIntentPermissionsState( |
There was a problem hiding this comment.
Should this be public and in the api module? It feels a bit dangerous 🫤 . Maybe it could be part of the fixtures sourceset? Also, the file name doesn't really match the contents.
There was a problem hiding this comment.
Yes, I hesitated, but all our StateProvider files contains a function which create state class instances (for instance aMessageComposerState) and those function are in production code, but I admit, generally in impl modules.
So that's maybe not a big deal?
There was a problem hiding this comment.
For the name, I keep the name we usually have for state provider... That does not exist, but I could create one, which could be used here:
But since the View is only using the lambda, I guess it does not worth it?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3618 +/- ##
===========================================
- Coverage 82.67% 82.65% -0.02%
===========================================
Files 1736 1735 -1
Lines 41199 41187 -12
Branches 5001 5000 -1
===========================================
- Hits 34060 34042 -18
- Misses 5372 5380 +8
+ Partials 1767 1765 -2 ☔ View full report in Codecov by Sentry. |
bmarty
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
Content
Do not inject presenter instance in constructor of presenter, but rather inject parameter with type
Presenter<State>and provide the implementation in module.This allow more isolated presenter tests.
Motivation and context
Cleaner code.
Screenshots / GIFs
Should not have any impact
Tests
Does not contain functional change, smoke test on the application should be enough, but please review carefully, maybe commit per commit.
Tested devices
Checklist