-
Notifications
You must be signed in to change notification settings - Fork 345
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
Make selfoss an offline web application #1014
Conversation
dde1ad2
to
417bf48
Compare
if (updatedStatuses) { | ||
// Ensure the status queue is not cleared and gets sync'ed at | ||
// next sync. | ||
var d = $.Deferred(); |
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.
Maybe we could use real Promise
, if the signature claims it is returned. It is widely supported https://caniuse.com/#feat=promises and if we really need to support IE 11, we can use node_modules/es6-promise/dist/es6-promise.auto.js
from this polyfill.
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.
A while ago we wanted to support IE11...
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 do not really know what we want to support. We would probably have to ask Tobias. But I like transpiling releases as described above more and more.
url: 'items/sync', | ||
type: 'GET', | ||
type: updatedStatuses ? 'POST' : 'GET', |
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 not always use POST
?
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.
POST
changes things whereas GET
does not change the database. This is a HTTP
convention.
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 guess I view POST /items/sync
as an endpoint that changes x
things, where x
can be zero as well. I guess, in true REST, the endpoint would be split into two, one for each synchronization direction.
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 chose this because in one request, changes are committed to the server and new items are retrieved. On a slow link, this may count.
Thanks a lot for the thorough review. |
Sorry for the missing hunks, I've been in merge hell regarding this for so long that I do not even know what should be part of it anymore. |
Is there any more work required here? |
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 want to make sure the new API endpoints are consistent. That means the parameter names will need to be to the point and have clear types. Everything else can be done iteratively after this is merged.
helpers/View.php
Outdated
'public/css/fonts.css' | ||
], | ||
self::ls('public/images/*'), | ||
self::ls('public/fonts/*.woff') |
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.
Removing fonts in #1072
controllers/Items.php
Outdated
@@ -190,26 +196,88 @@ public function sync() { | |||
'lastUpdate' => $last_update->format(\DateTime::ATOM), | |||
]; | |||
|
|||
$sinceId = 0; | |||
$wantNewItems = array_key_exists('itemsSinceId', $params) | |||
&& $params['itemsSinceId'] != '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.
Should not this be changed now?
Curiously once I refresh the page, it will no longer complain.
If fixes after reloading.
|
Point 2: disabled offline because of https://bugzilla.mozilla.org/show_bug.cgi?id=781982 |
I had no luck reproducing points 3 and 4. Will try again another time. |
I have the following config: [globals]
username=admin
password=15fa12535a14fdcf8d6587239d7d01b26192ad987d449e37c435e7fe375020d687ceb66f4f836733299b42ac41ed4b860849074d0ec954957f00b50791ce7d30
salt=lkjl1289
db_port=
db_prefix=
public=1
logger_level=sDEBUG
auto_mark_as_read=1
; logger_destination=error_log
language=cs
|
4 should be ok, still cannot reproduce 3. |
I can still reproduce both issues on e801e85. Here are the screenshots of the debugger for third issue (both the console error and the error bar message): Also, the first time I open the app in the anonymous mode of Chrome, it shows “selfoss has been updated, please reload. Reload” but the reload button only dismisses the message bar, no reload is performed. I am not even sure why it is displayed, since I would assume there is no previous state in the service worker. |
And I can reproduce both issues in Firefox too, it is just that deleting |
If I wrap the then handler for the transaction in I do not understand why does it call On a side note, perhaps a method like the following one would be useful for developments: selfoss.nukeLocalData = function() {
selfoss.db.clear(); // will not work after a failure, since storage is nulled
window.localStorage.clear();
navigator.serviceWorker.getRegistrations().then(function(registrations) {
registrations.forEach(function(reg) {
reg.unregister();
});
});
selfoss.logout();
}; |
As for number four, I think this is caused by |
This fixed the second part of 3 but the initial issue of Perhaps we should just use As for 4, what do you think about removing |
Can I have the actual stack for both errors, as a summary of what is left to be fixed ? Maybe your workarounds would work, but I fail to understand why the failure occurs in the first place. And has I cannot reproduce this behaviour, this is quite difficult to fix for me. |
There are no more exceptions, the population fixed it. (Though now I am going to negative numbers when I read articles.) I do not have an empty database. --- a/common.php
+++ b/common.php
@@ -13,7 +13,7 @@ if ($autoloader === false) {
$f3 = $f3 = Base::instance();
-$f3->set('DEBUG', 0);
+$f3->set('DEBUG', 1);
$f3->set('version', '2.19-SNAPSHOT');
$f3->set('AUTOLOAD', false);
$f3->set('cache', __DIR__ . '/data/cache');
--- a/controllers/Items.php
+++ b/controllers/Items.php
@@ -191,6 +191,8 @@ class Items extends BaseController {
$itemsDao = new \daos\Items();
$last_update = new \DateTime($itemsDao->lastUpdate());
+ \F3::get('logger')->info('Sync since=' . print_r($since, true) . '; last_update=' . print_r($last_update, true));
+
$sync = [
'lastUpdate' => $last_update->format(\DateTime::ATOM),
]; I am just getting
which would invalidate the condition |
Nailed it: when there are many items to load, the items are loaded online for the user not to wait the complete population of the offline db. However, in that particular case of storage being null in private mode, |
Thanks, this has done it. I think it is ready to merge after a rebase/squash. |
Do not hesitate to let me know if I should do the rebase/squash. |
It is always better if the author does the rebase, since then it will be signed by their GPG key. |
This change implements client side storing of items in a IndexedDB database. It also makes the changes necessary to use it instead of loading everything from the server each time. This makes it possible to use selfoss offline for items that have been loaded when last online.
Thank you. |
Here are a couple of patches that make selfoss an offline web application. I've been using this for some months now and fixed the last issues I could encounter a few weeks ago. This has been working quite well for me in planes or subways with no network access and the app seamlessly syncs back to the server when available.
This pull request contains a lot of code but it is mostly different code paths from the current online behavior, that's why regression are unlikely. I'd be happy to fix any issue reported with this.