making feed screen look nicer #84
Conversation
|
|
||
| @GET("/api/v2/search") | ||
| suspend fun search( | ||
| @Query("q") query: String, | ||
| @Query("type") type: String, | ||
| ) : SearchResult | ||
|
|
||
| @GET("/api/v1/accounts/{id}") | ||
| suspend fun getAccount( |
There was a problem hiding this comment.
Not required for this PR but I would strongly suggest we create separate API interfaces for the different official APIs instead of just one god MastodonAPI (AccountApi, StatusApi, etc.)
There was a problem hiding this comment.
Probably a good idea
| // get in reply to account names | ||
| async { | ||
| status.inReplyToAccountId?.let { accountId -> | ||
| status.copy( |
There was a problem hiding this comment.
Ok 2 things here:
- I think it's really important for the service/network layer to be completely decoupled from business logic, and by adding the display name to the model returned by
MastodonApi, we are coupling it to this repository. If we want to combine different models coming back from the API, IMO this should be done in a use case with a new model. That way, we don't have to deal with complicated logic to cache and reconcile different models, and instead it can all happen in a use case. Then our cache can also remain a pure model of the API. - We shouldn't block the UI waiting for the display names. This is more reason for this to happen in a use case- it can return the statuses as-is immediately, and then update with the display names when it gets them
There was a problem hiding this comment.
- I'll create a new network only model in the network module for this so we separate them. I think business logic is fine the repository class though. Repository classes are in the data module and not part of the service/network layer.
- I'm not sure about this. I think it's critical information to see the account name someone is replying to, and seeing things like that pop in wouldn't look very good. But either way, I'm thinking it won't be like this forever. I made a suggestion in daniel's new API improvements doc that should fix this https://docs.google.com/document/d/1uxJ8l83r-W9beWU1HYKlH2a4UIs8jz4Y42mp4YJfwgM/edit
There was a problem hiding this comment.
Actually, for the first one I'm going to do that in a separate PR. I think our whole model / network model stuff needs a big refactor
| it.collect { feedType -> | ||
| when (feedType) { | ||
| FeedType.HOME -> getHomeTimeline() | ||
| FeedType.LOCAL -> {} |
There was a problem hiding this comment.
Why did this change? I don't think updating the timeline via the functions below will work anymore. This was setup to support switching feed types once we implement the UI
There was a problem hiding this comment.
I'm going to fix that in the future. I made some changes to the feed, like changing the flow to a stateFlow and removing the retrofit Response wrapper in favor of a try catch block so we can catch other errors besides network response issues.
|
I'm going to merge this now and fix the model stuff in a separate PR |
No description provided.