search

thunder-app / thunder

Thunder - An open-source cross-platform Lemmy client for iOS and Android built with Flutter
https://thunderapp.dev
GNU Affero General Public License v3.0
732 stars 62 forks source link

Fixes to feed page refactor #798

Closed hjiangsu closed 8 months ago

hjiangsu commented 8 months ago

Pull Request Description

This PR fixes some of the issues that were present in the feed page refactor. So far, it includes fixes for the following items:

If other minor issues are found, more will be added onto this PR

Issue Being Fixed

This references #752

Issue Number: N/A

Screenshots / Recordings

Checklist

hjiangsu commented 8 months ago

Still can't open link preview from feed. Still can't open images or links from post.

Hmmm, could you pass me your app settings? I think it has something to do with the specific settings that you have!

micahmo commented 8 months ago

could you pass me your app settings?

You bet!

https://files.catbox.moe/thdw3o.json

hjiangsu commented 8 months ago

@micahmo Alright, I think I fixed it! Could you double check and make sure as well?

micahmo commented 8 months ago

Yep, seems to work fine now! The only thing I'll say is that it looks a little funny that we wait for the post to get marked as read before navigating to the link. I'm not sure if previously that would happen in the background or after the fact. Not a big deal.

https://github.com/thunder-app/thunder/assets/7417301/2c87efa1-7d3d-4f2e-bbab-2aba83834d96

hjiangsu commented 8 months ago

Oh, I added code in to optimistically mark the post as read when tapped so that might've been it 😅 It doesn't necessarily wait for it to get marked first before navigating to it, it just happens to be faster than the navigation animation I think

micahmo commented 8 months ago

@hjiangsu One more thing I noticed (not sure where else to mention it).

There is kind of a funny quirk with subscribing to communities where the initial response might say that the subscription is pending, but if you re-fetch, you will be successfully subscribed. In order to represent this to users, I had added a little trick in the community bloc so that, after subscribing, we'd wait 1 second, then fetch the community again. You could see this in the UI with the icon going to pending and then subscribed.

https://github.com/thunder-app/thunder/blob/330bf57bddc0220820356d7a8978c5416c281693/lib/community/bloc/community_bloc_old.dart#L364-L371

https://github.com/thunder-app/thunder/assets/7417301/7c757fd5-8333-4724-9bab-672477c9597c

However since the new feed page doesn't use the community bloc, this little trick is gone, and subscriptions often stay in pending until refreshed.

https://github.com/thunder-app/thunder/assets/7417301/9541983d-6ffe-44eb-92bc-1f56ec823802

Note that this trick is not strictly necessary (in fact, even the web UI gets stuck in pending until refreshed), but it was a nice little thing to have which save the user some time and confusion. Hopefully we can keep it on the new feed!

micahmo commented 8 months ago

Oh, I added code in to optimistically mark the post as read when tapped so that might've been it 😅 It doesn't necessarily wait for it to get marked first before navigating to it, it just happens to be faster than the navigation animation I think

Hmm, I usually like the optimistic UI updates, but do you think it looks a little funny here? Not sure.

hjiangsu commented 8 months ago

There is kind of a funny quirk with subscribing to communities where the initial response might say that the subscription is pending, but if you re-fetch, you will be successfully subscribed. In order to represent this to users, I had added a little trick in the community bloc so that, after subscribing, we'd wait 1 second, then fetch the community again. You could see this in the UI with the icon going to pending and then subscribed.

Hmm, I can add that back in but do you think its best to just set the subscribed type to subscribed then in this case? This way we can avoid an extra API call in the process. I'm actually not sure what the "pending" status means in terms of functionality so maybe I'm not the best person to decide if we should skip showing that status or not.

I'm thinking if "pending" doesn't really affect the end user (in terms of functionality), then setting it to subscribed directly would help reduce data usage even if just a little bit

hjiangsu commented 8 months ago

Hmm, I usually like the optimistic UI updates, but do you think it looks a little funny here? Not sure.

I can maybe add a slight delay to make that effect go away!

micahmo commented 8 months ago

do you think its best to just set the subscribed type to subscribed then in this case?

No, because pending is a legitimate status! In some cases, it does actually stay pending for a long time, and you just need to check back to see if your subscription ever went through. This little trick was only to handle the quirk where it would be pending and then immediately subscribed.

But I hear what you're saying about reducing API calls. I'm kinda hoping that this is a back-end bug that gets fixed eventually, and then we can take out the trick.

hjiangsu commented 8 months ago

Alright sounds good! I'll add that extra check again and I'll just make a note of this to check if it's needed in the future 😁

hjiangsu commented 8 months ago

Alright, I've added a delay for optimistically marking posts as read, and added the second check for community subscription @micahmo!

Let me know if it all looks good. I'll make another PR to address the tagline issue mentioned previously since that one is a slightly bigger change

micahmo commented 8 months ago

I've added a delay for optimistically marking posts as read

This feels much better now, thank you!

added the second check for community subscription

Hmm, this still isn't working for me. I debugged the code and I can see that the subscribed state is correct in the second request, but the UI doesn't update. Still looks like the second screen recording in my previous post.

I'll make another PR to address the tagline issue

Sounds good!

hjiangsu commented 8 months ago

Hmm, this still isn't working for me. I debugged the code and I can see that the subscribed state is correct in the second request, but the UI doesn't update. Still looks like the second screen recording in my previous post.

Do you have the specific community that you tested it with? I just did another test and it seems to be working for me. There is a 1 second delay due to the timer

https://github.com/thunder-app/thunder/assets/30667958/548ce97c-3b4f-4da1-9ffe-534ea0b93073

hjiangsu commented 8 months ago

And here is a recording from the community sidebar

https://github.com/thunder-app/thunder/assets/30667958/f1dce637-5116-4452-b2db-73b25ba9603a

micahmo commented 8 months ago

I tried the same community you showed in your recording and it doesn't work for me. 😔 You can see the commit log in the background to prove I have the latest.

https://github.com/thunder-app/thunder/assets/7417301/49a4a6a2-73a6-4bc5-9a06-eba9f6d33449

BTW, the one second delay does work, and the API call does return the community with the right subscribed state. It's just that the UI doesn't update.

It sounds like this might be an Android issue, so feel free to merge this PR for now if you want (especially since the other stuff is pretty important!).

hjiangsu commented 8 months ago

Interesting, I'll have to look into that further then. Thanks for the screen recording! I'll merge this in first and make a separate fix for this issue when I figure it out

micahmo commented 7 months ago

Just wanted to follow up on this and say that the delay and subsequent update work fine on a physical device, so I'd call this one good!