Differential D7928

Do not mark messages as unread immediately, makes unread/new filter unusable
ClosedPublic
Actions

Authored by marten on Sep 21 2017, 5:41 PM.

Details

Reviewers
dvratil
Group Reviewers
KDE PIM
Commits
R201:bb7f35c7938e: Do not remove read messages from the unread filter immediately
R201:c2d31a56f0d4: Do not remove read messages from the unread filter immediately
R201:08fe30f0973b: Do not remove read messages from the unread filter immediately
Summary

https://bugs.kde.org/show_bug.cgi?id=350731 describes a problem where, if the search filter is set to "Unread messages" and the "Mark as read after..." is set to zero or a short time, as soon as an article is removed from the list the next article is selected and then marked unread in turn. The effect of this is, with these filter settings, that all unread articles in a feed are marked read as soon as the feed is shown. These settings worked and had the intended effect in KDE4.

The diff here is the patch as attached to the bug comment #39, with the API doc cleaned up a bit.

Test Plan

Built akregator with this change, conformed correct operation of feed list (as it was in KDE4).
Other bug reporters have also applied patch and checked correct operation.

Diff Detail

Repository
R201 Akregator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
marten created this revision.Sep 21 2017, 5:41 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptSep 21 2017, 5:41 PM
cfeck added a subscriber: cfeck.Sep 21 2017, 7:43 PM
cfeck added inline comments.
src/article.cpp
389

Spaces around !=

marten updated this revision to Diff 19774.Sep 22 2017, 6:05 AM

Updated for PIM coding style

marten marked an inline comment as done.Sep 22 2017, 6:05 AM
anthonyfieroni added a subscriber: anthonyfieroni.Sep 28 2017, 7:50 AM
anthonyfieroni added inline comments.
src/feed/feed.cpp
834

You get status from here, then check for it's read below, e.g. it does not need a 3-th parameter

marten updated this revision to Diff 20037.Sep 28 2017, 1:51 PM

Thanks for the hint, have tested updated diff and it works correctly.

marten marked an inline comment as done.Sep 28 2017, 1:52 PM
anthonyfieroni added a comment.Sep 28 2017, 2:13 PM

Is there a call with oldStatus -1 and did you know if oldStatus is -1 and status() returns != Read articlesModified call is correct, e.g. should it be

if (oldStatus != -1 && newStatus != Read) {
    articlesModified();
}
marten updated this revision to Diff 20084.Sep 29 2017, 10:40 AM

I've done some more investigating and the simplified 2-parameter version doesn't seem to work correctly, in the case where the "Important" article flag is toggled by Article::setKeep(). If the article is already read then the flag is correctly changed but, because of the newStatus!=Read check, the article list is not visually updated - until the Akregator main window loses focus(!).

The only way to distinguish the mark-as-read case from any other is to have the 3rd bool parameter. So the diff is reverted to the original version as per the patch.

dvratil accepted this revision.Sep 29 2017, 12:04 PM
This revision is now accepted and ready to land.Sep 29 2017, 12:04 PM
Closed by commit R201:08fe30f0973b: Do not remove read messages from the unread filter immediately (authored by marten). · Explain WhySep 29 2017, 3:58 PM
This revision was automatically updated to reflect the committed changes.
ltoscano reopened this revision.Sep 29 2017, 4:14 PM
ltoscano added a subscriber: ltoscano.

Two comments (sorry, I'm late):

  • If the patch was mostly the one from comment https://bugs.kde.org/show_bug.cgi?id=350731#c39, it should be committed with the wrong author (the person who sent it); if it's the case, please revert and commit again after fixing the author (git commit --author).
  • it should probably go to Applications/17.08
  • in general, please try to use arcanist; there is a disconnection between what was written here (and was part of the evaluation) and what ended in the commit message.
This revision is now accepted and ready to land.Sep 29 2017, 4:14 PM
Closed by commit R201:bb7f35c7938e: Do not remove read messages from the unread filter immediately (authored by Erik van 't Wout <erik@hofhom.nl>, committed by marten). · Explain WhySep 29 2017, 4:40 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/article.cpp (2 lines)
Msrc/feed/feed.h (9 lines)
Msrc/feed/feed.cpp (8 lines)

Diff 20103

View Options

src/article.cpp

Loading...
View Options

src/feed/feed.h

Loading...
View Options

src/feed/feed.cpp

Loading...