-
Notifications
You must be signed in to change notification settings - Fork 24
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
Remove legacy datasets tab and use new compact route #6834
Conversation
…r cache when updating a dataset etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fm3 I integrated the compact datasets now for the most part. However, some bits are still missing which regard mutation. For mutation, the backend expects the full dataset object which doesn't exist in some places due to the compactness. I could adapt the code to always pull the entire dataset object before mutating it, but it feels a bit cumbersome and wasteful. How do you feel about adapting the update route so that a subset of the properties can be sent? Due to the undefined key vs missing key problem in scala, we could think about an interface where one explicitly sends the properties which should be sent.
As far as I see the following two cases are the important ones:
- moving a ds to another folder
- changing the tags of a ds
firstSegmentationLayer != null && supportsFallback | ||
? `?fallbackLayerName=${firstSegmentationLayer.name}` | ||
: ""; | ||
// todo: do in backend? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fm3 Do you think it would be feasible to have this functionality in the backend? The old code checked whether the dataset has a segmentation layer and generated a URL so that a fallback layer was created for the first segmentation layer. Since this create "button" is a link, the URL needs to exist before clicking on it (otherwise, middle click wouldn't work). The necessary information is not there anymore, though (due the compactness).
I would suggest to have something like ?autoFallback=true
so that the back-end does the functionality explained above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit adding this logic to the backend. Note that name
is now also optional (as it should follow the name of the segmentation layer if needed). The frontend-sent name has precedence, though.
I adapted admin_rest_api.ts to test this, but it does not yet support all use cases (i.e. the autoFallbackLayer boolean is not yet passed from the react router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent 👍 I just incorporated the flag everywhere. However, I noticed that the annotation creation will fail now when autoFallbackLayer is set to true even though there is no volume layer. Since the front-end doesn't know the existing layers, it cannot check this so I would hope the backend can simply ignore the parameter in that case. Does this make sense?
Yes, I think this can be done in the backend. I don’t have time this week but maybe let’s talk about a new api for mutating datasets next week. We’ll have to keep an eye on compatibility with older wk-libs versions, though. |
@philippotto I added the new route
description and displayname can be set to null. I have finally been able to convince scala to distinguish between null and absent keys, encoded with this Option[Option[String]]. I did this for only this route at this time, but we may reuse this functionality elsewhere later. Note that the webknossos-libs still use the old update request, I’ll create an issue there to use this new one. Then we may eventually be able to retire the old version. |
…eded anymore thanks to updateDatasetPartial
awesome news, thank you!
this will be a number in JSON, right?
This wasn't created yet, right? See my latest commit. I removed the |
I vaguely remember that you said that wk-libs didn't use this? The front-end doesn't use the old updateDataset route anymore. If you think, it can be removed from the backend, feel free to do so :) @daniel-wer I think, the PR is ready for review :) |
I think, I might have confused this with the |
I added scalableminds/webknossos-libs#874 to adapt the libs to use the new update route. The libs already don’t rely on any fields that are thrown out in the “compact” version of the dataset list route. That means, the non-compact version can be removed. However, I wrote #6862 to do this separately and not in this PR, to not make this change any bigger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM and worked very well for me 🎉
Not sure whether a backend review is needed. Otherwise feel free to approve @fm3.
@jstriebel Could you have a brief look at the backend? Changes include
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
URL of deployed dev instance (used for testing):
Steps to test:
Issues: