worker8 / Pixels

Pixels is an Android app that brings you the beautiful and interesting pictures from Reddit
24 stars 3 forks source link

Remove UI and presentation logic from View Models #32

Open razilsh opened 5 years ago

razilsh commented 5 years ago

I was looking into #18 and the solution requires a reference to Context. This can be done either by inheriting from AndroidViewModel or by passing the context to setupGetComments(). Neither of which seems like a good solution as it'd add further coupling between the view and view models. I suggest removing all the UI related logic from View Models so that we have a better separation of concern between different layers. This would allow us to test and extend different components in isolation. What do you think @worker8?

worker8 commented 5 years ago

ah for solving #18, we can use an interface to solve it, so we don't have to reference to context, I don't want to use AndroidViewModel, because ViewModel simply doesn't need to know about context, so that it can make testing easier.

So for example, we can do this:

CommentContract.kt

class CommentContract {
    interface Input {
        ...
        val authorColorString: String // <--- new
    }

CommentActivity.kt

    private val commentInput = object : CommentContract.Input {
         ...
         override val authorColorString by lazy { 
             getColor(R.color.something)).let { /* convert to string */} 
         }
    }

Then we can use in CommentViewModel:

"<strong><u><font color='${input.authorColorString}'>${commentData.author}</font></u></strong>"

removing all the UI related logic from View Models

for this ☝️ , are you referring to this? https://github.com/worker8/Pixels/blob/master/app/src/main/java/beepbeep/pixelsforredditx/ui/comment/CommentViewModel.kt#L55-L83

The reason I place this portion in ViewModel is because there is a noticeable lag while scrolling through comments, and I suspect (didn't do a deeper dive) that it's due to comments flattening + Html.fromHtml() who is causing the lag. So I moved them into ViewModel and into a background thread.

p/s: sorry for accidentally hitting on the close button 😅

razilsh commented 5 years ago

removing all the UI related logic from View Models

for this ☝️ , are you referring to this? /app/src/main/java/beepbeep/pixelsforredditx/ui/comment/CommentViewModel.kt@master#L55-L83

Yes, along with this life-cycle method as the view model could easily outlive whatever life cycle owner it's holding a reference to. Ideally, view model would only dispatch data and events to the view. Just my 2c ^_^