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.

Sign up for GitHub

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

Jump to bottom

Use new Poll class for filebrowser contents polling. #6305

Merged
merged 7 commits into from May 11, 2019
Merged

Use new Poll class for filebrowser contents polling. #6305

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a93826
Use new Poll class for filebrowser contents polling.
ian-r-rose May 4, 2019
07d7b4d
Get rid of `scheduleUpdate`, retain minimum refresh time.
ian-r-rose May 4, 2019
e3ee844
Remove logic around MIN_REFRESH interval.
ian-r-rose May 4, 2019
627b1db
Add explanatory note to poll refresh function.
ian-r-rose May 4, 2019
99faafa
Don't await poll.refresh in filebrowser model refresh, as it cancels
ian-r-rose May 4, 2019
be4cc1a
Use poll refresh, fix up test.
ian-r-rose May 4, 2019
6746f7d
Void unhandled promise.
ian-r-rose May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 22 additions & 30 deletions packages/filebrowser/src/model.ts
View file
Open in desktop
Expand Up @@ -5,7 +5,8 @@ import {
IChangedArgs,
IStateDB,
PathExt,
PageConfig
PageConfig,
Poll
} from '@jupyterlab/coreutils';

import { IDocumentManager, shouldOverwrite } from '@jupyterlab/docmanager';
Expand Down Expand Up @@ -87,8 +88,7 @@ export class FileBrowserModel implements IDisposable {
format: 'text'
};
this._state = options.state || null;
this._baseRefreshDuration =
options.refreshInterval || DEFAULT_REFRESH_INTERVAL;
const refreshInterval = options.refreshInterval || DEFAULT_REFRESH_INTERVAL;

const { services } = options.manager;
services.contents.fileChanged.connect(this._onFileChanged, this);
Expand All @@ -104,7 +104,23 @@ export class FileBrowserModel implements IDisposable {
};
window.addEventListener('beforeunload', this._unloadEventListener);
this._scheduleUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Something should be done with _scheduleUpdate too - that should probably be going away with the conversion to using Poll.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the refresh() method may need some work too.

Copy link
Member Author

Choose a reason for hiding this comment

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

_scheduleUpdate has some logic around minimum refresh intervals, which I was endeavoring to leave intact for now. This is triggered upon fileChanged signals. I'm not aware of the history of that logic, but had thought that maybe there was some debouncing that was necessary.

If not, I'm happy to simplify this further, was just starting out with a more conservative approach.

this._startTimer();
this._poll = new Poll({
factory: async () => {
if (this._requested) {
return this.refresh();
}
let date = new Date().getTime();
if (date - this._lastRefresh > refreshInterval) {
return this.refresh();
}
Copy link
Contributor

@jasongrout jasongrout May 4, 2019 •
edited

Choose a reason for hiding this comment

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

All of this logic can go away with the new Poll class, I think. It's sufficient to just call the update method. When we want to have the poll immediately refresh, we call the poll's refresh method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point. It also does some logic around a minimum refresh interval, which I tried to keep intact.

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a minimum interval option in Poll, but it didn't handle manual refreshes (it assumed a manual refresh was always immediate). Perhaps we add a minimum refresh interval to Poll for this debouncing. I think it would be pretty straightforward - the poll refresh() method would just schedule using logic like here.

So this logic isn't really a true debounce, in that it doesn't schedule a refresh according to MIN_REFRESH, it simply ignores requests if they are too soon after the last tick. That's perhaps a little frustrating - you click and click and click, and if you stop clicking before MIN_REFRESH, it's like you never clicked at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it's a bit more complicated for Poll, since we really want to debounce the callback (and every time we are about to call the callback, we check to see if we should go into standby, etc.). So that means the Poll should really be storing the last time it called the callback and make sure it's not calling the callback more often than it should.

},
frequency: {
interval: refreshInterval,
backoff: true,
max: 300 * 1000
},
standby: 'when-hidden'
});
}

/**
Expand Down Expand Up @@ -198,7 +214,7 @@ export class FileBrowserModel implements IDisposable {
}
window.removeEventListener('beforeunload', this._unloadEventListener);
this._isDisposed = true;
clearTimeout(this._timeoutId);
this._poll.dispose();
this._sessions.length = 0;
this._items.length = 0;
Signal.clearData(this);
Expand Down Expand Up @@ -269,7 +285,6 @@ export class FileBrowserModel implements IDisposable {
if (this.isDisposed) {
return;
}
this._refreshDuration = this._baseRefreshDuration;
this._handleContents(contents);
this._pendingPath = null;
if (oldValue !== newValue) {
Expand All @@ -296,7 +311,6 @@ export class FileBrowserModel implements IDisposable {
this._connectionFailure.emit(error);
return this.cd('/');
} else {
this._refreshDuration = this._baseRefreshDuration * 10;
this._connectionFailure.emit(error);
}
});
Expand Down Expand Up @@ -605,26 +619,6 @@ export class FileBrowserModel implements IDisposable {
});
}

/**
* Start the internal refresh timer.
*/
private _startTimer(): void {
this._timeoutId = window.setInterval(() => {
if (this._requested) {
void this.refresh();
return;
}
if (document.hidden) {
// Don't poll when nobody's looking.
return;
}
let date = new Date().getTime();
if (date - this._lastRefresh > this._refreshDuration) {
void this.refresh();
}
}, MIN_REFRESH);
}

/**
* Handle internal model refresh logic.
*/
Expand All @@ -651,15 +645,13 @@ export class FileBrowserModel implements IDisposable {
private _requested = false;
private _sessions: Session.IModel[] = [];
private _state: IStateDB | null = null;
private _timeoutId = -1;
private _refreshDuration: number;
private _baseRefreshDuration: number;
private _driveName: string;
private _isDisposed = false;
private _restored = new PromiseDelegate<void>();
private _uploads: IUploadModel[] = [];
private _uploadChanged = new Signal<this, IChangedArgs<IUploadModel>>(this);
private _unloadEventListener: (e: Event) => string;
private _poll: Poll;
}

/**
Expand Down