Widget with toolbar and sidepanel in cell output #419

brichet · 2025-01-31T13:08:49Z

Fixes #397

This PR:

creates a new widget (JupyterGISOutputWidget) extending main area widget, to be used in the cell output. It extends the main area widget to ease the integration of the toolbar, but it could a simplest widget (to be discussed ?)
allows the tracker to track these new widget, which enables the toolbar buttons and the side panel
the document context is often used to get the document path and model. This PR ~~adds a simplified context interface to share the same interface between document widget (main area) and JupyterGISOutputWidget, and~~ allows the commands to work with both widgets.
uses the localPath property of the context instead of path, to avoid bringing the RTC: prefix.

EDIT

the document context was often used to get the model (IJupyterGISModel) or the document path (also contained in the model). The new JupyterGISOutputWidget is not a document widget and do not have a proper Context. This PR remove the use of the context and use directly the model instead, which can easily be retrieved from JupyterGISOutputWidget or JupyterGISWidget, the 2 widget tracked by the WidgetTracker.

Checklist

PR has a descriptive title and content.
PR description contains references to any issues the PR resolves, e.g. Resolves #XXX.
PR has one of the labels: documentation, bug, enhancement, feature, maintenance
Checks are passing.
Failing lint checks can be resolved with:
- pre-commit run --all-files
- jlpm run lint

📚 Documentation preview: https://jupytergis--419.org.readthedocs.build/en/419/
💡 JupyterLite preview: https://jupytergis--419.org.readthedocs.build/en/419/lite

github-actions · 2025-01-31T13:09:02Z

👈 Launch a Binder on branch brichet/jupytergis/cell_output_document_widget

github-actions · 2025-01-31T13:20:46Z

Integration tests report: appsharing.space

brichet · 2025-01-31T17:52:41Z

I keep it as draft as I can't find a way to handle the resize event, which leads to toolbar elements hidden.

brichet · 2025-02-02T21:52:30Z

I keep it as draft as I can't find a way to handle the resize event, which leads to toolbar elements hidden.

It should work now.

brichet · 2025-02-03T13:54:49Z

Please update snapshots

brichet

Here are the 2 main changes that could be ported to JupyterCAD.

Other changes may be related to JupyterGIS only.

brichet · 2025-02-03T13:57:02Z

packages/base/src/widget.ts

+/**
+ * A main area widget designed to be used as Notebook cell output widget, to ease the
+ * integration of toolbar and tracking.
+ */
+export class JupyterGISOutputWidget
+  extends MainAreaWidget<JupyterGISPanel>
+  implements IJupyterGISOutputWidget
+{
+  constructor(options: JupyterGISOutputWidget.IOptions) {
+    super(options);
+    this.addClass(CELL_OUTPUT_WIDGET_CLASS);
+    this.model = options.model;
+
+    const resizeObserver = new ResizeObserver(() => {
+      // Send a resize message to the widget, to update the child size.
+      MessageLoop.sendMessage(this, Widget.ResizeMessage.UnknownSize);
+    });
+    resizeObserver.observe(this.node);
+  }
+
+  readonly model: IJupyterGISModel;
+}
+
+export namespace JupyterGISOutputWidget {
+  export interface IOptions extends MainAreaWidget.IOptions<JupyterGISPanel> {
+    model: IJupyterGISModel;
+  }
+}
+


The widget displayed in cell output.

brichet · 2025-02-03T13:57:42Z

python/jupytergis_lab/src/notebookrenderer.ts

+    const content = new JupyterGISPanel({ model });
+    const toolbar = new ToolbarWidget({
+      commands,
+      model,
+      externalCommands: externalCommands.getCommands()
+    });
+    this._jgisWidget = new JupyterGISOutputWidget({
+      model,
+      content,
+      toolbar
+    });
+


Where it is created in the notebook renderer.

…ents and tracker

for more information, see https://pre-commit.ci

martinRenou · 2025-02-03T14:30:31Z

python/jupytergis_lab/jupytergis_lab/notebook/gis_document.py

@@ -72,7 +72,9 @@ def __init__(
        ydoc = Doc()

        super().__init__(
-            comm_metadata=dict(ymodel_name="@jupytergis:widget", **comm_metadata),
+            comm_metadata=dict(
+                ymodel_name="@jupytergis:widget", cwd=os.getcwd(), **comm_metadata


Do we really need this? cc. @davidbrochart for awareness

It is required to be able to add local layer from the widget buttons.
It could probably be removed if we don't use file path relative to the JGIS project file.

It is required to be able to add local layer from the widget buttons

Once the JGIS file is created, we know its "cwd" is the path to the file, right? Since we know that path in the front-end already, we may not need to pass this info from Python?

Yes, it is not used if a path is provided.
But we want to create a GISDocument without file, no ?

doc = GISDocument()

In that case, we can assume the "cwd" is the Notebook path, and since we have the notebook tracker handy, we can get its path?

All this situation is not great. We need a better approach but I don't have a good quick solution to suggest.

See #426 (comment)

Are the widgets supported in console ? I don't know how to display it in console.
If not, we don't need the path, it is useful for the buttons only, not for the API I think.

Otherwise something like the following should work in the front-end :

const currentWidget = app.shell.currentWidget; let currentPath: string | undefined = undefined; if (currentWidget instanceof NotebookPanel && notebookTracker) { currentPath = notebookTracker.currentWidget?.context.localPath; } else if (currentWidget instanceof ConsolePanel && consoleTracker) { currentPath = consoleTracker.currentWidget?.sessionContext.path; } if (currentPath) { this.jupyterGISModel.filePath = PathExt.join( PathExt.dirname(currentPath), 'unsaved_project' ); }

That should work in most cases. It's only when people changed did os.chdir that it will break this. But I guess we can live with this.

mfisher87

Beautiful 🤩

packages/base/src/annotations/model.ts

packages/base/src/types.ts

mfisher87 · 2025-02-03T16:45:30Z

packages/base/src/panelview/rightpanel.tsx

+        header.title.label = this._currentModel.filePath;
+        this._annotationModel.model =
+          options.tracker.currentWidget?.model || undefined;
+        // await changed.context.ready;


Suggested change

// await changed.context.ready;

Yes, thanks for remind me this.
It seems to work without, but I don't know if it was necessary and should be kept somehow.

mfisher87 · 2025-02-03T16:48:02Z

packages/base/src/widget.ts

@@ -22,6 +28,10 @@ export class JupyterGISWidget
    super(options);
  }

+  get model(): IJupyterGISModel {
+    return this.context.model;
+  }


🤩

This refactor to pass around model instead of context feels so much simpler!!

mfisher87 · 2025-02-04T03:24:21Z

@brichet it would be amazing to release this feature before the hackathon on Wednesday. How do you feel about that?

I didn't approve because although I read the entire PR, I don't feel I have the expertise to "approve" on this project yet. I have a lot to learn from you all! :)

…g the API without file

for more information, see https://pre-commit.ci

…utput widget

…necessary

…that can be updated between the model creation and the widget creation.

martinRenou · 2025-02-04T14:23:47Z

python/jupytergis_lab/src/notebookrenderer.ts

+          if (currentWidgetPath) {
+            this.jupyterGISModel.filePath = PathExt.join(
+              PathExt.dirname(currentWidgetPath),
+              'unsaved_project'


Instead of giving this file path, what would it take to create the file from the front-end? If no file name is provided, we create an "untitled" file, if a new file path is provided (something we don't know) we create the file.

In replacement of what is removed in https://github.com/geojupyter/jupytergis/pull/426/files

I think it is also nice to be able to debug without creating a file. And to add a save() method to the API, to be able to save the document.
But indeed, if the path is provided and does not exist, we could create it from the frontend.

I think it is also nice to be able to debug without creating a file. And to add a save() method to the API, to be able to save the document.

💯 💯 💯

brichet · 2025-02-06T09:04:08Z

Some more information about the changes in this PR.

There are still issues with the widgets when deleting a cell output.
We have to choose the least worst solution between:

doing nothing, the tracker still think this widget is the current and does not update side bar
informing the model that the cell output has been deleted and dispose of the model and widget (so the widget is removed from the tracker). But it will need to be instantiated again in the notebook to be displayed properly.
only dispose of the widget, which is then removed from the tracker. Since the model is kept in yjs-widgets registry, it can be displayed again. The side effect is that disposing of the widget remove it from all cell output where it is displayed.

The last solution I can think of (my preference) is to use some kind of isVisible on the widget, and display the side bar only if the widget is the current one and is visible. We can also use it to empty the side bar when the main area widget is not the current widget, it can be less confusing for users.

brichet · 2025-02-06T10:02:41Z

After some discussion with @martinRenou and @trungleduc the best is probably to go the first option temporary.

doing nothing, the tracker still think this widget is the current and does not update side bar

The idea is to fix this behavior upstream in yjs-widget, by properly dispose of the actual widget in the concerned cell output.

brichet

@martinRenou for me it is ready.
My only concern is commented below, and I don't know if this code is tested somewhere.

brichet · 2025-02-06T10:05:45Z

packages/base/src/panelview/annotationPanel.tsx

-    this._annotationModel.contextChanged.connect(async () => {
-      await this._annotationModel?.context?.ready;
+    this._annotationModel.modelChanged.connect(async () => {
+      // await this._annotationModel?.context?.ready;


@gjmooney do you remember if this await was required ? Should we restore something similar, now we removed the context ?

trungleduc · 2025-02-06T10:19:03Z

The idea is to fix this behavior upstream in yjs-widget, by properly dispose of the actual widget in the concerned cell output.

you are trackingthis._jgisWidget and not the widget disposed by yjs-widget, so you might need to dispose this._jgisWidget in YJupyterGISWidget.dispose method

brichet · 2025-02-06T12:34:38Z

The idea is to fix this behavior upstream in yjs-widget, by properly dispose of the actual widget in the concerned cell output.

Should be fixed with QuantStack/yjs-widgets#20

martinRenou

Looks great!

Let's merge :D

brichet added the enhancement New feature or request label Jan 31, 2025

brichet force-pushed the cell_output_document_widget branch from 60ef40e to fe38083 Compare January 31, 2025 13:19

martinRenou added the Needs Port To JupyterCAD label Jan 31, 2025

brichet marked this pull request as ready for review February 2, 2025 21:50

brichet force-pushed the cell_output_document_widget branch from 937f7a7 to 68d24a2 Compare February 2, 2025 21:51

brichet marked this pull request as draft February 3, 2025 07:48

brichet marked this pull request as ready for review February 3, 2025 13:55

brichet commented Feb 3, 2025

View reviewed changes

brichet and others added 13 commits February 3, 2025 15:18

Add minimal Context interface for jupytergis widgets

76b58e7

Add a new widget to be include in Notebook cell output

f20e098

Fix the relative path issue when add a layer in cell output widget

5624e3e

Use context 'localPath' instaed of 'path' to avoid 'RTC:' in the path

745d79b

Handle the resize event of the cell output widget

223b504

lint

906e6ef

Use the model (IJupyterGISModel) instead of the context in the compon…

e448960

…ents and tracker

lint

765edb8

Fix dependencies in '@jupytergis/jupytergis-lab' package

1adef39

Allow using the python API without file on disk

6715005

[pre-commit.ci] auto fixes from pre-commit.com hooks

464cf2c

for more information, see https://pre-commit.ci

lint

71deb9f

Update Playwright Snapshots

7f18130

brichet force-pushed the cell_output_document_widget branch from a795d35 to 7f18130 Compare February 3, 2025 14:19

martinRenou reviewed Feb 3, 2025

View reviewed changes

mfisher87 reviewed Feb 3, 2025

View reviewed changes

brichet and others added 7 commits February 4, 2025 08:57

Use the notebook or console path as temporary project path, when usin…

b363a1d

…g the API without file

[pre-commit.ci] auto fixes from pre-commit.com hooks

761436e

for more information, see https://pre-commit.ci

Rename widget for consistency, and remove console comands from cell o…

e374493

…utput widget

Build again the widget if the path is updated, to add the toolbar if …

953d127

…necessary

Add the widget to the tracker after rebuilt, and fix the model path, …

0918536

…that can be updated between the model creation and the widget creation.

Dispose of the widget when the model is disposed

e55c1c1

lint

e7fe9ab

martinRenou reviewed Feb 4, 2025

View reviewed changes

This was referenced Feb 5, 2025

Fix issue with file loading in widget #431

Closed

earthquakes.jGIS fails to load points when opened in widget #399

Closed

mfisher87 linked an issue Feb 5, 2025 that may be closed by this pull request

earthquakes.jGIS fails to load points when opened in widget #399

Closed

Do not dispose of the model when clearing cell output

59e83c5

brichet commented Feb 6, 2025

View reviewed changes

Dispose of the GIS widget when the yjs widget is disposed

724318e

brichet mentioned this pull request Feb 6, 2025

Add a dispose method to IJupyterYWidget QuantStack/yjs-widgets#20

Merged

martinRenou approved these changes Feb 6, 2025

View reviewed changes

martinRenou merged commit 2669fad into geojupyter:main Feb 6, 2025
14 checks passed

brichet deleted the cell_output_document_widget branch February 6, 2025 13:26

This was referenced Feb 6, 2025

Bump yjs-widgets to >=0.3.9 #449

Merged

[WIP] Prototype of streamlined Graphical GIS <-> Notebook "bounce" workflow #340

Draft

arjxn-py mentioned this pull request Feb 10, 2025

Widget with toolbar and sidepanel in cell output jupytercad/JupyterCAD#679

Merged

martinRenou removed the Needs Port To JupyterCAD label Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widget with toolbar and sidepanel in cell output #419

Widget with toolbar and sidepanel in cell output #419

brichet commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025 •

edited

Loading

brichet commented Jan 31, 2025

brichet commented Feb 2, 2025

brichet commented Feb 3, 2025

brichet left a comment

brichet Feb 3, 2025

brichet Feb 3, 2025

martinRenou Feb 3, 2025

brichet Feb 3, 2025

martinRenou Feb 3, 2025

brichet Feb 3, 2025

martinRenou Feb 3, 2025 •

edited

Loading

martinRenou Feb 3, 2025

martinRenou Feb 3, 2025

brichet Feb 3, 2025

brichet Feb 3, 2025 •

edited

Loading

martinRenou Feb 4, 2025

mfisher87 left a comment

mfisher87 Feb 3, 2025

brichet Feb 3, 2025

mfisher87 Feb 3, 2025

mfisher87 commented Feb 4, 2025

martinRenou Feb 4, 2025

brichet Feb 4, 2025

mfisher87 Feb 4, 2025

brichet commented Feb 6, 2025 •

edited

Loading

brichet commented Feb 6, 2025

brichet left a comment •

edited

Loading

brichet Feb 6, 2025

trungleduc commented Feb 6, 2025

brichet commented Feb 6, 2025

martinRenou left a comment

Widget with toolbar and sidepanel in cell output #419

Widget with toolbar and sidepanel in cell output #419

Conversation

brichet commented Jan 31, 2025 • edited Loading

Checklist

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025 • edited Loading

brichet commented Jan 31, 2025

brichet commented Feb 2, 2025

brichet commented Feb 3, 2025

brichet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinRenou Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brichet Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisher87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisher87 commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brichet commented Feb 6, 2025 • edited Loading

brichet commented Feb 6, 2025

brichet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trungleduc commented Feb 6, 2025

brichet commented Feb 6, 2025

martinRenou left a comment

Choose a reason for hiding this comment

brichet commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

martinRenou Feb 3, 2025 •

edited

Loading

brichet Feb 3, 2025 •

edited

Loading

brichet commented Feb 6, 2025 •

edited

Loading

brichet left a comment •

edited

Loading