-
Notifications
You must be signed in to change notification settings - Fork 344
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
Sync client from server using last update timestamp #846
Conversation
if( $last_update > $since ) { | ||
$sync['stats'] = $itemsDao->stats(); | ||
|
||
if( array_key_exists('tags', $_GET) && $_GET['tags'] == 'true' ) { |
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 this is almost like GraphQL. Would not it be better to create a GraphQL endpoint? It could also save a lot requests during initial connection.
@@ -104,7 +93,7 @@ selfoss.events.entriesToolbar = function(parent) { | |||
type: 'POST', | |||
error: function(jqXHR, textStatus, errorThrown) { | |||
// rollback ui changes | |||
setButton(!starr); | |||
selfoss.ui.entryStarr(id, starr); |
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 !starr
@@ -151,8 +151,9 @@ selfoss.events.entries = function(e) { | |||
dataType: 'json', |
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.
After user loads the last page of the feed, this request is fired on every scroll event. Probably because now the stream-more
button is always present: https://github.com/niol/selfoss/blob/2620ee0c2a17ae44e4fffb63968f1cd5a23b3312/public/js/selfoss-events-entries.js#L130
...and use ISO8601. This triggered an inconsistency for pgsql wich was recording datetimes with millionth of a second precision whereas ISO8601 only handles thousandth of a second precision. This would have caused problems of duplicate entries for stream more and new offset_from_datetime implementation. That's why pgsql datetimes columns are converted to second precision which is enough.
For instance, items read on your smartphone and still visible on your desktop get styled as read after the next sync.
This change is bigger than expected because stream buttons logic was moved client side.
- do not discard list while refreshing - call new sync function when done to probe the server, update stats
public/js/selfoss-ui.js
Outdated
}, | ||
|
||
|
||
refreshStreamButtons: function(entries=false, |
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.
Nice, I did not know that any browsers already support this syntax. IE does not, though so we’d better stay at ES5 for now.
helpers/ViewHelper.php
Outdated
*/ | ||
public static function date_iso8601($datestr) { | ||
$date = new \DateTime($datestr); | ||
return $date->format(\DateTime::ISO8601); |
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.
\DateTime::ISO8601
is not actually ISO 8601 compliant, \DateTime::ATOM
should be used: https://secure.php.net/manual/en/class.datetime.php#datetime.constants.atom
- default function parameters are not supported before ES6 - array.find is not supported before ES6
public/js/selfoss-ui.js
Outdated
var newStatus = false; | ||
entryStatuses.some(function(entryStatus) { | ||
if( entryStatus.id == id ) | ||
newStatus = entryStatus.id; |
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.
Should not this be just newStatus = entryStatus
? And since you are actually trying to find some Weird, I have been using entryStatus
, why do you use Array.prototype.some
?Array.prototype.find
for ages, never knew it was ES6 feature.
Seeing this kind of PR, make me dream of a PWA front one day 👍 |
* @return array of unread, starred, etc. status of specified items | ||
*/ | ||
public function statuses($since) { | ||
$res = \F3::get('db')->exec('SELECT id, unread, starred |
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.
Is it possible to use proper booleans? Otherwise the sync will mark all changed items as starred and unread since unlike in PHP !!"0" === true
in JS:
$ php -r 'var_dump(!!"0");'
Command line code:1:
bool(false)
$ node -e 'console.log(!!"0")'
true
- Open stream
- In another tab mark some item from the stream as read
- Wait for synchronization, the item will remain unread in the first tab
public/js/selfoss-ui.js
Outdated
|
||
refreshItemStatuses: function(entryStatuses) { | ||
$('.entry').each(function(index, item) { | ||
var id = $(this).data(('entry-id')); |
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.
Superfluous parentheses.
Edit: You don’t even have to mark it in a different tab, it will be synced to the first tab too. |
85196bd should fix both of the reported issues that occur when using mysql or sqlite:
|
... even if it seems PHP is smart enough not to do it.
} | ||
} | ||
} | ||
return $rows; |
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.
Why do you return the value though it is mutated in-place and the returned value is never used?
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 thought it might be more handy sometimes to directly return the call to ensureRowTypes()
.
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.
To be fair, I would prefer not using reference at all, unless the performance benefits are significant. Ideally, PHP engine would be smart enough to optimize it.
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.
While working on this, I though about this being used on big SQL results and I red about PHP arrays passed by value being duplicated in memory by the PHP engine when modified within the function. I wanted to prevent this. It is trivial to remove though.
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.
For now, I would remove the returns, if we need them in the future, they can always be re-added. Though, in the long term I would like to drop this altogether and use some abstraction like https://nextras.org/orm/
Sometimes when I use “refresh sources” button, after the refresh finishes, the view is not updated. I cannot reproduce it consistently, though, maybe there is some race condition with sync? |
What do you mean by "the view is not updated"? |
I mean that the new items are not added, unless I reload the page. |
For now, it only reloads if the list is empty. It only update stats and shows the read more button if applicable. |
if( dataDate <= selfoss.lastUpdate ) | ||
return; | ||
|
||
if( data.stats.unread>0 && | ||
($('.stream-empty').is(':visible') || | ||
$('.stream-error').is(':visible')) ) { | ||
selfoss.reloadList(); |
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 see now.
@@ -173,12 +170,13 @@ selfoss.events.navigation = function() { | |||
$('#nav-mobile-settings').click(); | |||
|
|||
// refresh list | |||
selfoss.reloadList(); | |||
selfoss.sync(true); |
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.
It should be fixed so the user does not have to reload after the refresh.
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.
Refreshing sources could take some time. The point of the change is to let that run in the background and let the user continue reading and reload list ONLY if it is empty. If list has to be reloaded when refreshing is done even if items may still be displayed thus red by the user, I would rather revert 3071660 .
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.
Good point. I got used to waiting for the page to reload from previous versions but this might be actually better. In the future, it would be nice to add UI for reloading the list.
It looks good to me. Any points before merging this? |
This PR includes #834 .
Those changes add a new /items/sync API endpoint that sends database changes from a given datetime. This ensures only data absent from the client side is sent. This PR includes some client side changes to take advantage of this, including some functional enhancements such as cross-device updating of item statuses.