Add user setting to auto-mark fetched posts as read. #5160

dessalines · 2024-11-02T01:22:20Z

Rather than apps collecting up viewed posts ids, and sending many mark as read requests, users can now turn this setting on, and any results from /post/list will be auto-marked as read.
Fixes Add a user setting to automatically mark fetched posts (list_posts) as read. #5144

- Rather than apps collecting up viewed posts ids, and sending many mark as read requests, users can now turn this setting on, and any results from /post/list will be auto-marked as read. - Fixes #5144

dessalines · 2024-11-02T01:23:18Z

crates/api_common/src/utils.rs

-  PostRead::mark_as_read(pool, HashSet::from([post_id]), person_id)
-    .await
-    .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead)?;
-  Ok(())


Most of these changes are boilerplate, because I've gotten rid of this pointless wrapper function, by having PostRead::mark_as_read return the correct LemmyError.

dessalines · 2024-11-02T01:25:41Z

crates/api/src/post/mark_read.rs


 #[tracing::instrument(skip(context))]
 pub async fn mark_post_as_read(
  data: Json<MarkPostAsRead>,
  context: Data<LemmyContext>,
  local_user_view: LocalUserView,
 ) -> LemmyResult<Json<SuccessResponse>> {
-  let post_ids = HashSet::from_iter(data.post_ids.clone());
+  let post_ids = &data.post_ids;



I got rid of all these fairly pointless HashSets (a way to enforce uniqueness of post ids to be marked as read).

Since we're no longer allowing vectors of marked read post ids via the API, and only through code, these seemed redundant. Also postgres can handle in queries with the same ID with no problem anyway.

It looks like this function is unused now.

I've removed that now in other PRs.

crates/apub/src/api/list_posts.rs

migrations/2024-11-01-233231_add_mark_fetched_posts_as_read/down.sql

Nutomic · 2024-11-02T22:02:23Z

Instead of a user setting, how about a parameter mark_as_read on /api/v3/post/list? Then it can be set explicitly by specific apps.

dessalines · 2024-11-04T20:19:21Z

I'll add an override there then also, similar to show_read / show_nsfw.

Nutomic · 2024-11-05T09:09:23Z

Should we get rid of /api/v3/post/mark_as_read then?

@EricBAndrews Please have a look at this.

dessalines · 2024-11-05T14:45:41Z

We still need mark_read, because this setting is optional (and default false), so most users will still need to explicitly mark posts in their feed as read if they don't want to click into them.

#5043 will change the mark_read from passing multiple post_ids, to a single one tho, which is why this PR is necessary (and this PR will help with many other apps who don't want to try to collect up post_ids to mark as read).

EricBAndrews · 2024-11-07T23:16:25Z

This feature unfortunately does not solve the use case that batch marking enables, namely marking read on scroll.

Mlem fetches in pages of 50, and preemptively fetches the next page when the user is ~10 posts from the bottom. This means that up to 60 posts could be marked as read without the user ever seeing them (e.g., if they close the app right after a load), which given the relatively low daily post volume on an average subscribed feed means a user might miss significant blocks of content and therefore have an artificially deflated sense of the activity level on Lemmy. Even on a reduced page size of 20, it's still expected to leave 15 posts erroneously marked as read every time the feed is closed. For this reason I would not utilize this feature to support mark read on scroll, and would default instead to sending individual requests per post. The rate limit of 180/min should be basically fine for standard use cases, though in the spirit of efficient engineering I would prefer to avoid such an implementation.

Nutomic · 2024-11-08T10:17:00Z

crates/api_common/src/post.rs

@@ -108,6 +108,9 @@ pub struct GetPosts {
  /// If true, then show the nsfw posts (even if your user setting is to hide them)
  #[cfg_attr(feature = "full", ts(optional))]
  pub show_nsfw: Option<bool>,
+  /// Whether to automatically mark fetched posts as read.
+  #[cfg_attr(feature = "full", ts(optional))]
+  pub auto_mark_fetched_posts_as_read: Option<bool>,


Should be changed to mark_as_read

Nutomic · 2024-11-08T10:18:42Z

You could still batch read posts on the client side, and then call /api/v3/post/list with auto_mark_fetched_posts_as_read: true (or mark_as_read: true), while ignoring the result. That would essentially be identical to /api/v3/post/mark_as_read with array as parameter, except it returns more data.

dessalines · 2024-11-08T12:42:16Z

Mlem fetches in pages of 50, and preemptively fetches the next page when the user is ~10 posts from the bottom. This means that up to 60 posts could be marked as read without the user ever seeing them

In that case I think I'll need to add back in a batch mark_posts_read endpoint, that returns SuccessResponse, like the other one used to.

I'll do that in a different PR tho, because I could still see this setting being useful, if the fetch sizes are small enough for a given client. Also keep in mind this setting is default off, so it won't actually do any marking by default.

You could still batch read posts on the client side, and then call /api/v3/post/list with auto_mark_fetched_posts_as_read: true (or mark_as_read: true)

This wouldn't be possible because they need a subset of the posts that come back from post_list. IE if they've fetched 50 posts, they would only want to mark the 10 or so that can currently be viewed. For cases where the batch size is really large, we've no choice but to add back in a post_ids: Vec<PostId>, but I'll do that as a separate endpoint.

Context: - #5043 - #4755 - #5160

Nutomic · 2024-11-08T14:52:46Z

If youre adding the endpoint for marking multiple posts as read, then clients should use that one to be explicit about it. This approach will lead to unexpected behaviour as explained by @EricBAndrews, so we should close this PR.

dessalines · 2024-11-08T15:00:20Z

I don't think this should be closed, it'd still be a useful feature to have, as long as the fetch size is small enough. It'd especially be useful for non-endless-scrolling clients.

This is also default off.

aeharding

I have the same concerns as @EricBAndrews on this proposal for Voyager app.

I am opposed to this PR because this setting only makes sense on paged layouts (page 1, page 2, page 3) where page directly corresponds to the request that will be made. This isn't true for infinite scrolling.

dessalines · 2024-11-09T14:27:36Z

I am opposed to this PR because this setting only makes sense on paged layouts (page 1, page 2, page 3) where page directly corresponds to the request that will be made. This isn't true for infinite scrolling.

So why are you opposed to this for paged layouts also? Not all apps do infinite scrolling.

Also this is entirely optional and default off. Are you opposed to optional features that some apps might find useful, but not yours specifically?

aeharding · 2024-11-09T17:39:16Z

Hi @dessalines thanks for your reply.

So why are you opposed to this for paged layouts also? Not all apps do infinite scrolling.

I am opposed to this for paged layouts because it breaking infinite scroll use cases:

Also this is entirely optional and default off. Are you opposed to optional features that some apps might find useful, but not yours specifically?

To be clear, this is NOT entirely optional. If a user turns it on, it WILL break custom mark as read logic for mark-as-read-while-infinite-scrolling apps, which creates problems because many times a user will turn it on in lemmy-ui settings, because they want it on for lemmy-ui but they are NOT expecting it to affect Voyager, and then complaining to me that Voyager is broken. This same issue occured with the "Hide Read Posts" setting until #4846 was introduced (thanks for adding that).

Speaking of #4846, this brings me to another option that would address both of our use cases: an override of the functionality in this PR per request.

Add mark_as_read?: boolean to /api/v3/post/list via a query param. If undefined, will default to user setting. If true, will always mark fetched as read, if false, will do no marking, just return posts.

This could probably be added to this PR.

dessalines · 2024-11-10T15:09:17Z

Add mark_as_read?: boolean to /api/v3/post/list via a query param. If undefined, will default to user setting. If true, will always mark fetched as read, if false, will do no marking, just return posts.

The override is already included in this PR, you'll just pass false to that field in list_posts.

This reverts commit 923d7f0.

aeharding · 2024-11-10T16:57:20Z

The override is already included in this PR, you'll just pass false to that field in list_posts.

My apologies, I did not read the PR close enough! (Or maybe I read an old commit accidentally? It has been a long week...)

That fully addresses my concern then!

Nutomic

Still need to address my previous comments, otherwise looks okay.

* Removing a few SuccessResponses for PostHide and MarkPostAsRead. - This also removes the pointless multiple post_ids. These can be done as individual calls on the front end anyway. - Fixes #4755 * Fixing federation tests. * Upgrading lemmy-js-client deps. * Add ability to mark several posts as read. Context: - #5043 - #4755 - #5160 * Fix ntfy to notify on success builds also. * Addressing PR comments.

* Removing a few SuccessResponses for PostHide and MarkPostAsRead. - This also removes the pointless multiple post_ids. These can be done as individual calls on the front end anyway. - Fixes #4755 * Fixing federation tests. * Upgrading lemmy-js-client deps. * Add ability to mark several posts as read. Context: - #5043 - #4755 - #5160 * Simplifying forms. * Fixing forms. * Cleanup post action forms by using derive_new defaults. - Fixes #5195 * Fix ntfy to notify on success builds also. * Removing pointless naive_now function. * Running taplo fmt.

Add user setting to auto-mark fetched posts as read.

bd0e68f

- Rather than apps collecting up viewed posts ids, and sending many mark as read requests, users can now turn this setting on, and any results from /post/list will be auto-marked as read. - Fixes #5144

dessalines requested review from phiresky, dullbananas, Nutomic and SleeplessOne1917 as code owners November 2, 2024 01:22

dessalines commented Nov 2, 2024

View reviewed changes

dessalines added 4 commits November 4, 2024 15:23

Adding list_post request option to auto-mark as read.

3dbc33e

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

176b9af

Moving db_perf to before federation tests.

8997119

Fixing lemmyerrortype import.

3034aa1

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

879b8e9

SleeplessOne1917 approved these changes Nov 6, 2024

View reviewed changes

dessalines and others added 4 commits November 6, 2024 11:47

Fixing ts_option.

a1ac93d

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

355c8f6

Fix clippy.

cdb324d

Merge branch 'main' into auto_mark_posts_as_read

9e7ab40

Nutomic reviewed Nov 8, 2024

View reviewed changes

dessalines added a commit that referenced this pull request Nov 8, 2024

Add ability to mark several posts as read.

f1aebf8

Context: - #5043 - #4755 - #5160

Nutomic closed this Nov 8, 2024

dessalines reopened this Nov 8, 2024

aeharding suggested changes Nov 9, 2024

View reviewed changes

dessalines added 3 commits November 10, 2024 10:15

Fix override logic.

923d7f0

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

11730a2

Revert "Fix override logic."

8b53a68

This reverts commit 923d7f0.

aeharding approved these changes Nov 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

bebb167

Nutomic approved these changes Nov 13, 2024

View reviewed changes

dessalines added 2 commits November 13, 2024 08:46

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

e0da787

Changing name to mark_as_read

912aafb

dessalines enabled auto-merge (squash) November 13, 2024 13:49

Merge remote-tracking branch 'origin/main' into auto_mark_posts_as_read

0747e60

dessalines disabled auto-merge November 13, 2024 15:05

dessalines merged commit a9d6d4e into main Nov 13, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user setting to auto-mark fetched posts as read. #5160

Add user setting to auto-mark fetched posts as read. #5160

dessalines commented Nov 2, 2024

dessalines Nov 2, 2024

dessalines Nov 2, 2024 •

edited

Loading

Nutomic Nov 7, 2024

dessalines Nov 13, 2024

Nutomic commented Nov 2, 2024

dessalines commented Nov 4, 2024 •

edited

Loading

Nutomic commented Nov 5, 2024

dessalines commented Nov 5, 2024 •

edited

Loading

EricBAndrews commented Nov 7, 2024 •

edited

Loading

Nutomic Nov 8, 2024

dessalines Nov 13, 2024

Nutomic commented Nov 8, 2024

dessalines commented Nov 8, 2024

Nutomic commented Nov 8, 2024

dessalines commented Nov 8, 2024 •

edited

Loading

aeharding left a comment

dessalines commented Nov 9, 2024 •

edited

Loading

aeharding commented Nov 9, 2024

dessalines commented Nov 10, 2024 •

edited

Loading

aeharding commented Nov 10, 2024

Nutomic left a comment

Add user setting to auto-mark fetched posts as read. #5160

Add user setting to auto-mark fetched posts as read. #5160

Conversation

dessalines commented Nov 2, 2024

dessalines Nov 2, 2024

Choose a reason for hiding this comment

dessalines Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

Nutomic Nov 7, 2024

Choose a reason for hiding this comment

dessalines Nov 13, 2024

Choose a reason for hiding this comment

Nutomic commented Nov 2, 2024

dessalines commented Nov 4, 2024 • edited Loading

Nutomic commented Nov 5, 2024

dessalines commented Nov 5, 2024 • edited Loading

EricBAndrews commented Nov 7, 2024 • edited Loading

Nutomic Nov 8, 2024

Choose a reason for hiding this comment

dessalines Nov 13, 2024

Choose a reason for hiding this comment

Nutomic commented Nov 8, 2024

dessalines commented Nov 8, 2024

Nutomic commented Nov 8, 2024

dessalines commented Nov 8, 2024 • edited Loading

aeharding left a comment

Choose a reason for hiding this comment

dessalines commented Nov 9, 2024 • edited Loading

aeharding commented Nov 9, 2024

dessalines commented Nov 10, 2024 • edited Loading

aeharding commented Nov 10, 2024

Nutomic left a comment

Choose a reason for hiding this comment

dessalines Nov 2, 2024 •

edited

Loading

dessalines commented Nov 4, 2024 •

edited

Loading

dessalines commented Nov 5, 2024 •

edited

Loading

EricBAndrews commented Nov 7, 2024 •

edited

Loading

dessalines commented Nov 8, 2024 •

edited

Loading

dessalines commented Nov 9, 2024 •

edited

Loading

dessalines commented Nov 10, 2024 •

edited

Loading