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

Drop modelDB from code editor #13247

Merged
merged 15 commits into from Nov 4, 2022

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Oct 14, 2022

References

Follow-up of #13168

Code changes

  • Remove modelDB from CodeEditor.Model
  • Use directly metadata from YJS.
  • CellModel and AttachementCellModel are now an abstract classes as the constructor does not need to access the cell type.
  • Remove onCellMoved on StaticNotebook and Notebook as motion does not exist. A motion will always be a removal + an addition (even when we will use the yjs move feature as there is no event for motion).
  • This also fixes the logic of cell selection when moving them.

User-facing changes

N/A

Backwards-incompatible changes

Break api for dealing with metadata for cells and notebook as it is not a observable json any longer.
CellModel and AttachementCellModel are now an abstract classes as the constructor does not need to access the cell type.

@jupyterlab-probot
Copy link

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

Copy link
Member

@hbcarlos hbcarlos left a comment

Choose a reason for hiding this comment

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

Thanks, @fcollonval! I left some comments.

packages/shared-models/src/api.ts Outdated Show resolved Hide resolved
Comment on lines 1199 to 1203
notebook.nbformat = nbformat.MAJOR_VERSION;
notebook.nbformat_minor = nbformat.MINOR_VERSION;
if (!notebook.ymeta.get('metadata')) {
notebook.ymeta.set('metadata', {});
}
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't initialize the YNotebook here. We re-initialize the nbformat and metadata every time a new client joins because we use this static method to instantiate the YNotebook here:

this.sharedModel = YNotebook.create({
disableDocumentWideUndoRedo: options.disableDocumentWideUndoRedo ?? false
});

This is done for every client joining the session. Instead, we should initialize the document in the backend (only the first client will initialize it) when in collaborative mode and in the initialize method of the NotebookModel when we are not in collaborative mode.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the initialization of documents doesn't have a good solution due to having the document registry in the frontend with all the information regarding the documents.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove the static methods.

this._ensureMetadata();
this._metadata.changed.connect(this._onMetadataChanged, this);
this._deletedCells = [];
this._ensureMetadata(options.languagePreference ?? '');
Copy link
Member

Choose a reason for hiding this comment

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

We should not modify the YNotebook here. Instead, we should do it on the backend and NotebookModel.initialize. Otherwise, we are re-initializing the metadata every time a new client joins.

The problem, the languagePreference is only available in the frontend.

Copy link
Member

Choose a reason for hiding this comment

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

Same as here

packages/docregistry/src/default.ts Outdated Show resolved Hide resolved
packages/cells/src/model.ts Show resolved Hide resolved
@fcollonval fcollonval merged commit cb9551c into jupyterlab:master Nov 4, 2022
@fcollonval fcollonval deleted the enh/drop-more-model-db branch November 4, 2022 10:25
brichet added a commit to brichet/jupyterlab that referenced this pull request Nov 16, 2022
@jtpio
Copy link
Member

jtpio commented Nov 18, 2022

Break api for dealing with metadata for cells and notebook as it is not a observable json any longer.
CellModel and AttachementCellModel are now an abstract classes as the constructor does not need to access the cell type.

There doesn't seem to be an update to the extension migration guide as part of this update: https://jupyterlab.readthedocs.io/en/latest/extension/extension_migration.html#api-breaking-changes

Were you planning to add something to this guide in a follow-up PR? Or maybe it's tracked in a separate issue?

@hbcarlos
Copy link
Member

@jtpio We are still making some changes to the API. I'll take care of documenting everything once we have a final version of the changes we need for Lab v4.0

@jtpio
Copy link
Member

jtpio commented Nov 22, 2022

Thanks @hbcarlos, I just noticed there is an issue about this for 3.6.0: #13332 👍

brichet added a commit to brichet/jupyterlab that referenced this pull request Nov 28, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Nov 30, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 2, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 5, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 6, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 12, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 15, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 20, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 20, 2022
brichet added a commit to brichet/jupyterlab that referenced this pull request Dec 21, 2022
fcollonval added a commit that referenced this pull request Dec 22, 2022
)

* Allow new sections in notebooktools, and set the 'common tools' section as RankedPanel

* Add a first version of the metadataform extension

* Update schema for a more generic usage mode. Several keys anywhere in the metadata set can now be used in a single form.

* Add a tsx file for the form itself

* Some refactoring for a better readability

* Add rjsf livevalidation and remove duplicate objects

* Insert the new section before the advancedTools by default

* Allows an extenal extension to register and use it own widget in form

* Allows any RJSF ui options in settings

* (1) Allows default values, which are not written in metadata if selected, and (2) clean empty nested object when a nested metadata is deleted

* Allows enable some metadata depending on cell type

* Improve the layout with templates and style

* update packages and clean schema

* update package files

* Includes modification in metapackage

* Automatic application of license header

* Update packages/ui-components-extension/src/index.ts

Rename registry constant

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

* Wraps the renderer in an interface

* Fix the callbacks on MetadataFormWidgets when an event occurred on Notebook

* Avoid updating the form if the widget is hidden

* Use common RJSF template objects between settings-editor and metadata-form

* Fixing UI tests

* Update Playwright Snapshots

* Put together meta information associated to properties

* Allow working on notebook level metadata

* Add dependency to @jupyterlab/observables

* prettier...

* Allow using custom fields in addition of custom widgets

* (1) Homogenizes registries and (2) revertes form update after updating metadata

* updateMetadata method can now be called from a custom field, and can reload the form too

* Fix integrity

* Invert logic of showing modified from default

* Allows external extension to update the properties of a form

* (1) Allows adding field(s) in an existing form, from an extension, and (2) orders fields if a rank is provided

* Refactoring of the setting's schema to match the ones from react-JSONschema-form (JSONSchema and uiSchema)

* Add support for 'allOf' feature of RJSF, to bring the conditional fields

* Send the whole metadataSchema to the form (after having removed the non relevent), instead of rebuilding the form schema

* Add tests on metadataform schema

* Split package in a singleton and an extension package

* Add unit test on metadataFromWidget

* Update packages/metadataform-extension/package.json

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* Update packages/metadataform-extension/package.json

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* Update packages/metadataform/package.json

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* Update packages/metadataform/package.json

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* Update packages/metadataform/package.json

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* fix syntax and update dependencies

* fix syntax

* (1) Allow showing the default value when the current is not the default, and (2) allow avoiding writing default value to metadata

* fix eslint

* Update documentation snapshot

* Update packages/metadataform/src/form.tsx

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

* Update packages/metadataform/src/form.tsx

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

* Update packages/metadataform/src/token.ts

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

* Includes all options of MetadataFormWidget in one object

* Include an interface for the object provided by the extension

* Includes conditional fields when wondering metadataKeys

* Update changes in usage of metadatada after #13247

* fix integrity

* Fix metadataform tests

* Fix metadataform ui-test

* re-align version number

* Bump to Jest 29 (follows #12584)

* Fix Jest tests by adding 'nanoid' in ignored packages

* Update Playwright Snapshots

* Automatic application of license header

* Update documentation snapshot

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Co-authored-by: foo <foo@bar.com>
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants