IdleService #427

gportela85 · 2018-07-03T01:29:19Z

lbwexler · 2018-07-05T17:40:55Z

Was looking for a default display of this (app overrideable) more like what we have in Hoist Sencha.
See App.showIdleWindow()

This looks like a start, but really need that bit in order to merge things.

Also, the Hoist Sencha version had the ability to change this per user via preference. Seems like for consistency we really just want to mirror everything we have in Hoist Sencha.

TomTirapani · 2018-07-06T14:54:13Z

Took a look at this.

I like how the app can specify any idleComponent, allowing us to override the alert to do something different / more elaborate should the app call for it.

We're missing a few features that we need to reach parity with Hoist Sencha:

A way for Apps to disable idle detection generally.
A way to disable per user using preferences.
We should stop any timer based tasks when the app becomes idle. I think we may be able to use Timer.cancelAll() for this.
We should also fire an appSuspended event, so that other parts of the app can react if necessary. Alternatively, rather than an event, this could be an observable on App itself.

See https://github.com/exhi/hoist-sencha/blob/develop/grails-app/assets/src/xh/all/svc/IdleService.js for how these features are implemented in Hoist Sencha

lbwexler · 2018-07-09T18:25:16Z

app/AppContainer.js

@@ -58,7 +59,8 @@ export class AppContainer extends Component {
    render() {
        return div(
            this.renderContent(),
-            exceptionDialog() // Always render the exception dialog -- might need it :-(
+            exceptionDialog(), // Always render the exception dialog -- might need it :-(
+            idleWrapper()


thinks this belongs more properly down around line 90. Is not analogous to the exceptionDialog in needing to be rendered in all states. Can we call it idleDialog for symmetry? Also, can we perhaps trigger this with a LoadState.IDLE. Think it makes more sense to handle that way, since this is a terminal state. It will be easier to reason about, and we can avoid the extra wrapper component.

lbwexler · 2018-07-09T18:43:37Z

app/impl/IdleWrapper.js

+import {Component} from 'react';
+import {XH, HoistComponent, elemFactory} from '@xh/hoist/core';
+
+@HoistComponent()


Sorry -- you made those last changes so quickly, I hadn't finished my comments. Don't think we need this wrapper component. This null check can be just made inline in the main renderer.

lbwexler · 2018-07-09T18:44:13Z

core/HoistApp.js

@@ -62,6 +63,21 @@ export function HoistApp(C) {
            };
        },

+        /**
+         * App must implement this method with appropriate logic to display a message when the app
+         * has been idle for the amount of minutes defined by the `xhxhIdleTimeoutMins` config.


spelling mistake here....

lbwexler · 2018-07-09T18:45:08Z

core/HoistApp.js

@@ -62,6 +63,21 @@ export function HoistApp(C) {
            };
        },

+        /**
+         * App must implement this method with appropriate logic to display a message when the app


'may override' instead of 'must implement'

lbwexler · 2018-07-09T18:48:21Z

core/HoistApp.js

+         * has been idle for the amount of minutes defined by the `xhxhIdleTimeoutMins` config.
+         * @param {Function} callback - A callback function to be handled by the showIdleDialog
+         */
+        showIdleDialog(callback) {


perhaps this should be named 'renderIdleDialog()' if its job is to return a react element that can be rendered.

lbwexler · 2018-07-09T18:55:47Z

svc/IdleService.js

+
+        window.dispatchEvent(new CustomEvent('appSuspended'));
+        XH.idleComponent = XH.app.showIdleDialog(() => {
+            window.location.reload();


Why is the flag for reload (e.g. reload(true)) missing?? This is clearly in the sencha source, and we spent a long time troubleshooting problems related to that...

I used the initial idleService PR (https://github.com/exhi/hoist/pull/250/files) as source for these changes so I would be able to take compare the whole implementation instead of just looking at the final code in IdleService.js. That's why some things were missing like using an app config or user config to disable the service. I'll add the flag to disable cache.

lbwexler · 2018-07-09T19:01:13Z

svc/IdleService.js

+        this.destroyAppListener();
+        Timer.cancelAll();
+
+        window.dispatchEvent(new CustomEvent('appSuspended'));


We don't use window.dispatchEvent, or anything like window events. Realize we are trying to mimic the sencha implementation here, but not sure we need this. Also, the term suspended is just a new word -- if we were going to have such an event would probably want something with the word 'idle' in it

lbwexler · 2018-07-09T19:10:39Z

yes, that's always a risk with looking at a single pull request -- gotta look at the latest and greatest of the code in question,

…

On Mon, Jul 9, 2018 at 3:02 PM, Guilherme Lopes ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In svc/IdleService.js <#427 (comment)>: > + destroyAppListener() { + window.removeEventListener('keydown', this.resetTimer, true); + window.removeEventListener('mousemove', this.resetTimer, true); + window.removeEventListener('mousedown', this.resetTimer, true); + window.removeEventListener('scroll', this.resetTimer, true); + } + + @action + timeout() { + this.task.cancel(); + this.destroyAppListener(); + Timer.cancelAll(); + + window.dispatchEvent(new CustomEvent('appSuspended')); + XH.idleComponent = XH.app.showIdleDialog(() => { + window.location.reload(); I used the initial idleService PR (https://github.com/exhi/ hoist/pull/250/files) as source for these changes so I would be able to take compare the whole implementation instead of just looking at the final code in IdleService.js. That's why some things were missing like using an app config or user config to disable the service. I'll add the flag to disable cache. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#427 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACM16TzUlcYvliPjSa3Lv_r0VcRZ64nfks5uE6i2gaJpZM4VAJ_c> .

+ enhancement to MessageModel for direct open + LoadState => AppState

# Conflicts: # core/XH.js

lbwexler · 2018-07-11T04:09:26Z

I refactored this a bit, and it led to a larger refactor to create a new notion of AppState, which includes AppState.SUSPENDED.

Also simplified the implementation of IdleService and its timers, and also the way in which applications override the default component.

Also some unrelated file moves from '/app' package to '/impl'.

cr: atm (partially, IdleService review still pending)

gportela85 requested a review from lbwexler July 3, 2018 01:29

gportela85 force-pushed the idleService branch from d595e66 to 7972625 Compare July 6, 2018 06:45

lbwexler reviewed Jul 9, 2018

View reviewed changes

gportela85 added 6 commits July 9, 2018 13:33

added idleService

aba7b40

XH.idleComponent should be configurable at app level

0dcadf1

fire appSuspended event

d68f83e

appLevel/userLevel disable idle detection

b4ac868

Cancel all tasks

a2f2c3a

only add idleWrapper if LoadState is COMPLETE

9fc960d

gportela85 force-pushed the idleService branch from 77d894c to 9fc960d Compare July 9, 2018 18:33

lbwexler reviewed Jul 9, 2018

View reviewed changes

gportela85 and others added 5 commits July 9, 2018 14:27

removed idleWrapper

f91451d

code cleanup

2ee13c2

+ Refactor of idleDetection/AppSuspension

57fcd34

+ enhancement to MessageModel for direct open + LoadState => AppState

Merge branch 'develop' into idleService

da55fbc

# Conflicts: # core/XH.js

Linting and merge fixes

bc7287e

lbwexler merged commit 4b7d9ec into develop Jul 11, 2018

lbwexler deleted the idleService branch July 11, 2018 04:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IdleService #427

IdleService #427

gportela85 commented Jul 3, 2018

lbwexler commented Jul 5, 2018

TomTirapani commented Jul 6, 2018

lbwexler Jul 9, 2018 •

edited

Loading

lbwexler Jul 9, 2018

lbwexler Jul 9, 2018

lbwexler Jul 9, 2018

lbwexler Jul 9, 2018

lbwexler Jul 9, 2018

gportela85 Jul 9, 2018

lbwexler Jul 9, 2018 •

edited

Loading

lbwexler commented Jul 9, 2018 via email

lbwexler commented Jul 11, 2018

IdleService #427

IdleService #427

Conversation

gportela85 commented Jul 3, 2018

lbwexler commented Jul 5, 2018

TomTirapani commented Jul 6, 2018

lbwexler Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

lbwexler Jul 9, 2018

Choose a reason for hiding this comment

lbwexler Jul 9, 2018

Choose a reason for hiding this comment

lbwexler Jul 9, 2018

Choose a reason for hiding this comment

lbwexler Jul 9, 2018

Choose a reason for hiding this comment

lbwexler Jul 9, 2018

Choose a reason for hiding this comment

gportela85 Jul 9, 2018

Choose a reason for hiding this comment

lbwexler Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

lbwexler commented Jul 9, 2018 via email

lbwexler commented Jul 11, 2018

lbwexler Jul 9, 2018 •

edited

Loading

lbwexler Jul 9, 2018 •

edited

Loading