Sync client from server using last update timestamp #846

niol · 2017-01-15T10:46:35Z

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.

jtojnar · 2017-01-22T11:13:27Z

controllers/Items.php

+        if( $last_update > $since ) {
+            $sync['stats'] = $itemsDao->stats();
+
+            if( array_key_exists('tags', $_GET) && $_GET['tags'] == 'true' ) {


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.

jtojnar · 2017-01-22T11:28:05Z

public/js/selfoss-events-entriestoolbar.js

@@ -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);


This should probably be !starr

jtojnar · 2017-01-22T11:59:51Z

public/js/selfoss-events-entries.js

@@ -151,8 +151,9 @@ selfoss.events.entries = function(e) {
            dataType: 'json',


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

fixed

...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

jtojnar · 2017-02-15T01:12:31Z

public/js/selfoss-ui.js

+    },
+
+
+    refreshStreamButtons: function(entries=false,


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.

jtojnar · 2017-02-15T01:13:38Z

helpers/ViewHelper.php

+     */
+    public static function date_iso8601($datestr) {
+        $date = new \DateTime($datestr);
+        return $date->format(\DateTime::ISO8601);


\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

jtojnar · 2017-02-15T12:32:51Z

public/js/selfoss-ui.js

+            var newStatus = false;
+            entryStatuses.some(function(entryStatus) {
+                if( entryStatus.id == id )
+                    newStatus = entryStatus.id;


Should not this be just newStatus = entryStatus? ~~And since you are actually trying to find some entryStatus, why do you use Array.prototype.some?~~ Weird, I have been using Array.prototype.find for ages, never knew it was ES6 feature.

armandabric · 2017-02-15T13:25:09Z

Seeing this kind of PR, make me dream of a PWA front one day 👍

jtojnar · 2017-02-17T23:33:27Z

daos/mysql/Items.php

+     * @return array of unread, starred, etc. status of specified items
+     */
+    public function statuses($since) {
+        $res = \F3::get('db')->exec('SELECT id, unread, starred


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

jtojnar · 2017-02-17T23:34:34Z

public/js/selfoss-ui.js

+
+    refreshItemStatuses: function(entryStatuses) {
+        $('.entry').each(function(index, item) {
+            var id = $(this).data(('entry-id'));


Superfluous parentheses.

jtojnar · 2017-02-18T09:08:58Z

Open a stream with only one page available.
Mark the first item as read.
In a different tab, mark the item as unread.
Wait for synchronization, “stream-more’ button will appear.
Click the button, nothing will be added since the item is above the offset.

Edit: You don’t even have to mark it in a different tab, it will be synced to the first tab too.

niol · 2017-02-27T11:14:50Z

85196bd should fix both of the reported issues that occur when using mysql or sqlite:

the bool type in json is required for item statuses to propagate
the int type for stat counts is required for the stream-more button not to show in the case described above

... even if it seems PHP is smart enough not to do it.

jtojnar · 2017-02-27T14:34:05Z

daos/mysql/Database.php

+                }
+            }
+        }
+        return $rows;


Why do you return the value though it is mutated in-place and the returned value is never used?

I thought it might be more handy sometimes to directly return the call to ensureRowTypes().

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.

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.

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/

jtojnar · 2017-02-27T16:11:27Z

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?

niol · 2017-02-27T17:16:07Z

What do you mean by "the view is not updated"?

jtojnar · 2017-02-27T17:18:57Z

I mean that the new items are not added, unless I reload the page.

niol · 2017-02-27T19:32:57Z

For now, it only reloads if the list is empty. It only update stats and shows the read more button if applicable.

jtojnar · 2017-02-27T19:38:39Z

public/js/selfoss-base.js

+                if( dataDate <= selfoss.lastUpdate )
+                    return;
+
+                if( data.stats.unread>0 &&
                    ($('.stream-empty').is(':visible') ||
                     $('.stream-error').is(':visible')) ) {
                    selfoss.reloadList();


jtojnar · 2017-02-27T19:41:34Z

public/js/selfoss-events-navigation.js

@@ -173,12 +170,13 @@ selfoss.events.navigation = function() {
                    $('#nav-mobile-settings').click();

                // refresh list
-                 selfoss.reloadList();
+                selfoss.sync(true);


It should be fixed so the user does not have to reload after the refresh.

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 .

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.

jtojnar · 2017-02-27T20:21:09Z

It looks good to me. Any points before merging this?

jtojnar previously requested changes Jan 22, 2017

View reviewed changes

jtojnar mentioned this pull request Feb 6, 2017

Revert 854 revert 834 ppr/navigable #865

Closed

niol force-pushed the ppr/sync branch from 239b9b2 to ae76c98 Compare February 6, 2017 12:16

niol mentioned this pull request Feb 6, 2017

Coding style #870

Merged

niol added 7 commits February 12, 2017 18:24

use db lastUpdatetime to prevent uneeded computation of stats

3946683

get updated item status from server and show them

6a2667b

For instance, items read on your smartphone and still visible on your desktop get styled as read after the next sync.

show stream more on unread view when there are new items on the server

9c781c0

This change is bigger than expected because stream buttons logic was moved client side.

fix reverting ui changes after starr xhr error

20f740b

fix stream-more xhr sent upon each scroll event when auto_stream_more=1

e9e69f8

improve nav-refresh button

3071660

- do not discard list while refreshing - call new sync function when done to probe the server, update stats

niol force-pushed the ppr/sync branch from ae76c98 to 3071660 Compare February 12, 2017 17:29

jtojnar reviewed Feb 15, 2017

View reviewed changes

niol added 2 commits February 15, 2017 13:17

use \DateTime::ATOM rather than \DateTime::ISO8601 for better compliance

6235fcb

restore ECMAScript 5 compatibility (and IE compatibility)

31ca9e6

- default function parameters are not supported before ES6 - array.find is not supported before ES6

jtojnar reviewed Feb 15, 2017

View reviewed changes

fix typo in restore IE compatibility patch

1b820f7

fix typo preventing syncing starred status to the ui

21de209

jtojnar requested changes Feb 17, 2017

View reviewed changes

niol added 2 commits February 27, 2017 11:58

remove js superfluous parentheses

a20e83b

ensure proper types in JSON for /items/sync regardless of the db backend

85196bd

ensureRowTypes: prevent copying of $rows in memory for no reason

7fea6c9

... even if it seems PHP is smart enough not to do it.

jtojnar reviewed Feb 27, 2017

View reviewed changes

ensureRowTypes(): do not use PHP references

795fccc

jtojnar requested changes Feb 27, 2017

View reviewed changes

jtojnar approved these changes Feb 27, 2017

View reviewed changes

jtojnar merged commit 890e95f into fossar:master Feb 27, 2017

jtojnar mentioned this pull request Feb 28, 2017

Make it more apparent refreshing sources has finished #881

Closed

niol deleted the ppr/sync branch March 3, 2017 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync client from server using last update timestamp #846

Sync client from server using last update timestamp #846

niol commented Jan 15, 2017 •

edited

Loading

jtojnar Jan 22, 2017

jtojnar Jan 22, 2017

jtojnar Jan 22, 2017 •

edited

Loading

jtojnar Feb 15, 2017

jtojnar Feb 15, 2017

jtojnar Feb 15, 2017 •

edited

Loading

armandabric commented Feb 15, 2017

jtojnar Feb 17, 2017 •

edited

Loading

jtojnar Feb 17, 2017

jtojnar commented Feb 18, 2017 •

edited

Loading

niol commented Feb 27, 2017

jtojnar Feb 27, 2017

niol Feb 27, 2017

jtojnar Feb 27, 2017

niol Feb 27, 2017

jtojnar Feb 27, 2017

jtojnar commented Feb 27, 2017

niol commented Feb 27, 2017

jtojnar commented Feb 27, 2017

niol commented Feb 27, 2017

jtojnar Feb 27, 2017

jtojnar Feb 27, 2017

niol Feb 27, 2017

jtojnar Feb 27, 2017

jtojnar commented Feb 27, 2017

		@@ -151,8 +151,9 @@ selfoss.events.entries = function(e) {
		dataType: 'json',

Sync client from server using last update timestamp #846

Sync client from server using last update timestamp #846

Conversation

niol commented Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

armandabric commented Feb 15, 2017

jtojnar Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar commented Feb 18, 2017 • edited Loading

niol commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar commented Feb 27, 2017

niol commented Feb 27, 2017

jtojnar commented Feb 27, 2017

niol commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtojnar commented Feb 27, 2017

niol commented Jan 15, 2017 •

edited

Loading

jtojnar Jan 22, 2017 •

edited

Loading

jtojnar Feb 15, 2017 •

edited

Loading

jtojnar Feb 17, 2017 •

edited

Loading

jtojnar commented Feb 18, 2017 •

edited

Loading