Shapedata bugfix #5761
Shapedata bugfix #5761
Conversation
The hashcode and equals methods looked a bit unusual, thought it might be better to have a unit test for that.
|
@awalter17 spotted another ROI related bug, will fix it within this PR, too. |
See http://lists.openmicroscopy.org.uk/pipermail/ome-devel/2018-May/004214.html We can go back to using ROIFacility when ome/openmicroscopy#5761 is merged.
How do we know? Should we deprecate here and remove after 5.4? Update: Ha, actually that's what you did, 👍 |
| return roiShapes.get(new ROICoordinate(z, t)); | ||
| List<ShapeData> res = roiShapes.get(new ROICoordinate(z, t)); | ||
| res.addAll(roiShapes.get(new ROICoordinate(-1, -1))); | ||
| return res; |
There was a problem hiding this comment.
This also covers if only one of z or t is -1?
|
It seems that with this PR included, several UpdateService and RoiService integration tests underwent some regression - see https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-java/892/ |
|
Thanks @sbesson ! Strange that I didn't spot that earlier. |
| ROICoordinate end) { | ||
| List<List<ShapeData>> res = new ArrayList<List<ShapeData>>(); | ||
| res.addAll(roiShapes.subMap(start, end).values()); | ||
| res.add(roiShapes.get(new ROICoordinate(-1, -1))); |
There was a problem hiding this comment.
Can this end up adding a null? (If so, that's okay?)
There was a problem hiding this comment.
Yes, thanks, that needs a null check, too.
|
Integration tests are green, tested the Insight ROITool again, I think the PR is ready to merge. |
| List<ShapeData> allZT = roiShapes.get(new ROICoordinate(-1, -1)); | ||
| if (allZT != null) | ||
| res.addAll(allZT); | ||
| return res; |
There was a problem hiding this comment.
What about if only one of z or t is -1?
There was a problem hiding this comment.
Argh, that's true, these cases have to be considered, too.
There was a problem hiding this comment.
👍 Longer-term for the API might be nice to say something like: new ROICoordinate(new Optional(z), new Optional(t)) or new LenientROICoordinate(z, t) or new ROICoordinate(z, t, lenient=true)
| } | ||
|
|
||
|
Fixed issue #5761 (comment) and also deprecated |
| @@ -209,9 +209,11 @@ public void removeShapeData(ShapeData shape) | |||
| ROICoordinate coord = shape.getROICoordinate(); | |||
| List<ShapeData> shapeList; | |||
| shapeList = roiShapes.get(coord); | |||
There was a problem hiding this comment.
List<ShapeData> shapeList = roiShapes.get(coord); in one line would look better
|
Looks okay to me |
|
Merging for inclusion in 5.4.7 |
What this PR does
Allows to remove the C, Z and T of a Shape. Previously when a negative value was passed to
ShapeData.setC()(etc.) the value was set to0. With this PR passing a negative value will set the underlying Ice Shape object's C, Z, T tonull. And for the other way round, if the Shape object's C, Z, T isnull,ShapeData.getC()(etc.) will return-1, indicating that the value has not been set, so that getters and setters now also behave in a consistent way.Had to modify the
ROIDatapojo, too, e.g. remove methods likefirstPlane()(which isn't used anywhere), because these wouldn't work properly withnullCZT ROIs (which are supposed to be present on all planes).Also updated the integration test accordingly.
Another bug was noticed: The various setters (
setX(),setY(), etc.) on the ShapeData subclasses (RectangleData, etc.) didn't set thedirtyflag, that's why the ROIFacility didn't save them back to the server whensaveROIswas called. Fixed the setters and added an unit test for all setters of all ShapeData classes.Testing this PR
Related reading
http://lists.openmicroscopy.org.uk/pipermail/ome-devel/2018-May/004199.html
/cc @awalter17