Convert gallery from mvp to mvvm

ARK-Builders / ARK-Navigator

Android app for navigation through your data

MIT License

15 stars 15 forks source link

Convert gallery from mvp to mvvm #443

Open hieuwu opened 2 months ago

hieuwu commented 2 months ago

:rocket: Summary

Convert gallery from mvp to mvvm Details of the analysis https://glamorous-lung-568.notion.site/MVP-to-MVVM-Gallery-screen-analysis-fdb7ac52dfaf4f3a92d4fe0acbd1ab66?pvs=4

:framed_picture: Screenshots:

Provide screenshots to make it visible to reviewer if possible (Optional)

| Before | After | N/A

kirillt commented 2 months ago

@hieuwu thanks, could you also tell a bit about motivation behind these changes ⏤ what goal you want to achieve by the refactoring? Maybe pros & cons of MVP and MVVM

Am I right, that some parts of current codebase will be removed when you complete this PR? I see only additions right now

hieuwu commented 2 months ago

@hieuwu thanks, could you also tell a bit about motivation behind these changes ⏤ what goal you want to achieve by the refactoring? Maybe pros & cons of MVP and MVVM

Am I right, that some parts of current codebase will be removed when you complete this PR? I see only additions right now

Yeah I am writing a document that includes current code execution, pros/cons & how to analyze current code then convert them to MVVM step by step. One thing I can reveal for now is that it would help us a lot in adoption to Jetpack Compose if we want in the future.

Yes, current code could be removed after we complete testing for new one.

mdrlzy commented 2 months ago

@hieuwu Nice work! We need to get rid of outdated MVP. What do you think about using mvi-orbit? For example you can check FoldersViewModel

@kirillt MVP is outdated and no longer developed. MVVM is a standard in Android development and is promoted by Google. MVVM has less code and is easier to work with. And as Hieu noted you won't be able to use Compose with MVP.

shubertm commented 2 weeks ago

https://github.com/ARK-Builders/ARK-Navigator/assets/87703131/67d43ce7-7d47-49b8-9d8c-c2986308eede

[x] Tag shows on wrong resource in Gallery
[x] Selection goes forward by 1 step or more when scrolling in Gallery

https://github.com/ARK-Builders/ARK-Navigator/assets/87703131/d05c80d0-a91b-40ca-ad76-07d6412c4537

[ ] Closing the keyboard erases the tag I think user should be able to back away from tag editing using back button

hieuwu commented 2 weeks ago

VID_20240613_161155.mp4

[x] Tag shows on wrong resource in Gallery

[x] Selection goes forward by 1 step or more when scrolling in Gallery

Thank you for pointing them out, I have fixed them. Please help check again.

sonarcloud[bot] commented 1 week ago

Quality Gate failed

Failed conditions
11.4% Duplication on New Code (required ≤ 5%)

See analysis details on SonarCloud