T303936 : Refactor GalleryActivity to use ViewModel #3640
Conversation
bosankus
requested review from
cooltey,
dbrant and
sharvaniharan
as code owners
…_to_use_viewmodel' into T303936-refactor_galleryactivity_to_use_viewmodel
…eryActivity.kt - Shifted hardcoded operation to GalleryViewModel - Added GalleryRepository.kt to separate out network calls. - Added 2 new methods in Service.kt -> getProtectionInfoSuspend & getEntitiesByTitleSuspend - Removed 1 old method in RestService.kt -> getMediaList - Added TODOs for future implementation - Removed rxjava imports.
…_to_use_viewmodel' into T303936-refactor_galleryactivity_to_use_viewmodel
…_to_use_viewmodel' into T303936-refactor_galleryactivity_to_use_viewmodel
|
Hi @cooltey , Requesting for review of these changes. |
|
Hello @bosankus , sorry we've been rather busy with other priorities recently; we will review this change in the next few days. |
| suspend fun getMediaListSuspend( | ||
| @Path("title") title: String, | ||
| @Path("revision") revision: Long | ||
| ): Observable<MediaList> | ||
| ): MediaList | ||
|
|
||
| /** | ||
| * Support method added to pass GalleryClientTest | ||
| * TODO: To be demised | ||
| */ | ||
| @GET("page/media-list/{title}/{revision}") | ||
| suspend fun getMediaListSuspend( | ||
| fun getMediaList( | ||
| @Path("title") title: String, | ||
| @Path("revision") revision: Long | ||
| ): MediaList | ||
| ): Observable<MediaList> |
There was a problem hiding this comment.
Do these changes relate to the comments only?
There was a problem hiding this comment.
Currently there is one use of getMediaList which is in GalleryClientTest, I do not know if this method is required in future.
But getMediaListSuspend is already being used LinkPreviewViewModel, and I have used in GalleryRepository.
Considering this i have added demise comment, so that devs can demise when GalleryClientTest starts using getMediaListSuspend .
Do let me know if you think these needs to be removed/changed.
There was a problem hiding this comment.
From what I see, the getMediaList in line 85 has been changed to line 95 which the changes you made were swapping two functions.
| Date: 19,May,2021 | ||
| **/ | ||
|
|
||
| sealed class GalleryViewState<out T> { |
There was a problem hiding this comment.
We have a similar class called Resource and you can check the usage in LanguagesListViewModel.kt. Would you be able to use it?
There was a problem hiding this comment.
If you see Resource there is no loading state,
Also there is no error part used in LanguagesListViewModel.kt .
However I can see .Error usage on TalkReplyViewModel.kt.
Data class is meant as container for values, where as open class is different concept. Additionally in GalleryViewState there is a base state called InitialState which is used as default value when mutable live data is initialized.
I am open to know more what do you think on my thought. Kindly educate me if I am missing something. @cooltey
There was a problem hiding this comment.
Since there's a similar class doing this (without loading and initial state handling), would it be possible to extend the Resouce and add InitialState and Loading in this class?
bosankus
requested review from
cooltey
and removed request for
dbrant and
sharvaniharan
…_to_use_viewmodel' into T303936-refactor_galleryactivity_to_use_viewmodel
|
Hi @cooltey did you get some time to check my comments please? |
| } | ||
| } | ||
|
|
||
| inline fun <reified T : Parcelable> Bundle.parcelable(key: String): T? = when { |
There was a problem hiding this comment.
This is not really necessary. It's perfectly fine to continue using the deprecated getParcelable, until the official libraries give us a compat method.
|
Close since this has not been updated for a year. |
This PR is for issue: T303936