Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark as Read on Open and Scroll #1025

Merged
merged 27 commits into from
Aug 3, 2023
Merged

Mark as Read on Open and Scroll #1025

merged 27 commits into from
Aug 3, 2023

Conversation

ZJouba
Copy link
Contributor

@ZJouba ZJouba commented Jul 12, 2023

Addresses: #403, #535

Mark on scroll works beatifully:

Mark on open doesn't update the LazyColumn on Home. In fact, any update in PostActivity doesn't reflect on Home until refresh:
NotUpdating

val read: MutableState = mutableStateOf(false)) seems to be the answer but GSON doesn't know how to deserialize MutableState 🤷‍♂️

Any advice would be appreciated

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 12, 2023

Instead of passing that through the navigation as a JSONed string, pass the homeviewmodel to the post activity and only update that post. (I think it already does that in some places)

# Conflicts:
#	app/src/main/java/com/jerboa/MainActivity.kt
#	app/src/main/java/com/jerboa/db/AppDB.kt
#	app/src/main/java/com/jerboa/model/HomeViewModel.kt
#	app/src/main/java/com/jerboa/model/PostViewModel.kt
#	app/src/main/java/com/jerboa/ui/components/community/CommunityActivity.kt
#	app/src/main/java/com/jerboa/ui/components/home/HomeActivity.kt
#	app/src/main/java/com/jerboa/ui/components/person/PersonProfileActivity.kt
#	app/src/main/java/com/jerboa/ui/components/post/PostActivity.kt
#	app/src/main/java/com/jerboa/ui/components/post/PostListing.kt
#	app/src/main/java/com/jerboa/ui/components/settings/lookandfeel/LookAndFeelActivity.kt
@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 12, 2023

@MV-GH like so?

MarkPostAsRead(
    post_id = it,
    read = true,
    auth = account!!.jwt,
).run {
    postViewModel.markPostAsRead(this)
    homeViewModel.markPostAsRead(this)
}

Won't that mark the post as read with the api twice?

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 12, 2023

No i was thinking about updatePost but this still does a network call, If you your markPostAsRead network call is successfull do a updatePost (make one) that doesnt do the network call and just marks it as read.

@ZJouba ZJouba changed the title Issue/403 Mark as Read on Open and Scroll Jul 13, 2023
@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 13, 2023

@MV-GH Okay, I think I got it, not 100% sure if its correct though, if a user navigates back to home to quickly, I don't think it will update
UpdateHome

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, actually i think the viewmodel approach I said earlier, is not the way we should take this. We can open posts from many different locations. And this would only work with homeviewmodel. It also makes it a global and to make it work with the other viewmodels it would also make them a global. Instead I propose we should use the navController.ConsumeReturn Where if you return from a postActivity to a postlisiting. You refresh that post you went to. You will probably have to add this in each relevant activity. Such as PersonProfile, Home and Community

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 13, 2023

While I'm busy should I just do this for all activities that navigate to a post?

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 13, 2023

Yes

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 13, 2023

I'm not super familiar with android architecture, why do we have duplicate functions in each ViewModel, can't they be extracted to a common place?

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 13, 2023

Such as? If you mean the nextPage and such yes they can be. See #807

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 13, 2023

@MV-GH this approach seems to suffer from race conditions. If I set a breakpoint to inspect, it works. The Home/Community post list is updated and I can see my saved/like/dislike etc. But if I don't have a pause anywhere, the change doesn't reflect until I refresh. Any advice?

Nevermind sorry, I was just putting it in the back stack too early

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 13, 2023

Can you push what you have, I take a look when I have some time

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 13, 2023

Can you push what you have, I take a look when I have some time

Sorry, nevermind. Updated my previous comment

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 13, 2023

Okay, so I need to know if this is acceptable. Instead of having to return the post after every action (like, save, dislike) etc. I thought I might just do a request to get the latest post once you navigate back to the HomeActivity, this way, if someone else liked the post while you were navigating back, you'd have the latest info on screen load.

However, this resulted in the request for the updated post not completing before the composition and thus not showing in the Home posts list. And it's also not updating the post after composition (not sure why). So now I've had to add an "await" of sorts until the latest post is fetched and updated in the list of posts. Is this acceptable?
await

This is how I update the post in HomeViewModel.kt:

fun refreshSinglePost(id: Int, account: Account) {
    viewModelScope.launch {
        val postForm = GetPost(id = id, auth = account.jwt)
        postRes = apiWrapper(API.getInstance().getPost(postForm.serializeToMap()))
        val refreshedPost = postRes
        val existing = postsRes
        when {
            refreshedPost is ApiState.Success && existing is ApiState.Success -> {
                val refreshedPosts =
                    findAndUpdatePost(existing.data.posts, refreshedPost.data.post_view)
                val newPosts = ApiState.Success(existing.data.copy(posts = refreshedPosts))
                postsRes = newPosts
            }

            else -> {}
        }
    }
}

This line refreshedPost is ApiState.Success && existing is ApiState.Success seems to only be true after the Home screen has already loaded.

By adding postsRes = ApiState.Loading after this line, I effectively await the refreshing of said post which causes the wait you see in the gif

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 19, 2023

I'll take a look at this once the conflicts are fixed

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 20, 2023

I've also noticed that voting on a post doesn't show on the home activity if you navigate back too quickly since the request hasn't been completed by the time the return is consumed 😢

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 20, 2023

I don't understand why it doesn't re-compose when the postsRes is updated in HomeViewModel.kt?

@ZJouba ZJouba marked this pull request as ready for review July 28, 2023 08:05
Copy link
Contributor

@twizmwazin twizmwazin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but in the future please avoid reformatting code unrelated to your change.

Zack added 2 commits August 3, 2023 09:24
# Conflicts:
#	app/schemas/com.jerboa.db.AppDB/22.json
#	app/src/main/java/com/jerboa/api/Http.kt
#	app/src/main/java/com/jerboa/db/AppDBMigrations.kt
#	app/src/main/java/com/jerboa/ui/components/community/CommunityActivity.kt
#	app/src/main/java/com/jerboa/ui/components/home/HomeActivity.kt
#	app/src/main/java/com/jerboa/ui/components/person/PersonProfileActivity.kt
#	app/src/main/java/com/jerboa/ui/components/post/PostListings.kt
#	app/src/main/res/values/strings.xml
Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes are since that I have made account not nullable anymore, so now instead you have to check if it is Anon account or not

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 3, 2023

I did some testing, and if you open a post and close it before it even gets to load and thus fire our markRead, and then open it again let it properly load and then go back you will see that the post in the feed does not turn read. That is because when you open it again and thus request the post again. The read is already true and it wont fire markRead and thus not the (read= true)

So what I think is happening is that as soon as you even request to load the post. It will automatically be set to read, by lemmy making our markRead changes unnecessary but I have not confirmed this yet.

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 3, 2023

Alright just confirmed this with postman. If you open a unread post, you will get a response that is read=false, if you request that post again it will be read=true.

I'm thinking as soon as you click on a post it should mark it instantly as read in the feed (the feed itself updates the read). The problem is, what if you click on a post and it fails loading it.

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the approach we should take is, just keep the addReturn were it is now but it should always do it if you are logged in. Even when the post is read=true. The feed will only show it as read if the user sees the post. This will also prevent that previous issue where the post is not turning read in the feed if you open and close it quickly.

@MV-GH MV-GH enabled auto-merge (squash) August 3, 2023 10:34
@MV-GH
Copy link
Collaborator

MV-GH commented Aug 3, 2023

Ah pipeline died, you can try git commit --allow-empty -m "Trigger woodpecker" && git push

@ZJouba
Copy link
Contributor Author

ZJouba commented Aug 3, 2023

Ah pipeline died, you can try git commit --allow-empty -m "Trigger woodpecker" && git push

Oh I wish. This is a windows laptop 😁 we use ; here not &&

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 3, 2023

It does actually work in CMD and since PS 7

@MV-GH MV-GH merged commit 67debf5 into LemmyNet:main Aug 3, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants