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

Fix spurious "File Changed" dialogs using hash from jupyter-server v2.11.1+ #15577

Merged
merged 10 commits into from Jan 27, 2024

Conversation

Wh1isper
Copy link
Contributor

@Wh1isper Wh1isper commented Dec 29, 2023

References

Fixes #7743
Fixes #11352

References PR: #15037

Upstream:

Code changes

Since jupyter server may provide hash in model, we compare hash for the file changes first.

Forgive me for being a TypeScript and front-end novice, more advice would be appreciated, thanks!

User-facing changes

N/A

Backwards-incompatible changes

N/A

Copy link

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

Copy link

welcome bot commented Dec 29, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@Chralay Chralay mentioned this pull request Jan 4, 2024
@krassowski krassowski self-requested a review January 17, 2024 17:56
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! It looks good except for one condition which I am not sure about.

Would you consider also adding a test for this? I think it could be added along:

describe('#fileChanged', () => {
it('should be emitted when the file is saved', async () => {
const path = context.path;
let called = false;
context.fileChanged.connect((sender, args) => {
expect(sender).toBe(context);
expect(args.path).toBe(path);
called = true;
});
await context.initialize(true);
expect(called).toBe(true);
});
it('should not contain the file content attribute', async () => {
let called = false;
context.fileChanged.connect((sender, args) => {
// @ts-expect-error content is omitted
expect(args['content']).toBeUndefined();
called = true;
});
await context.initialize(true);
expect(called).toBe(true);
});
});

packages/docregistry/src/context.ts Outdated Show resolved Hide resolved
packages/docregistry/src/context.ts Outdated Show resolved Hide resolved
packages/docregistry/src/context.ts Outdated Show resolved Hide resolved
packages/docregistry/src/context.ts Outdated Show resolved Hide resolved
packages/docregistry/src/context.ts Outdated Show resolved Hide resolved
Wh1isper and others added 2 commits January 23, 2024 09:16
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
@Wh1isper
Copy link
Contributor Author

I will add a test on this soon, thanks!

@Wh1isper
Copy link
Contributor Author

Wh1isper commented Jan 26, 2024

I changed some logic about emit fileChanged and added hash and hash_algorithm in mock.

I have tested following situations manually:

  • When hash is provided, fileChanged is not emitted.
  • When hash is not provided, fileChanged is emitted because of timeStamp changed.

But I don't know how to reorganise the test cases to support both scenarios.

Feel free to give me more feed back, thanks!

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @Wh1isper!

I pushed a few more tests and tiny adjustments in the conditionals.

@krassowski krassowski changed the title Feature: Intro hash and hash_algorithm for saving Fix spurious "File Changed" dialogs using hash from jupyter-server v2.11.1+ Jan 27, 2024
@krassowski krassowski merged commit e38e0b8 into jupyterlab:main Jan 27, 2024
77 of 78 checks passed
Copy link

welcome bot commented Jan 27, 2024

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment