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

Various issues with feed refactor #811

Closed micahmo closed 6 months ago

micahmo commented 7 months ago

I've experienced a few more issues after the feed refactor, so I'm opening a new issue to track them. @hjiangsu Let me know if this is ok!

https://github.com/thunder-app/thunder/assets/7417301/1fcd93a7-ca7b-4cf1-ab2b-d44a4bf97103

https://github.com/thunder-app/thunder/assets/7417301/cda0cd32-6ef1-4912-8f4b-b3e28f28c435

https://github.com/thunder-app/thunder/assets/7417301/449b06d8-6e2e-4710-84d9-57f93f2691f7

Let me know if you need a hand with any of these (especially the Android back gesture one).

EDIT: Adding one more.

Header Header
image image
hjiangsu commented 7 months ago

Thanks for filling out more of the issues!

Navigated feeds do not have FAB.

Was this the case before the refactor? I can't remember if navigating to communities was present when tapping on a community

Using the back gesture on a sidebar feed doesn't go back to the default post listing (this was introduced in https://github.com/thunder-app/thunder/pull/402, for reference)

Yeah, I might need a bit of help on this one - I imagine I just forgot to migrate that back handler logic during the refactor

Feeds navigated via the sidebar do not respect the default sort and in fact override the main feed sort.

I think I know the issue on this one so I can do a quick fix for it!

Feeds extend below the OS navigation area while comments do not.

This one is most likely just a SafeArea issue that should be fixable pretty quickly

shortwavesurfer2009 commented 7 months ago

Various buttons have lost their accessibility labels. The hamburger menu, refresh, 3 dot button next to username instance in the hamburger menu, the action buttons on a reply long press (upvote, downvote, save, reply, edit)

hjiangsu commented 7 months ago

Thanks for reporting some more issues @shortwavesurfer2009!

The hamburger menu

Is there a particular message that the label should say? "Open", "Open drawer", etc?

shortwavesurfer2009 commented 7 months ago

@

Thanks for reporting some more issues @shortwavesurfer2009!

The hamburger menu

Is there a particular message that the label should say? "Open", "Open drawer", etc?

I can't remember what it said, but the previous version 0.2.4 was fine, and so you could just reuse that label.

shortwavesurfer2009 commented 7 months ago

Found another issue. When the sidebar is opened talkback doesnt focus on it and will continue to read what is behind the sidebar. The new post button should gain focus upon sidebar opening. Screenshot_20231010-051305_Thunder

micahmo commented 7 months ago

Using the back gesture on a sidebar feed doesn't go back to the default post listing (this was introduced in https://github.com/thunder-app/thunder/pull/402, for reference)

Fixing in #817!

ggichure commented 7 months ago

when switching feed types, one has to click multiple times to switch feed type.

https://github.com/thunder-app/thunder/assets/31619962/2e3c4e08-d5e1-4303-a32f-45b28f8d069a

micahmo commented 7 months ago

Is this due to throttling? @hjiangsu I know we've talked about this before, but I would advocate for greatly reducing the amount of throttling we do. I understand we don't want to overwhelm the instances, but I also think it would take hundreds/thousands of requests per second to even get close to that. I highly doubt a user could tap fast enough in Thunder to overwhelm an instance (and if they could, that instance is in trouble anyway 😆).

hjiangsu commented 7 months ago

Thanks @ggichure - I think I know what the problem is. It's similar to what @micahmo mentioned, where there is a throttle for events. To add some more details: most, if not all events are attached to a single bloc event FeedFetchedEvent which has the throttling enabled. So actions like fetching for new posts, switching sort types, and so on use that same event under the hood which causes the throttling.

I did take a look at this yesterday because it was affecting the drawer a bit too when attempting to switch communities but haven't come up with a good solution yet.

I know we've talked about this before, but I would advocate for greatly reducing the amount of throttling we do.

I tried to remove the throttling, but doing that caused the app to reach the instance's rate limit when I scroll down to fetch more posts for the feed. I can continue to look into ways of making this better, and maybe remove the throttling that happens!

micahmo commented 7 months ago

This is a strange one. I just double-checked, and our throttle duration for feed events is only 100 ms! We should easily be past that in this case, but clearly we're still getting throttled changing the sort type and changing feeds.

I tried to remove the throttling, but doing that caused the app to reach the instance's rate limit when I scroll down to fetch more posts for the feed.

Ahh ok, I think you told me that. I wonder if we were accidentally making way more requests than we thought? That's strange.

hjiangsu commented 7 months ago

This is a strange one. I just double-checked, and our throttle duration for feed events is only 100 ms! We should easily be past that in this case, but clearly we're still getting throttled changing the sort type and changing feeds.

Yeah, this one was puzzling me too. I even set the duration to 50 ms and it was still exhibiting the same behaviour. I'm not too sure if this is the case but it could be that the throttling is for completed/emitted bloc events? For example, once a bloc emits a new event, then the throttle kicks in and waits x ms before allowing another event to be triggered.

Ahh ok, I think you told me that. I wonder if we were accidentally making way more requests than we thought? That's strange.

This was because we were continuously calling the fetch more posts event once we scroll down past a given percentage of the feed. In the case where the throttling was removed, every scroll event past the threshold triggered a new event which caused the rate limiting.

hjiangsu commented 7 months ago

The following issues are still present and have not been fixed yet:

When the sidebar is opened talkback doesnt focus on it and will continue to read what is behind the sidebar. The new post button should gain focus upon sidebar opening.

@micahmo do you know how we could potentially fix this? I think you have a bit more experience handling these cases so some insight would be helpful!

Various buttons have lost their accessibility labels. The hamburger menu, refresh, 3 dot button next to username instance in the hamburger menu, the action buttons on a reply long press (upvote, downvote, save, reply, edit)

For this one, only refresh and the hamburger menu semantic labels have been added in.

CTalvio commented 7 months ago

Did anyone realise yet that FAB unsummon (swipe down) and summon (swipe up from corner) is borked?

hjiangsu commented 7 months ago

Did anyone realise yet that FAB unsummon (swipe down) and summon (swipe up from corner) is borked?

Could you give some more information on this? The summon and unsummon gestures should work, but the gesture area and position may have changed during the refactor. Having some sort of picture to illustrate where the gesture area should be would be helpful!

hjiangsu commented 7 months ago

Also, adding on to the list of issues with the feed refactor:

hjiangsu commented 6 months ago

I'll close this as there has not yet been any other issues mentioned with regards to the feed refactor