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

Fix swipe gesture attempting to mark feeds and groups as read #522

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

mbestavros
Copy link
Collaborator

@mbestavros mbestavros commented Jan 12, 2024

After some digging into #515, I think I've figured out the root cause of the various issues with "swipe to mark as read" marking unrelated items as read.

Internally, Read You has a markAsRead function that is called when a part of the app wants to mark an article as read. It takes a few parameters: either a group ID, feed ID, or article ID. It would take appropriate action depending on which ID was provided.

Now, an unwritten assumption of this bit of code was that only one of the IDs would be supplied to markAsRead at once. This assumption was violated by the swipe to dismiss code, which provided all three. This manifested as various inconsistent behavior throughout the app, especially on the Fever API - which has additional mark as read code.

The fix is simple: make sure the swipe to mark as read code only provides the article ID. That's what this PR does. I've tested it on Fever, and it seems to work.

FYI, in case you're curious, @Ashinch @kid1412621 and @nvllz. Resolves #515.

Signed-off-by: Mark Bestavros <markbest@bu.edu>
@mbestavros
Copy link
Collaborator Author

mbestavros commented Jan 12, 2024

@nvllz Could you grab the APK built from the PR and test that it fixes the issue? If it does, I'll merge and cut a new release. The swipe to dismiss bug was the last big bug I'm aware of right now.

Actually, I'm going to wait until I hear back on #523. Would be nice to have that bugfix in as well.

Copy link
Owner

@Ashinch Ashinch left a comment

Choose a reason for hiding this comment

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

Yes, this method requires selecting one of the three scopes: group, feed, or article.

@mbestavros mbestavros merged commit db99ab4 into Ashinch:main Jan 12, 2024
1 check passed
@mbestavros
Copy link
Collaborator Author

Merging so I can move forward with cutting a new release. @nvllz If you get a chance to test the fix, post in #515 if it works!

@nvllz
Copy link
Contributor

nvllz commented Jan 12, 2024

Sure, I'm checking it out now.

Edit: @mbestavros thanks for the fix, now it seems to work as expected.

@kid1412621
Copy link
Contributor

@mbestavros thanks for the fix. I guess we better refactor this fun markAsRead into 3 different ones. E.g. markArticleAsRead, markFeedAsRead and markGroupAsRead.
And we need to fix the SwipeableArticleItem to support already read case. (like able to toggle read/unread)

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.

Single article swipe dismisses all the items
4 participants