Add feature for mark read on scroll #1139
Conversation
|
@micahmo You mentioned rebasing these changes to separate them from the Docker changes, however since the CI is broken until that PR is merged, it shohuld probably be done first. So I just left this as-is. |
|
And this is the only repo I've worked on where the PR #s and the Issue #s are so close, crazy! |
I always thought GitHub shared ID ranges for PRs and issues. Someone just opened a new issue and it's 1140. So I think the numbers will always be close. 😊 |
|
Sorry for the delay, and thanks for your work on this! I do have one discussion point that I'd like to bring up which may simplify the logic for determining which posts to mark as read. There is this package: https://pub.dev/packages/visibility_detector which allows us to detect when a widget is visible or not. This might be a more robust solution for determining if the post has been scrolled past since it doesn't rely on touch events, and we can adjust the threshold at which the callback triggers based on Let me know your thoughts! |
|
Oh good find. I will take a look later and see how it performs. |
|
Forgot to post here last night, but I have a working version using |
|
Ok, this should be ready now.
|
|
If it's ok, I'll review this after #1137 so the intended changes are more obvious. 😊 |
|
Found a small bug, will fix soon. |
|
Ok, this one is good for sure now. This weekend I added in a check to ensure the scroll was moving down, that way it wouldn't accidentally mark as read from in the middle of the screen when scrolling back up. |
|
Just released 0.2.9, so I'll start looking at the PR backlog soon. Thanks again for doing this @Fmstrat! |
|
I should say I haven't done any device testing on my new change yet, will do that tonight. |
|
I've been testing on a phone and tablet for the past day off and on, and things seem to be working cleanly after the bug fix. |
|
#1137 has been merged in now. I'll review this once the merge conflicts are fixed! |
lib/feed/bloc/feed_bloc.dart
Outdated
| // Give a slight delay to have the UI perform any navigation first | ||
| await Future.delayed(const Duration(milliseconds: 250)); |
There was a problem hiding this comment.
I'm not sure if this delay is needed for this use case.
lib/post/utils/post.dart
Outdated
| // TODO: Lemmy 0.19 support postIds in MarkPostAsRead | ||
| // Check what version the server is, and use that if we can |
There was a problem hiding this comment.
Check out the supportsFeature method in lemmy_client.dart if you want to make a version-specific call.
There was a problem hiding this comment.
Just left one comment! One thing I am concerned about is the number of API calls that might come from this feature.
I'm not quite sure if there are rate limits for mark as read action, so I'll leave it up for discussion whether or not we should reserve this feature for v0.19 to take advantage of the new API.
| @@ -73,14 +73,16 @@ final class FeedItemActionedEvent extends FeedEvent { | |||
| /// If both are provided, [postId] will take precedence | |||
| final int? postId; | |||
|
|
|||
| final List<int>? postIds; | |||
There was a problem hiding this comment.
I would add a small comment describing what this parameter is used for (e.g., If passed in, the [postAction] is applied to all the posts in the list)
|
I can say over the past week of use I haven't hit any rate limit issues, but that doesn't mean limits don't exist. |
|
Hey! A little update - so I did a bit of testing on my device, and it feels like its a finicky sometimes (sometimes, it'll miss marking as read for some posts event after i scrolled for a bit) I'll try to do some tweaks to see if maybe I can make it work a bit more fluidly for me if thats okay with you? I'll also add in logic to handle the 0.19 multi-read API (I'll keep the 0.18 logic since I haven't ran into rate limit issues either) |
|
Oh definately, do what you will with it! |
|
Alright, I did the following:
All of the adjustments were made in this commit: fcc1737 @Fmstrat Could you try this out and let me know if this is working well for you? Thanks! |
|
I'll go ahead an merge this in first, but let me know if you encounter any issues with implementation! |
|
Is there an APK or a possible date somewhere when a beta version will be available? |
|
@hjiangsu I'm not able to compile at the moment, so I can't check the changes on my devices. But given that and the above comment, any interest in setting up a nightly? https://stackoverflow.com/questions/63014786/how-to-schedule-a-github-actions-nightly-build-but-run-it-only-when-there-where Fully understand if not. |
|
Definitely! I was planning on setting up a nightly build within the next couple of days. I'm currently building nightlies manually rather than automatically because I write up the release notes for them instead of generating the default GitHub changelog. This is so that I can provide more context into some of the changes and also mention notable features to test out for each nightly (although this might change in the future if there's too much overhead in doing this 😅). |
|
Note that it is possible for GitHub actions to make "draft" releases (which are not public) to which you can attach the manual release notes and then publish. So it would remove all of the overhead except for the notes. Speaking of which, we haven't been very good about manually updating CHANGELOG.md. Is that still valuable to you @hjiangsu? If so, we should be better about enforcing it for PRs. If not, we can remove that checkbox. |
Thats what I'm doing at the moment! I do wonder if there's a way to make a "template" release (similar to how you can make autogenerated changelogs in releases)
Ahh yeah, I haven't been relying on CHANGELOG.md as much now because I try to make the commit messages for PRs the changelog if that makes sense. I have no preferences either way (whether we enforce it more or remove it) |
I meant that we can create an automated GitHub Action (like our build and instance updater) that actually generates the release artifacts for you (apk/ipa), either on a schedule, or manually, or whatever you want. It can create the draft release with whatever contents you want (like a template) and then literally all you'd have to do is go in and fill out the info. To make it even more fancy, you could have the release notes controlled by a file in the repo. Then you could push an update to that file, which would kick off the release and use the contents of that file as the contents of the release. Just some ideas! |
|
Oh thats really cool! I realized that you can create release artifacts through GitHub Actions, but did not know you could create draft releases from it. That might be something I look into then - I'll still have to find a way to coordinate making the associated Lemmy post, and publish it to Apple's TestFlight (could be done through fastlane if I set that up properly) On another note, @Offerel, I just released a new nightly version https://github.com/thunder-app/thunder/releases/tag/0.3.0-1 which contains the mark as read on scroll. Feel free to test it out and let me know how it goes! |
|
Many thx. I think the work you're doing is great. It's really nice to see how wishes from the community are not only taken seriously, but also implemented sensibly. Thumbs up! |
Pull Request Description
Adds setting under General -> Feed for Mark Read On Scroll.
NOTE
This should be merged after #1137 as that PR is a minor change that also fixes CI. This PR also contains those changes as it was built off that branch.
Issue Being Fixed
N/a, but discussion here: #1138
Issue Number: 1138
Screenshots / Recordings
Checklist
semanticLabels where applicable for accessibility?