Skip to content
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

Provide methods for resetting the app and bypassing appCache #797

Merged
merged 24 commits into from
Jan 16, 2022

Conversation

Jaifroid
Copy link
Member

This is intended to address #794, because we have a number of new developers who will find the new functionality confusing.

This provides a UI for resetting the app (see screenshot) under Expert Settings.

The button "Reset" performs a full app reset consisting of:

  • Warning the user and seeking confirmation
  • Clearing any cookie entries
  • Clearing any Local Storage entries
  • Deleting the Cache API caches
  • Deregistering any registered Service Workers
  • Rebooting the app

Of course all settings are lost and reverted to defaults.

The checkbox is intended for Service Worker mode only (it will complain if set in JQuery mode). It bypasses the appCache by:

  • Clearing the Cache API caches
  • Preventing Service Worker from using the appCache, forcing it to fetch all app files

When re-enabling the appCache (by unticking the checkbox), it will:

  • Deregister the current Service Worker
  • Reboot the app in order to reinstall the Service Worker and repopulate the offline appCache.

image

@Jaifroid Jaifroid linked an issue Jan 12, 2022 that may be closed by this pull request
@Jaifroid
Copy link
Member Author

If these settings or UI are considered confusing for basic users of the app (non-developers) we could hide them and have a small link at the bottom of the Expert settings panel that would say:

Reset and dev options >

This link would reveal the UI.

@Jaifroid Jaifroid marked this pull request as ready for review January 12, 2022 11:25
Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the purpose of the PR.
I did not test but had a quick look at the code (see my comments).

I'm wondering if we should not move the content injection mode choice outside of the "expert settings", now that we are close to switching it to SW by default (but it might be a different topic/PR)
And it could be renamed "Expert/developer settings" with your new content.

Regarding the "Bypass appCache" checkbox, I find the UI confusing. The fact that it's on the same line as the reset button tends to make me believe it is an option of the reset button.
And I think it should be hidden when in jQuery mode

service-worker.js Outdated Show resolved Hide resolved
service-worker.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/lib/settingsStore.js Outdated Show resolved Hide resolved
www/js/lib/settingsStore.js Outdated Show resolved Hide resolved
www/js/lib/settingsStore.js Show resolved Hide resolved
@Jaifroid
Copy link
Member Author

@mossroy Thanks for the quick response. Some comments below:

I'm wondering if we should not move the content injection mode choice outside of the "expert settings"

That's a good idea, and can be done as part of this PR. Any idea what to call the box for this? Suggestions:

  • "Compatibility settings"
  • "Compatibility mode"
  • "Operation"
  • "Mode of operation"

Regarding the "Bypass appCache" checkbox, I find the UI confusing.

OK, I can see that. I can relocate to next line and only show that option if SW mode is enabled. The Reset button works in both modes, btw.

@Jaifroid
Copy link
Member Author

He, he, I had to fire up IE11 and run it from the file:// protocol to discover (remember) that we don't actually use the keyPrefix ('kiwixjs-') in front of legacy cookie keys, hence settings weren't being reset in IE11.

I've amended the code to fix this and also discovered another very minor incompatibility also fixed.

I was actually quite impressed that our code still runs fine from file protocol in IE11! That's quite some achievement in 2022...

@Jaifroid
Copy link
Member Author

Suggested new UI in screenshots below (left is what you see in jQuery mode, right in SW mode). I've also set the gh-pages implementation to this branch: https://kiwix.github.io/kiwix-js/www/index.html . If you had already visited this implementation, it won't immediately show the new code. It will self-update in SW mode, but only after exit and restart of app, if it has had time to cache the new code. Thereafter, the reset button should show. Alternatively, go to Dev Tools, Application tab (or Storage tab in FF), and delete the appCache, then reload.

image

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 13, 2022

Should I move the option "Permanently hide active content warning" into Expert settings too? I'm not sure we really want ordinary users deactivating that warning if we move to having SW mode as the default. We need to be able to warn them if they've switched to JQuery mode and forgotten about it.

Alternatively, this could be done as part of the PR for #196.

@Jaifroid
Copy link
Member Author

Possible alternative UI arrangement (rough approximation, because this is done as a mockup in KJSW):

image

@mossroy
Copy link
Contributor

mossroy commented Jan 14, 2022

Thanks for the screen snapshots.
"Compatibility settings" looks good. Maybe we should remove the "requires secure context" : with the PWA version for Firefox, it should now be supported on all recent platforms. And I don't think the users will understand what we mean by "secure context".

For "Expert settings", I prefer your second ("alternative") proposal. The layout makes it more clear that these are separate options/actions
I agree with moving the "Permanently hide active content warning" option there

@Jaifroid
Copy link
Member Author

This is what UI looks like now (after latest commit):

image

@mossroy
Copy link
Contributor

mossroy commented Jan 14, 2022

Looks great!

@Jaifroid
Copy link
Member Author

Test implementation: https://kiwix.github.io/kiwix-js/ updated to latest code on this branch, for quick testing. App may need to self-update to version 3.3.7 (a dummy version number) and should notify when that version is ready (in Config) if app is being used. Anything lower, and it's not on the latest code.

@Jaifroid Jaifroid mentioned this pull request Jan 15, 2022
18 tasks
Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test a lot, but I'm sure you did.
OK for me

www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
@Jaifroid
Copy link
Member Author

Will test on real IE11 on a Windows 10 machine before merging. Works OK in IE11 Mode on Windows 11, but it's not 100% the same.

@Jaifroid
Copy link
Member Author

Test on real IE11 successful.

@Jaifroid
Copy link
Member Author

Small "bug" (more of a feature!) discovered when testing reset in the Firefox extension in SW mode. Because the app uses the query string to set parameters between the local extension and the PWA, if the user presses the reset button while the query string still has several parameters in it, then the parameters are set again from the query string.

The solution will be to clear the query string EXCEPT for the referrerExtensionURL, which should be retained so that the user can go back to the extension from the UI. We will have to retain the contentInjectionMode too, as by definition the remote extension/PWA should be in SW mode.

I'll test a quick fix for that.

@Jaifroid
Copy link
Member Author

That issue is now fixed. All tests passing, including the following:

  • Reset App button works fine in IE11
  • Reset App in the PWA launched from Firefox extension will remain in SW mode and will retain access to local extension code if user switches back to jQuery mode
  • Unchecking the Bypass App Cache checkbox will reboot the app to repopulate the AppCache, but all settings are retained;
  • Works in Chromium extension

So I'll squash/merge this after midday GMT unless I hear any objections.

@Jaifroid Jaifroid merged commit 06e528a into master Jan 16, 2022
@Jaifroid Jaifroid deleted the Provide-app-reset-mechanism branch January 16, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checkbox or button to empty appCache in Cache API panel
2 participants