-
Notifications
You must be signed in to change notification settings - Fork 227
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
Add "Date Published" sorting option and table column #1770
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have "date published" and "date added" which is really confusing in my opinion. Isn´t "publishing" the action of first releasing the original text of an article? I would rather call the two cases "Publication date" and "Last update".
Last update would be wrong though, because "createddate" in the database is created once and never changed. The problem with the "date" field is that it means something different depending on the feed type and whether a date is provided by the feed. For RSS it means "date published", for Atom it means either "date created", "date modified" or "date updated" – whichever is latest – and for JSONFeed it currently means "date modified". If no date is given, Vienna will fall back to a date at the feed level (which also differs per feed type) or – if no date at all is provided – the date when the feed was fetched by Vienna. I think another database column would give more flexibility, to distinguish publication date from modification date (both of which could still be filled in by Vienna). Also, the "createddate" column really ought to be updated too when an existing entry is modified. |
I meant createddate to be "publication date", which misses the point just a little (it is the date when Vienna first loaded the article), but offers the highest level of distinction to the other date value
Yes, for RSS there is no update date offering a distinction from the creation date. However, it rather reflects the last update than its publication date: vienna-rss/Vienna/Sources/Parsing/RSSFeed.m Lines 136 to 140 in 95f120b
and NSDate * articleDate = newsItem.modifiedDate;
// (...)
article.date = articleDate; in vienna-rss/Vienna/Sources/Fetching/RefreshManager.m Lines 767 to 850 in 95f120b
In conclusion, I am not 100% convinced of the naming either, but I would opt for the most distinguishable instead of the most accurate terms. The wording will be of no importance to the users who keep their settings as before, for them the change will be "Date" -> "Last update". The users who have problematic feeds that are updated in retrospective, they will have the option to change to "Publication date" which might not be totally accurate but have the desired effect, to keep their sorting in the order they originally received the articles. One refinement may be to not set the article "createddate" to the date where the article was received, if the article itself contains a date, but to the first value we encounter for the date of the article, and only use the date of receiving the article as fallback. Alternatively, we could introduce an additional field in the feed item that we set to the actual publication date in the feed types that provide it, and to the current date for the feed types that don´t, subsequently setting // We set the publication date ourselves if it is not contained in the feed
article.publicationDate = article.publicationDate ?: [NSDate date];
NSTimeInterval publicationIntervalSince1970 = article.publicationDate.timeIntervalSince1970; instead of the current way of setting the |
Please see https://github.com/TAKeanice/vienna-rss/tree/feature/date-added-field for a draft to implement my suggestion, for some reason I don´t seem to be able to open a PR to your repository @Eitot |
That is for the date of the feed ( vienna-rss/Vienna/Sources/Parsing/RSSFeed.m Lines 258 to 263 in 95f120b
What about RSS feeds though? Would "last update" and "created date" not be the same, if in the case of RSS they are likely both based on |
I think if there is no distinction like for RSS, either name for the date is fine. For all cases where there is a distinction, the "date" in article is the last updated date, so we should name it to reflect that. Please check the code in my implementation draft for reference |
I cleaned up some confusion that I had yesterday, now the code does what I intended it to do |
4360dd3
to
26eba00
Compare
@@ -1014,19 +1021,19 @@ - (BOOL)filterArticle:(Article *)article usingMode:(NSInteger)filterMode { | |||
case VNAFilterUnread: | |||
return !article.read; | |||
case VNAFilterLastRefresh: { | |||
NSDate *date = article.createdDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be the lastUpdate
date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TAKeanice I have incorporated your changes, with some changes:
- the UI string "Last update" I upper-cased to "Last Update" (per HIG guidelines)
- the UI string "Publication date" I renamed to "Date Published" (this phrasing is more common in English localisations and also shown in the HIG guidelines)
- "modifiedDate" properties I renamed to "modificationDate" (for consistency with publicationDate)
However, one thing remaining is the "last refresh" filter. This is probably broken now, since Vienna might add entries that have modification date (lastUpdate
) and publication date (publicationDate
) before the "last refresh".
I am OK with "last update date". |
Agree with you there, but we get into a distinction problem again: for JSON Feeds the publication date may actually be set by the author, and for atom feeds we at least have a chance to catch the publication date. Only RSS Feeds don't have it. We may set the first encounter of the article date in an RSS feed to the publication date, though. @Eitot feel free to change that at the comment line I added in the RSS feed class.
Too technical in my eyes. "Received date" or "Reception" maybe ("Empfangsdatum" in German) if you don't like "Publication date" at all |
Thank you, I appreciate that you added the thoroughness my original changes lacked
Another thing I suspect to be broken now is the intelligent folder feature - I don't remember how much it relies on the names of fields and which constants it uses for that. The persisted filters might fail if they contain one of the changed fields. Additionally, I think an option for filtering the "Publication / Added / ... " date should be included. |
The predicate editor seems fine at the first glance. I found at least one place where there's now inconsistency though, in SmartFolder.m in some predicate templates the hardcoded String |
“Reception date” sounds good to me. |
I added "Date published" to the intelligent folder predicates on my branch: https://github.com/TAKeanice/vienna-rss/tree/feature/date-added-field |
I am not sure about "reception date" (or "date received") as a substitute for "publication date" (or "date published"). Date received is the date when Vienna received the article from the source. That is not what this field will represent after this PR: it changes the implementation so that the value of that field will be the earliest date provided by the feed, if one is available ( I still see it as a challenge that we are attempting to reconcile different meanings of "date", that are inconsistent among the three feed types, in two fields. Recall that what gave rise to the PR is the problem that Vienna changes the visible date of an article even when the user knows that the article is older (#1749). To accommodate this, we sacrifice the "last refresh" filter. |
Vienna/Sources/Fetching/OpenReader.m
Outdated
NSDate *articleDate = [NSDate dateWithTimeIntervalSince1970:[newsItem[@"published"] doubleValue]]; | ||
NSDate *publicationDate = [NSDate dateWithTimeIntervalSince1970:[newsItem[@"published"] doubleValue]]; | ||
NSDate *lastUpdate = [NSDate dateWithTimeIntervalSince1970:[newsItem[@"updated"] doubleValue]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test on newsItem[@"updated"]
being nil
or not is needed here: I ended up with articles having "last update" set to year 1970…
Vienna/Sources/Fetching/OpenReader.m
Outdated
@@ -611,7 +612,8 @@ -(void)feedRequestDone:(NSMutableURLRequest *)request response:(NSURLResponse *) | |||
article.link = refreshedFolder.feedURL; | |||
} | |||
|
|||
article.date = articleDate; | |||
article.publicationDate = publicationDate == nil ? lastUpdate : publicationDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if neither newsItem[@"published"]
nor newsItem[@"updated"]
is provided, we should use [NSDate data]
Vienna/Sources/Database/Database.m
Outdated
NSTimeInterval publicationIntervalSince1970 = | ||
(article.publicationDate == nil ? [NSDate date] : article.publicationDate).timeIntervalSince1970; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to test on nil
here, as article.publicationDate
has already been set a few lines ago.
@barijaona I cleaned up the places you mentioned and made the logic as robust as I could. @Eitot i don´t know why I can´t push to your branch, this used to be possible when ticking "allow edits and access to secrets by maintainers" during the creation of a PR, but maybe I am doing something wrong. Instead, I opened a PR to your branch: Eitot#7 |
@TAKeanice , would you try again pushing to this branch ? For some reason, you were visible in the @ViennaRSS/maintainers group, without having the maintainer role… |
f9ffa05
to
e9c4002
Compare
Thanks for pushing my commits @Eitot . I need to check and test the criteria implementation, if I add any additional commits for that I will try pushing directly again. |
This partially reverts dbae950. With the ArticleFieldIDCreatedDate case restored, the explicit value of 415 for ArticleFieldIDEnclosure is no longer needed.
Co-authored-by: Eitot <[email protected]>
e9c4002
to
4bdb0d6
Compare
I have :
|
Vienna/Sources/Database/Database.m
Outdated
// we never change the publication date, unless the date provided in the feed is prior to it and makes sense | ||
NSDate * publicationDate = existingArticle.publicationDate; | ||
if (article.publicationDate && [article.publicationDate isLessThan:existingArticle.publicationDate]) { | ||
publicationDate = article.publicationDate; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the "and makes sense" part in this? What would happen if it doesn´t "make sense"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this comment is outdated.
At a time, I encountered dates equivalent to 1970 here, and had to add other tests, but I have since solved the issue elsewhere and could remove the extra precautions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don´t see any logic errors in the code that @barijaona added. Please consider the two comments I made though, to make sure we have not overlooked anything. I also fixed the smart folder to properly deal with the publication date and allow for predicates that contain it, so from my perspective (having seen the code and tested the predicate editor) the PR is good to go live.
Only when there is a provided publication date on creation, use that one without questioning it Fix publication date not being transmitted by RefreshManager.m Most dates interpretations/manipulations are now in Database.m During fetching from feeds, we just retrieve the infos and store them in relevant Article fields. This makes the logic more apprehensible and easier to maintain. Solves issue ViennaRSS#1749 Co-authored-by: Barijaona Ramaholimihaso <[email protected]>
Therefore, regarding dates, adopt in RSSFeed the same logic than in AtomFeed. Adapt RefreshManager to this unified logic.
As the meaning of `createddate` has changed, we have to adopt another approach which provides similar (but not identical) results.
this allows the predicate editor to correctly re-load "date published" predicates. Also, make the SmartFolder code better by extracting the repeated array of Date expressions and remove superfluous space in predicate translation strings
d692efb
to
2930d7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated a comment, as suggested by @Eitot, and force pushed.
This makes the "Last Refresh" filter to behave more like it did before pull request ViennaRSS#1770 (cf. commit bb06760) Significant difference though: "Last Refresh" refers to the last time a particular feed was refreshed and got new articles, while previously it referred to the last time "Refresh All Subscriptions" was triggered.
Improve management of folder's caches of articles After PR 1770 <#1770> was merged, I noticed that the "Last Refresh" filter did not always have the expected behavior. Investigation led to various discoveries regarding the use of NSCache and parts of the code where article's status was lost. This PR solves these issues and gives back a functional "Last Refresh" filter, with a difference though: "Last Refresh" now refers to the last time each feed was refreshed and got new articles, while previously it referred to the last time "Refresh All Subscriptions" was triggered.ent of folder's caches of articles
As suggested in #1749 the
createddate
SQL field is repurposed to show a "Date Published" option in the Sort By menu and add a new column for the horizontal layout. The existing "Date" sort option and column is renamed to "Last Update" to reflect its current purpose better.createddate
was previously set to a current date when Vienna added the entry to the database. To keep it simple, I have not added the "Date Published" field to the vertical layout, since that would require more elaborate changes to the table-view cell.While working on this I also stumbled on two bugs that caused the sort indicator in the column header not to show on app launch and when the user changes the sorting via the main menu. Both are fixed. The former bug is fixed with a band aid however, since the underlying cause is that ArticleListView is initialised before ArticleController, the latter which holds the property that determines the sorting order.