Maintain autosave timers while disconnected

[holzman]

References

Fixes #16798.

Code changes

The autosave feature works by starting a timer. When the timer expires, it clears itself; then, if the session is connected, it attempts a save (which then restarts the timer for the next save). However, if the session is disconnected, the timer never gets restarted - this code just calls _setTimer() in that case.

User-facing changes

Backwards-incompatible changes

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[krassowski]

Sounds sensible! Would you be able to add a test that fails before the fix and passes after to the following test suite?

jupyterlab/packages/docmanager/test/savehandler.spec.ts

Lines 45 to 210 in 9cc6267

	describe('SaveHandler', () => {
	describe('#constructor()', () => {
	it('should create a new save handler', () => {
	expect(handler).toBeInstanceOf(SaveHandler);
	});
	});
	describe('#saveInterval()', () => {
	it('should be the save interval of the handler', () => {
	expect(handler.saveInterval).toBe(120);
	});
	it('should be set-able', () => {
	handler.saveInterval = 200;
	expect(handler.saveInterval).toBe(200);
	});
	});
	describe('#isActive', () => {
	it('should test whether the handler is active', () => {
	expect(handler.isActive).toBe(false);
	handler.start();
	expect(handler.isActive).toBe(true);
	});
	});
	describe('#isDisposed', () => {
	it('should test whether the handler is disposed', () => {
	expect(handler.isDisposed).toBe(false);
	handler.dispose();
	expect(handler.isDisposed).toBe(true);
	});
	it('should be true after the context is disposed', () => {
	context.dispose();
	expect(handler.isDisposed).toBe(true);
	});
	});
	describe('#dispose()', () => {
	it('should dispose of the resources used by the handler', () => {
	expect(handler.isDisposed).toBe(false);
	handler.dispose();
	expect(handler.isDisposed).toBe(true);
	handler.dispose();
	expect(handler.isDisposed).toBe(true);
	});
	});
	describe('#start()', () => {
	it('should start the save handler', () => {
	handler.start();
	expect(handler.isActive).toBe(true);
	});
	it('should trigger a save', () => {
	const promise = signalToPromise(context.fileChanged);
	context.model.fromString('bar');
	expect(handler.isActive).toBe(false);
	handler.saveInterval = 0.1;
	handler.start();
	return promise;
	});
	it('should continue to save', async () => {
	let called = 0;
	// Lower the duration multiplier.
	(handler as any)._multiplier = 1;
	const promise = testEmission(context.fileChanged, {
	test: () => {
	if (called === 0) {
	context.model.fromString('bar');
	called++;
	}
	return called === 1;
	}
	});
	context.model.fromString('foo');
	expect(handler.isActive).toBe(false);
	handler.saveInterval = 0.1;
	handler.start();
	return promise;
	});
	it('should overwrite the file on disk', async () => {
	const delegate = new PromiseDelegate();
	// Lower the duration multiplier.
	(handler as any)._multiplier = 1;
	context.model.fromString('foo');
	await context.initialize(true);
	// The context allows up to 0.5 difference in timestamps before complaining.
	setTimeout(async () => {
	await manager.contents.save(context.path, {
	type: factory.contentType,
	format: factory.fileFormat,
	content: 'bar'
	});
	handler.saveInterval = 1;
	handler.start();
	context.model.fromString('baz');
	context.fileChanged.connect(() => {
	expect(context.model.toString()).toBe('baz');
	delegate.resolve(undefined);
	});
	}, 1500);
	// Extend the timeout to wait for the dialog because of the setTimeout.
	await acceptDialog(document.body, 3000);
	await delegate.promise;
	});
	it('should revert to the file on disk', async () => {
	const delegate = new PromiseDelegate();
	const revert = () => {
	const dialog = document.body.getElementsByClassName('jp-Dialog')[0];
	const buttons = dialog.getElementsByTagName('button');
	for (let i = 0; i < buttons.length; i++) {
	if (buttons[i].textContent === 'Revert') {
	buttons[i].click();
	return;
	}
	}
	};
	// Lower the duration multiplier.
	(handler as any)._multiplier = 1;
	await context.initialize(true);
	context.model.fromString('foo');
	context.fileChanged.connect(() => {
	expect(context.model.toString()).toBe('bar');
	delegate.resolve(undefined);
	});
	// The context allows up to 0.5 difference in timestamps before complaining.
	setTimeout(async () => {
	await manager.contents.save(context.path, {
	type: factory.contentType,
	format: factory.fileFormat,
	content: 'bar'
	});
	handler.saveInterval = 1;
	handler.start();
	context.model.fromString('baz');
	}, 1500);
	// Extend the timeout to wait for the dialog because of the setTimeout.
	await waitForDialog(document.body, 3000);
	revert();
	await delegate.promise;
	});
	});
	describe('#stop()', () => {
	it('should stop the save timer', () => {
	handler.start();
	expect(handler.isActive).toBe(true);
	handler.stop();
	expect(handler.isActive).toBe(false);
	});
	});
	});
	});

[holzman]

Added a test.

[JasonWeill]

@krassowski All tests are now passing. OK to merge and backport to 4.3.x?

[kellyrowland]

This should get backported further back than 4.3.x, no?

[JasonWeill]

We typically only backport changes further upon request. @kellyrowland How far back should we backport this fix once it's been merged into main?

[kellyrowland]

Ideally as far back as the version where the bug was introduced, but I would settle for 4.2.x 🙂

[krassowski]

Ideally tests should interface with the public API rather than private implementation details, as the implementation may change more rapidly and the test will need modifying (or worse would get deleted). That said I do not want to hold this off so if anyone else is happy to approve as-is I will not block it.

[holzman]

Good point - I've revised the test.

[holzman]

BTW, while poking around the docmanager tests, I found one that wasn't really testing anything and fixed it in #16933.

[krassowski]

Thank you @holzman!

[krassowski]

@meeseeksdev please backport to 4.3.x
@meeseeksdev please backport to 4.2.x

[kellyrowland]

Is there an ETA for when these backports will get a release? We'd really like to get the fix out to our users.

[JasonWeill]

@jupyterlab/release Per comment above, is now an appropriate time to cut new 4.2.x and 4.3.x releases to include backports of this fix?

[jtpio]

@JasonWeill sure, if you would like to cut the releases please feel free to do so!

I think @krassowski mentioned somewhere that he wanted to make a 4.3.x release too this week.

[krassowski]

Yes 4.3.x is on the menu today. Not sure if 4.2.x is ready to go I will let @JasonWeill make the call.

[krassowski]

As of today it looks like this would be the only user-facing change on 4.2.6: v4.2.5...4.2.x.

[JasonWeill]

@kellyrowland Version 4.2.6 is now available, which includes this change: https://github.com/jupyterlab/jupyterlab/releases/tag/v4.2.6

[kellyrowland]

Thank you!

[JasonWeill]

v4.3.1 is now also released, and that also includes this fix. Thank you @krassowski !