Read only status notification on ctrl+s

[andrewfulton9]

References

adds feature described in #15046

depends on lumino #644 to function

Code changes

Uses contextual information passed from lumino commands _executeKeyBinding via args to modify execution if the save is called from a keybinding to raise a notification notifying a user that the file is read-only and can't be saved

User-facing changes

If a user trys to save read-only document with the keybinding "ctr+s", a notification will briefly pop up notifying them that the document is read-only and cannot be saved.

read-only.notification.mp4

Backwards-incompatible changes

None that I am aware

[jupyterlab-probot]

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

[krassowski]

Kicking CI which failed due to npm outage: https://status.npmjs.org/incidents/zdznxkrp22py

[krassowski]

Could you look at how hard adding an integration test would be here? I does not need to include a snapshot as there is a dedicated page.notifications galata helper which could be used, as in:

jupyterlab/galata/test/jupyterlab/announcements.test.ts

Lines 15 to 26 in c6e7a14

	test('Announcements requires user agreement', async ({ page }) => {
	const notifications = await page.notifications;
	expect(notifications).toHaveLength(1);
	expect(notifications[0].message).toEqual(
	'Would you like to receive official Jupyter news?\nPlease read the privacy policy.'
	);
	expect(notifications[0].options.actions).toHaveLength(3);
	expect(notifications[0].options.actions[0].label).toEqual(
	'Open privacy policy'
	);
	});

[andrewfulton9]

Could you look at how hard adding an integration test would be here? I does not need to include a snapshot as there is a dedicated page.notifications galata helper which could be used, as in:

jupyterlab/galata/test/jupyterlab/announcements.test.ts

Lines 15 to 26 in c6e7a14

	test('Announcements requires user agreement', async ({ page }) => {
	const notifications = await page.notifications;
	expect(notifications).toHaveLength(1);
	expect(notifications[0].message).toEqual(
	'Would you like to receive official Jupyter news?\nPlease read the privacy policy.'
	);
	expect(notifications[0].options.actions).toHaveLength(3);
	expect(notifications[0].options.actions[0].label).toEqual(
	'Open privacy policy'
	);
	});

@krassowski, I'm a little lost trying to figure out how to make a test notebook read only. Do you have any suggestions on how I should do this?

[krassowski]

Good question. I think you that setting immutable attribute could work on CI but this would annoy anyone running tests locally as it would require a manual/OS-specific step before running the test. Probably the best option would be adding a mock that forces the "writable" attribute to false. You can follow the existing mocks such as freezeContentLastModified:

jupyterlab/galata/src/galata.ts

Lines 573 to 625 in 453adc4

	export async function freezeContentLastModified(page: Page): Promise<void> {
	// Listen for closing connection (may happen when request are still being processed)
	let isClosed = false;
	const ctxt = page.context();
	ctxt.once('close', () => {
	isClosed = true;
	});
	ctxt.browser()?.once('disconnected', () => {
	isClosed = true;
	});
	return page.route(Routes.contents, async (route, request) => {
	switch (request.method()) {
	case 'GET': {
	// Proxy the GET request
	const response = await ctxt.request.fetch(request);
	if (!response.ok()) {
	if (!page.isClosed() && !isClosed) {
	return route.fulfill({
	status: response.status(),
	body: await response.text()
	});
	}
	break;
	}
	const data = await response.json();
	// Modify the last_modified values to be set one day before now.
	if (
	data['type'] === 'directory' &&
	Array.isArray(data['content'])
	) {
	const now = Date.now();
	const aDayAgo = new Date(now - 24 * 3600 * 1000).toISOString();
	for (const entry of data['content'] as any[]) {
	// Mutate the list in-place
	entry['last_modified'] = aDayAgo;
	}
	}
	if (!page.isClosed() && !isClosed) {
	return route.fulfill({
	status: 200,
	body: JSON.stringify(data),
	contentType: 'application/json'
	});
	}
	break;
	}
	default:
	return route.continue();
	}
	});
	}

[krassowski]

The test looks good, thank you! Two suggestions/questions on:

• showing just file name not path
• throwing an error if command gets executed when it should not be.

[krassowski]

Should it just show the base name? Or is showing the full path valuable here?

[andrewfulton9]

I see it as being a tradeoff between full path being more explicit vs just the file name potentially being more tidy. Personally I would lean towards the more explicit option. You probably have a better understanding of JLab users though, so I'm happy to defer to your opinion. It's also worth noting that the same function and input are being used for the notification for the save-all and save commands though. I could see the full path being better for the save-all notification even if we update the save notification to just use the base name.

[krassowski]

Thanks @andrewfulton9