Fix updated entry filters #6334
Conversation
| $entry->applyFilterActions($titlesAsRead); | ||
| if ($readWhenSameTitleInFeed > 0) { | ||
| $titlesAsRead[$entry->title()] = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
entry_auto_unread is called before the filter marks articles as read.
I think not calling it for filtered articles because they will be marked as read finally.
There was a problem hiding this comment.
The action filters are designed to perform more things than just marking as read (although not everything is implemented yet). Already read articles will be skipped a bit later:
Line 661 in 0ffcf41
But let me know if you spot any bug in the logic
There was a problem hiding this comment.
Oh, not that part.
I think the following part calls entry_auto_unread before filter action.
FreshRSS/app/Controllers/feedController.php
Lines 580 to 583 in 094ebf9
Then apply the filter action:
FreshRSS/app/Controllers/feedController.php
Line 591 in 094ebf9
Articles matching filter action are marked as read finally even though entry_auto_unread was called.
But when entry_auto_unread was called, I will think articles have been marked as unread (in fact it was marked as read).
So, I think it would be better if entry_auto_unread only calls for articles that it finally marks as unread.
Please let me know if my understanding is wrong.
There was a problem hiding this comment.
Sorry, please forget my comment.
Hook call behavior is a different topic and seems to be consistent with existing policy.
The existing code calls entry_auto_unread for no read status change too (unread -> unread).
It seems there is not need to change, because no problems have occurred now.
There was a problem hiding this comment.
Thanks for commenting again. I had not had time yet to investigate and was about to do so :-)
When the option *mark as read if an identical title already exists in the top n newest articles*, that should not apply to updated articles, as they risk being marked as read due to themselves having the same title. Identifying the different sub-cases would require a more complicated logic, so disable for now. Regression due to FreshRSS#6334
| @@ -587,6 +588,11 @@ public static function actualizeFeeds(?int $feed_id = null, ?string $feed_url = | |||
| continue; | |||
| } | |||
|
|
|||
| $entry->applyFilterActions($titlesAsRead); | |||
There was a problem hiding this comment.
Regression: marking as read for duplicated titles should not apply to updated articles.
Fixed in #6664
…#6664) When the option *mark as read if an identical title already exists in the top n newest articles*, that should not apply to updated articles, as they risk being marked as read due to themselves having the same title. Identifying the different sub-cases would require a more complicated logic, so disable for now. Regression due to #6334
|
@Alkarex Okay, I can't take the time to test specifically, but I'll watch out for any problems while using edge for a while. |
Excellent, thanks |
fix #6331