Move user info to modern architecture #3781
Conversation
tobiasKaminsky
commented
Mar 22, 2019
•
tobiasKaminsky
commented
- Needs Set user info android-library#266
tobiasKaminsky
force-pushed
the
userInfoModern
branch
from
1b0f144
to
f9e7108
Compare
AndyScherzinger
force-pushed
the
userInfoModern
branch
2 times, most recently
from
4b9482b
to
cf6aa1a
Compare
|
❗️ @tobiasKaminsky I rebased the branch to resolve the conflicts and saw that it still uses room 1.1.1 while room v2 is already out (which has quite some API changes) so I'd say the PR should go straight to room2. Also pinging @ezaquarii since this is an architecture change I am positive about this PR especially for adding room for the persistence implementation 👍 |
| } | ||
|
|
||
| public UserInfoModule(Application application) { | ||
| userInfoDatabase = Room.databaseBuilder(application, UserInfoDatabase.class, "userInfo.db").build(); |
There was a problem hiding this comment.
This should be @Provides-ed and @Singleton-ed.
|
|
||
| @Provides | ||
| public UserInfoViewModel userInfoViewModel(UserInfoRepository userInfoRepository) { | ||
| return new UserInfoViewModel(userInfoRepository); |
There was a problem hiding this comment.
Watch out with view models. VMs are managed with activity lifecycle and require special approach to injection.
For the time being, it would be easier to have them managed in vanilla way in activity.
I have some code somewhere where VM factory is hooked up to dagger.
| private UserInfoDatabase userInfoDatabase; | ||
|
|
||
| @Provides | ||
| public Executor executor() { |
There was a problem hiding this comment.
This one could go to main app component - it's pretty generic stuff.
|
Room is huge improvement over raw My biggest concern is that we're exposing Google junk. I don't trust Google a bit and as @AndyScherzinger observerd, they changed things before we even managed to finish implementing this and I'm sure they will break it again. Our persistence is already "challenging", migrating this will take time, so doing it on google's quicksand API could be risky. Sadly, Google is not designing their APIs for longevity, as most mobile apps are one night stands, so they can innovate rapidly not caring about backward compatibility too much. I totally understand this mentality, but it doesn't fit our case very well. IMO we should have our own repository data layer hiding whatever DB library is trending on social media this month and not leak any details outside it.
|
AndyScherzinger
force-pushed
the
userInfoModern
branch
from
cf6aa1a
to
4607c49
Compare
|
Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10322 |
|
Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10323 |
|
@AndyScherzinger I want to add the possibility to change the infos, e.g. phone, address, etc.
|
split up code use data binding add groups Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
tobiasKaminsky
force-pushed
the
userInfoModern
branch
from
0bd547d
to
22d6979
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10361.apk |
|
Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10361 |
Codacy276Lint
SpotBugs (new)
SpotBugs (master)
Lint increased!SpotBugs increased! |
sure thing, since I already had this discusion for the deck app with @juliushaertl. Simply replace it with EditText fields. For server propagation you likely will then have to do a check if the data changed for each field individually and send a request for each, changed field. |
|
https://developer.android.com/topic/libraries/data-binding/two-way I tried this and it seems that, as methods are auto generated, you cannot use this with adapter, but have to use one xml. We will loose flexibility, but as far as I can see it, it should be easy to maintain. Shall I go with this? |
|
@tobiasKaminsky does this work for propagating data to the server and only doing so on focus lost / back navigatioen etc (as-in not with every character change)? |
This is something I have to test, but I would have expected that text somehow needs to be "confirmed". |
