Add opacity as a top-level property for all layers #879
Conversation
|
|
for more information, see https://pre-commit.ci
|
Integration tests report: appsharing.space |
|
bot please update snapshots!!! |
There was a problem hiding this comment.
Thanks a lot for starting this!
Since this is a big schema change, we should probably bump the schema version and have some backward compatibility here https://github.com/geojupyter/jupytergis/blob/main/python/jupytergis_core/jupytergis_core/jgis_ydoc.py#L56
Something like: if the schema version is the old one (when opacity was in the layer parameters) then patch valueDict['layers'] there accordingly by moving the opacity attribute out of the parameters.
This will allow us to continue opening the old file format without error.
for more information, see https://pre-commit.ci
| if file_version < Version("0.8.1"): | ||
| for _layer_id, layer in valueDict.get("layers", {}).items(): |
There was a problem hiding this comment.
@martinRenou currently we are using the latest JupyterGIS version 0.8.1 to update the schema. But I am wondering if anyone has an old version, how could we handle that situation? I also use SCHEMA_VERSION instead of latest, but it didn't work.
There was a problem hiding this comment.
Thanks for your work on this, Nakul! Haven't had time to go deep on this yet, but some questions immediately came to mind. My biggest thought right now is that we should push as much of this as possible into the sharedModel API, as it seems to not be robust enough to support what we're trying to do. Specifically, I think we need the ability to deep-merge state updates (and a DeepPartial generic type to support that).
i.e.
// new update method
updateObject(
id: string,
value: DeepPartial<IJGISLayer> | DeepPartial<IJGISSource>,
) {
// DEEP merge `value` with the current state of object.
}
// ...
// updated call would _only_ change the specified properties
this.props.model.sharedModel.updateObject(myLayerId, {
opacity: true,
parameters: {
symbologyState: {
colorRamp: "viridis"
}
},
});(or, maybe better, use the updateLayer, updateSource methods for this)
How do you all feel about this? @martinRenou @arjxn-py @gjmooney more thoughts on this in a comment below.
| @@ -111,6 +111,14 @@ export class CreationForm extends React.Component<ICreationFormProps, any> { | |||
| layerSchema['required'] = ['name', ...layerSchema['required']]; | |||
| layerSchema['properties'] = { | |||
| name: { type: 'string', description: 'The name of the layer' }, | |||
| opacity: { | |||
There was a problem hiding this comment.
Do we need this? It should already be part of the object, right?
| maximum: 1, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
We can omit this since it's defined in the jsonSchema, right?
| @@ -112,6 +112,7 @@ export const LayerBrowserComponent: React.FC<ILayerBrowserDialogProps> = ({ | |||
| source: sourceId, | |||
| }, | |||
| visible: true, | |||
| opacity: 1.0, | |||
There was a problem hiding this comment.
If you want, we can omit this since the default is 1
| this.props.layer, | ||
| params, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The fact that a change like this is necessary makes me wonder if we're on the wrong path (I'm not saying your approach is wrong, more that our architecture needs work ;) ) with this change. We have an API for updating layer/source parameters, and moving opacity out of the parameters key is causing us to work around that API. That makes this change much more extensive than I expected. Perhaps the right path here is to update the sharedModel API to support more selective updates.
Perhaps the updateObjectParameters method could be updated to look like this?
updateObject(
id: string,
value: Partial<IJGISLayer> | Partial<IJGISSource>,
) {
// DEEP merge `value` with the current state of object.
}Perhaps it's making our life a bit more complicated to have updateObjectParameters capable of acting on layers or sources, and we should instead have updateLayer, updateSource, etc. (these already exist, but they require you to pass a full layer object in, and replace it in the model) as the sole API method for doing updates of these nested objects?
Description
This PR adds
opacityas a generic property of allJGISlayers, defined once in the schemapackages/schema/src/schema/project/jgis.json.Now
opacityis the exact same type of property asvisibleScreencast.From.2025-08-19.21-28-23.mp4
Closes #829
Checklist
Resolves #XXX.Failing lint checks can be resolved with:
pre-commit run --all-filesjlpm run lint📚 Documentation preview: https://jupytergis--879.org.readthedocs.build/en/879/
💡 JupyterLite preview: https://jupytergis--879.org.readthedocs.build/en/879/lite