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 updated entry filters #6334

Merged
merged 3 commits into from
Apr 26, 2024
Merged

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Apr 18, 2024

fix #6331

@Alkarex Alkarex added this to the 1.24.0 milestone Apr 18, 2024
Comment on lines +591 to +595
$entry->applyFilterActions($titlesAsRead);
if ($readWhenSameTitleInFeed > 0) {
$titlesAsRead[$entry->title()] = true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

if (!$this->isRead()) {

But let me know if you spot any bug in the logic

Copy link
Contributor

@hkcomori hkcomori Apr 18, 2024

Choose a reason for hiding this comment

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

Oh, not that part.
I think the following part calls entry_auto_unread before filter action.

$entry->_isRead($mark_updated_article_unread ? false : null); //Change is_read according to policy.
if ($mark_updated_article_unread) {
Minz_ExtensionManager::callHook('entry_auto_unread', $entry, 'updated_article');
}

Then apply the filter action:

$entry->applyFilterActions($titlesAsRead);

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.

Copy link
Contributor

@hkcomori hkcomori Apr 26, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for commenting again. I had not had time yet to investigate and was about to do so :-)

@Alkarex Alkarex merged commit 5ca0b89 into FreshRSS:edge Apr 26, 2024
2 checks passed
@Alkarex Alkarex deleted the fix-updated-entry-filters branch April 26, 2024 11:29
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.

Apply *mark as read* to updated articles too
2 participants