Skip to content

[WIP] Create GISDocuments as untitled when no path provided #630

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 8 additions & 27 deletions python/jupytergis_lab/jupytergis_lab/notebook/gis_document.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,9 @@ class GISDocument(CommWidget):
"""
Create a new GISDocument object.

:param path: the path to the file that you would like to open. If not provided, a new empty document will be created.
:param path: the path to the file that you would like to open. If not provided, a new untitled document will be created.
"""

path: Optional[Path]

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't know the path when we create an untitled document; it could be untitled.jGIS or untitled99.jGIS. The Javascript side knows, but how do we get that info into Python land? See #629

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should require the path argument in the GISDocument, to avoid

Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise the untitled.jGIS should probably be created from the kernel itself.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, that's also what I commented in #629

def __init__(
self,
path: Optional[str | Path] = None,
Expand All @@ -59,17 +57,16 @@ def __init__(
pitch: Optional[float] = None,
projection: Optional[str] = None,
):
if isinstance(path, str):
path = Path(path)

self.path = path

comm_metadata = GISDocument._path_to_comm(str(self.path) if self.path else None)
Copy link
Member Author

Choose a reason for hiding this comment

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

We're only using this variable in one place.

if isinstance(path, Path):
path = str(path)

ydoc = Doc()

super().__init__(
comm_metadata=dict(ymodel_name="@jupytergis:widget", **comm_metadata),
comm_metadata={
"ymodel_name": "@jupytergis:widget",
**self._path_to_comm(path),
Copy link
Member Author

Choose a reason for hiding this comment

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

Instance methods can call class methods from self, which is less brittle than calling it from GISDocument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing from dict() to {} was just personal preference. No strong feeling.

},
ydoc=ydoc,
)

Expand Down Expand Up @@ -109,23 +106,7 @@ def layer_tree(self) -> List[str | Dict]:
"""
return self._layerTree.to_py()

def save_as(self, path: str | Path) -> None:
"""Save the document at a new path."""
if isinstance(path, str):
path = Path(path)

if path.name.lower().endswith(".qgz"):
_export_to_qgis(path)
self.path = path
return

if not path.name.lower().endswith(".jgis"):
path = Path(str(path) + ".jGIS")

path.write_text(json.dumps(self.to_py()))
self.path = path

def _export_to_qgis(self, path: str | Path) -> bool:
def export_to_qgis(self, path: str | Path) -> bool:
# Lazy import, jupytergis_qgis of qgis may not be installed
from jupytergis_qgis.qgis_loader import export_project_to_qgis

Expand Down
13 changes: 7 additions & 6 deletions python/jupytergis_lab/jupytergis_lab/notebook/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,13 @@ def test_remove_nonexistent_layer_raises(self):
self.doc.remove_layer("foo")


def test_save_as(tmp_path):
def test_untitled_doc(tmp_path):
os.chdir(tmp_path)

doc = GISDocument()
assert not list(tmp_path.iterdir())
GISDocument()
assert len(list(tmp_path.iterdir())) == 1
assert (tmp_path / "untitled.jGIS").is_file()

fn = "test.jgis"
doc.save_as(fn)
assert (tmp_path / fn).is_file()
GISDocument()
assert len(list(tmp_path.iterdir())) == 2
assert (tmp_path / "untitled1.jGIS").is_file()
Copy link
Member Author

@mfisher87 mfisher87 Apr 14, 2025

Choose a reason for hiding this comment

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

This seemed like the natural way to test this, but I believe because there is no JupyterLab instance running, the JavaScript code which actually creates the files is not being exercised. Is there a better way to test this? Please advise :)

12 changes: 7 additions & 5 deletions python/jupytergis_lab/src/notebookrenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,13 @@ export const notebookRendererPlugin: JupyterFrontEndPlugin<void> = {
});
}
} else {
// If the user did not provide a path, do not create
localPath = PathExt.join(
PathExt.dirname(currentWidgetPath),
'unsaved_project'
);
// If the user did not provide a path, create an untitled document
const model = await app.serviceManager.contents.newUntitled({
path: PathExt.dirname(currentWidgetPath),
type: 'file',
ext: '.jGIS'
});
localPath = model.path;
}

const sharedModel = drive!.sharedModelFactory.createNew({
Expand Down
Loading